Add selinuxfs to be mounted in the container #344
Conversation
lib/src/install.rs
Outdated
| @@ -865,37 +865,42 @@ pub(crate) fn setup_tmp_mounts() -> Result<()> { | |||
| #[context("Ensuring sys mounts")] | |||
| pub(crate) fn setup_sys_mounts() -> Result<()> { | |||
| tracing::debug!("Setting up sys mounts"); | |||
| let filesystems = vec
There was a problem hiding this comment.
Minor nit, we don't need to allocate here, dropping the vec! will get us a static slice.
lib/src/install.rs
Outdated
| @@ -865,37 +865,42 @@ pub(crate) fn setup_tmp_mounts() -> Result<()> { | |||
| #[context("Ensuring sys mounts")] | |||
| pub(crate) fn setup_sys_mounts() -> Result<()> { | |||
There was a problem hiding this comment.
Offhand this looks sane, but we can also now drop the code elsewhere which was mounting selinuxfs right?
There was a problem hiding this comment.
Should this just become a function that takes a /sys subpath instead of looping? That seems to make more sense now that I look at it... Or as you said, we can just drop the other code that does this, and just make sure this is called ahead of where that was done before.
lib/src/install.rs
Outdated
| ]; | ||
| for (fstype, fspath) in filesystems { | ||
| let rootfs = format!("/proc/1/root/{fspath}"); | ||
| // Does efivars even exist in the host? If not, we are |
There was a problem hiding this comment.
Comment is outdated now, how about e.g. // Does this mountpoint exist on the host?
lib/src/install.rs
Outdated
| if inspect.is_ok() { | ||
| tracing::trace!("Already have efivarfs {root_efivars}"); | ||
| return Ok(()); | ||
| // First of all, does the container already have the mount? |
There was a problem hiding this comment.
So I like the change conceptually to checking for a mountpoint...but unfortunately the is_mountpoint we have here right now is not 100% reliable.
Perhaps the simplest thing really is to check if the directory is non-empty here too?
lib/src/install.rs
Outdated
| @@ -750,7 +756,7 @@ pub(crate) fn reexecute_self_for_selinux_if_needed( | |||
| // already queried by something else (e.g. glib's constructor), we would also need | |||
| // to re-exec. But, selinux_ensure_install does that unconditionally right now too, | |||
| // so let's just fall through to that. | |||
| crate::lsm::container_setup_selinux()?; | |||
| let _ = setup_sys_mount("selinuxfs", SELINUXFS)?; | |||
There was a problem hiding this comment.
The let _ here looks unnecessary.
lib/src/lsm.rs
Outdated
| @@ -123,6 +123,7 @@ pub(crate) fn selinux_ensure_install_or_setenforce() -> Result<Option<SetEnforce | |||
|
|
|||
| /// Ensure that /sys/fs/selinux is mounted, and ensure we're running | |||
| /// as install_t. | |||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
We can delete this function instead of marking it dead, no?
lib/src/install.rs
Outdated
| @@ -1002,7 +1001,7 @@ async fn prepare_install( | |||
| super::cli::ensure_self_unshared_mount_namespace().await?; | |||
| } | |||
|
|
|||
| setup_sys_mounts()?; | |||
| let _ = setup_sys_mount("efivarfs", EFIVARFS)?; | |||
lib/src/install.rs
Outdated
| // Check that the path that should be mounted is even populated. | ||
| // Since we are dealing with /sys mounts here, if it's populated, | ||
| // we can be at least a little certain that it's mounted. | ||
| if Path::new(fspath).try_exists()? && !std::fs::read_dir(fspath)?.next().is_none() { |
There was a problem hiding this comment.
Minor nit, I think std::fs::read_dir(fspath)?.next().is_some() is easier to read (avoids double negation)
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>
As a follow-on to #302, we want to also mount the selinuxfs special filesystem if the host also has that filesystem mounted.
Related #303