Ensure that efivarfs is mounted in the container #302

bcrochet · 2024-02-06T17:04:38Z

Especially on ARM, which utilizes UEFI for booting in most cases, it is important that the /sys/firmware/efi/efivars be mounted and populated, otherwise bootc will fail to complete a to-filesystem installation.

This patch attempts a mount as long as the hosts efivars directory has an entry, signifying the system is at least capable of UEFI.

Note that it is sufficient to just attempt to mount efivars. If it's already mounted elsewhere, it triggers the mount to be made at the /sys location.

Fixes #291

openshift-ci · 2024-02-06T17:04:49Z

Hi @bcrochet. Thanks for your PR.

I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cgwalters · 2024-02-06T19:23:46Z

lib/src/install.rs

+    tracing::debug!("Setting up sys mounts");
+    // First of all, does efivars even exist in the host? If not, we are
+    // not dealing with an EFI system
+    let _ = rustix::fs::lstat("/proc/1/root/sys/firmware/efi/efivars")?;


This will cause us to hard fail though if it doesn't exist, which is not what we want.

Right. Forgot to test that case on a BIOS system.

cgwalters · 2024-02-06T19:28:30Z

lib/src/install.rs

+    let _ = rustix::fs::lstat("/proc/1/root/sys/firmware/efi/efivars")?;
+
+    // Now, let's find out if it's populated
+    let od = rustix::fs::open(


So far typically here we use std::fs unless there's a reason to use rustix.

Understood.

cgwalters · 2024-02-06T19:29:27Z

lib/src/install.rs

+    // Now, let's find out if it's populated
+    let od = rustix::fs::open(
+        "/proc/1/root/sys/firmware/efi/efivars",
+        rustix::fs::OFlags::DIRECTORY,


In particular, this is missing OFlags::CLOEXEC which is generally a good idea. While this codebase isn't security sensitive in general, see e.g. https://github.com/opencontainers/runc/releases/tag/v1.1.12 which was a bad CVE in runc from leaking fds and handling them incorrectly.

cgwalters · 2024-02-06T19:31:21Z

lib/src/install.rs

+    let dir_populated = rustix::fs::Dir::read_from(od)?.next().is_some();
+    if dir_populated {


Combining the above I think something just like:

let efivars = "/proc/1/root/sys/firmware/efi/efivars"; if !Path::new(efivars).try_exists()? { return Ok(()); } if std::fs::read_dir(efivars)?.next().is_none() { return Ok(()); } ...rest of code

or so?

Another option is https://docs.rs/cap-std-ext/latest/cap_std_ext/dirext/trait.CapStdExtDirExt.html#tymethod.open_dir_optional which we use in a few places in this project, and is intended for this use case.

cgwalters · 2024-02-06T19:31:40Z

lib/src/install.rs

+        // This looks to be a workaround for a problem elsewhere. Not sure if
+        // it's in podman, but just attempting to mount this is enough to trigger
+        // it being populated even when there is an existing mount.
+        // Given that, we will just try to mount and ignore the result.


I don't quite understand the rationale behind ignoring the error. Is it erroring out now?

I will double-check tomorrow. But what I think I was seeing today was that when bootc install to-filesystem --replace=alongside /target was run... On the first invocation /sys/firmware/efi/efivars was unpopulated, but {/proc/1/root,/target}/sys/firmware/efi/efivars are populated, and the mount command fails, saying "efivarfs already mounted at /target/sys/firmware/efi/efivars".

But once that was run, and failed out, "magically" /sys/firmware/efi/efivars was now populated, and the bootc run would complete without error.

I'm totally game with working on this further to make it more resilient. I also think this is some kind of bug elsewhere. I'm just not sure where to point the finger.

So, my rationale here was that it didn't matter if the mount command itself succeeded, but just that it was run.

FWIW I did verify that adding -v /sys:/sys:rslave to our podman run invocation gets us the efivars mount (as expected) alongside others that we also currently mount manually like selinuxfs.

and the mount command fails, saying "efivarfs already mounted at /target/sys/firmware/efi/efivars".

That's weird indeed. In a quick test I can't reproduce that with a simple

$ podman run --rm -ti --privileged --pid=host quay.io/fedora/fedora:39 # mount -t efivarfs efivars /sys/firmware/efi/efivars

Probably worth trying to dig into what's different about what we're doing here.

The proper direction may be then to just do a doc change for this, while we investigate the origin of the error.

Is it perhaps ARM specific?

Also, I think the other key is that -v /:/target is used. As efivarfs will be mounted in /target.

And of course I'm now unable to reproduce this scenario. 🙄

I'll update this patch with all of the suggestions and stop ignoring this error, and we can go from there.

Also, I think the other key is that -v /:/target is used. As efivarfs will be mounted in /target.

Ahh yes, tricky. In the case that we have the full target rootfs, we could in our container simply do e.g. mount --rbind /target/sys /sys or so.

Actually for this "alongside" case it would definitely help there if we still didn't need to pass in the -v /:/target even and just got the root filesystem by jumping out of the mount namespace.

cgwalters · 2024-02-07T13:16:32Z

/ok-to-test

cgwalters · 2024-02-12T20:56:17Z

lib/src/install.rs

+        let inspect = crate::mount::inspect_filesystem(path);
+        if inspect.is_ok() {


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.

cgwalters · 2024-02-12T20:57:19Z

lib/src/install.rs

+    }
+
+    // This means the host has this mounted, so we should mount it too
+    tracing::debug!("mounting efivarfs");


Very minor nit, I doubt we need this debug invocation because Task::new_and_run will already show something like this with the tracing::debug!("exec: {cmd:?}");

cgwalters · 2024-02-12T21:00:04Z

lib/src/install.rs

+#[context("Ensuring sys mounts")]
+pub(crate) fn setup_sys_mounts() -> Result<()> {


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)

cgwalters · 2024-02-12T21:00:26Z

/ok-to-test

cgwalters

This is OK as is, we can do other fixes as followups. Does this solve the problem in your testing?

bcrochet · 2024-02-12T21:15:51Z

This is OK as is, we can do other fixes as followups. Does this solve the problem in your testing?

So far, yes.

I'll do the cleanups suggested.

Especially on ARM, which utilizes UEFI for booting in most cases, it is important that the /sys/firmware/efi/efivars be mounted and populated, otherwise bootc will fail to complete a to-filesystem installation. This patch attempts a mount as long as the hosts efivars directory has an entry, signifying the system is at least capable of UEFI. Note that it is sufficient to just attempt to mount efivars. If it's already mounted elsewhere, it triggers the mount to be made at the /sys location. Fixes containers#291 Signed-off-by: Brad P. Crochet <brad@redhat.com>

As a follow-on to containers#302, we want to also mount the selinuxfs special filesystem if the host also has that filesystem mounted. Related containers#303 Signed-off-by: Brad P. Crochet <brad@redhat.com>

openshift-ci bot added the needs-ok-to-test label Feb 6, 2024

github-actions bot added the area/install Issues related to `bootc install` label Feb 6, 2024

cgwalters reviewed Feb 6, 2024

View reviewed changes

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Feb 7, 2024

cgwalters mentioned this pull request Feb 8, 2024

bootc install to-disk --disable-selinux does not disable selinux when the host has selinux disabled #303

Closed

bcrochet force-pushed the fixes/291 branch 2 times, most recently from 7e12123 to cf1ff27 Compare February 12, 2024 18:18

cgwalters reviewed Feb 12, 2024

View reviewed changes

cgwalters approved these changes Feb 12, 2024

View reviewed changes

bcrochet force-pushed the fixes/291 branch from cf1ff27 to 408f16e Compare February 12, 2024 21:50

cgwalters approved these changes Feb 12, 2024

View reviewed changes

cgwalters enabled auto-merge February 12, 2024 22:05

cgwalters merged commit e4c2971 into containers:main Feb 12, 2024
11 checks passed

bcrochet mentioned this pull request Feb 14, 2024

Add selinuxfs to be mounted in the container #344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that efivarfs is mounted in the container #302

Ensure that efivarfs is mounted in the container #302

bcrochet commented Feb 6, 2024

openshift-ci bot commented Feb 6, 2024

cgwalters Feb 6, 2024

bcrochet Feb 7, 2024

cgwalters Feb 6, 2024

bcrochet Feb 7, 2024

cgwalters Feb 6, 2024

cgwalters Feb 6, 2024

cgwalters Feb 6, 2024

cgwalters Feb 6, 2024

bcrochet Feb 7, 2024

cgwalters Feb 7, 2024

bcrochet Feb 7, 2024

bcrochet Feb 7, 2024

cgwalters Feb 7, 2024

cgwalters commented Feb 7, 2024

cgwalters Feb 12, 2024

cgwalters Feb 12, 2024

cgwalters Feb 12, 2024

cgwalters commented Feb 12, 2024

cgwalters left a comment

bcrochet commented Feb 12, 2024

		let dir_populated = rustix::fs::Dir::read_from(od)?.next().is_some();
		if dir_populated {

		let inspect = crate::mount::inspect_filesystem(path);
		if inspect.is_ok() {

		#[context("Ensuring sys mounts")]
		pub(crate) fn setup_sys_mounts() -> Result<()> {

Ensure that efivarfs is mounted in the container #302

Ensure that efivarfs is mounted in the container #302

Conversation

bcrochet commented Feb 6, 2024

openshift-ci bot commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Feb 12, 2024

cgwalters left a comment

Choose a reason for hiding this comment

bcrochet commented Feb 12, 2024