Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that efivarfs is mounted in the container #302

Merged
merged 1 commit into from
Feb 12, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions lib/src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use std::io::BufWriter;
use std::io::Write;
use std::os::fd::AsFd;
use std::os::unix::process::CommandExt;
use std::path::Path;
use std::process::Command;
use std::str::FromStr;
use std::sync::Arc;
Expand Down Expand Up @@ -859,6 +860,44 @@ pub(crate) fn setup_tmp_mounts() -> Result<()> {
Ok(())
}

/// By default, podman/docker etc. when passed `--privileged` mount `/sys` as read-only,
/// but non-recursively. We selectively grab sub-filesystems that we need.
#[context("Ensuring sys mounts")]
pub(crate) fn setup_sys_mounts() -> Result<()> {
Comment on lines +865 to +866
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense as a name, though let's add some commentary here, something like:

/// By default, podman/docker etc. when passed `--privileged` mount `/sys` as read-only, but non-recursively.  We selectively grab sub-filesystems that we need.

I think your naming is right though because it'd make sense to also handle selinuxfs exactly this same way. (Don't need to do it in this PR though)

tracing::debug!("Setting up sys mounts");

let root_efivars = "/sys/firmware/efi/efivars";
let efivars = format!("/proc/1/root/{root_efivars}");
// Does efivars even exist in the host? If not, we are
// not dealing with an EFI system
if !Path::new(efivars.as_str()).try_exists()? {
return Ok(());
}

// Now, let's find out if it's populated
if std::fs::read_dir(efivars)?.next().is_none() {
return Ok(());
}

// First of all, does the container already have the mount?
let path = Utf8Path::new(root_efivars);
if path.try_exists()? {
tracing::debug!("Check if efivarfs already mount");
let inspect = crate::mount::inspect_filesystem(path);
if inspect.is_ok() {
Comment on lines +886 to +887
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK as is, but is_ok() is a pattern to try to avoid as a general rule because it "swallows" all forms of errors (if e.g. the findmnt binary failed to be executed, not just if the target isn't a mountpoint).

Unfortunately detecting if something is a mountpoint is surprisingly tricky (we use a helper ostree_ext::mountutil::is_mountpoint in other places here). It looks like we could extend our findmnt wrapper to support passing -T/--target, which would always succeed (in general), and then we can check if the filesystem type is efivarfs.

tracing::trace!("Already have efivarfs {root_efivars}");
return Ok(());
}
}

// This means the host has this mounted, so we should mount it too
Task::new_and_run(
"Mounting efivarfs /sys/firmware/efi/efivars",
"mount",
["-t", "efivarfs", "efivars", "/sys/firmware/efi/efivars"],
)
}

/// Verify that we can load the manifest of the target image
#[context("Verifying fetch")]
async fn verify_target_fetch(imgref: &ostree_container::OstreeImageReference) -> Result<()> {
Expand Down Expand Up @@ -954,6 +993,8 @@ async fn prepare_install(
super::cli::ensure_self_unshared_mount_namespace().await?;
}

setup_sys_mounts()?;

// Now, deal with SELinux state.
let (override_disable_selinux, setenforce_guard) =
reexecute_self_for_selinux_if_needed(&source, config_opts.disable_selinux)?;
Expand Down
Loading