bootloader: Chroot into composefs image root for bootupd installs

Closes: #2270

Like the ostree backend, run bootupctl inside a chroot of the target
system rather than the host, so we use the bootupd binaries and
plugins shipped in the deployed container image. This is needed for
use cases like Anaconda.

This turned out to be fairly complicated though in terms of
how we set up the bind mounts; see the code for details.

Generated-by: https://github.com/cgwalters/#llms
Signed-off-by: Colin Walters <walters@verbum.org>

Author: cgwalters

crates/lib/src/bootc_composefs/boot.rs

@@ -1196,11 +1196,21 @@ pub(crate) fn setup_composefs_uki_boot(
 /// `<tmpdir>/tmp` to provide a writable scratch area for tools invoked with
 /// `--root`.
 ///
-/// Drop order matters: the ESP and tmpfs guards are declared before `composefs`
-/// so they are unmounted (and flushed) before the composefs root is detached.
+/// Drop order matters: the tmpfs guard is declared before `composefs` so it
+/// is unmounted (and flushed) before the composefs root is detached.
 pub(crate) struct MountedImageRoot {
-    // Unmounted before `composefs` on drop; ESP before tmp (inner before outer).
-    _esp: bootc_mount::tempmount::MountGuard,
+    // The ESP's device path. The ESP is intentionally *not* mounted here for
+    // our whole lifetime; it's mounted on demand via `with_esp()` instead.
+    // That's because `install_via_bootupd` runs bootupd's install tooling
+    // inside a `ChrootCmd`, which unshares the mount namespace and
+    // recursively self-binds the chroot directory (our `root_path()`) onto
+    // itself. If the ESP were already mounted at `<root_path>/boot` when
+    // that happens, the self-bind would duplicate it into an
+    // orphaned/shadowed mountinfo entry that's still visible to naive
+    // `findmnt`-based scans (like bootupd's), causing it to pick the wrong
+    // filesystem UUID. See the commit introducing this comment for details.
+    esp_device: Utf8PathBuf,
+    // Unmounted before `composefs` on drop.
     _tmp: bootc_mount::tempmount::MountGuard,
     composefs: TempMount,
     pub(crate) esp_subdir: &'static str,
@@ -1236,10 +1246,6 @@ impl MountedImageRoot {
         // unconditionally mount the ESP at /boot for now.
         let esp_subdir = "boot";
 
-        let esp_path = composefs.dir.path().join(esp_subdir);
-        let esp =
-            mount_esp_at(&esp_part.path(), esp_path).context("Mounting ESP into composefs root")?;
-
         // Mount a tmpfs over /tmp so that tools invoked with --root have a
         // writable scratch area without touching the read-only EROFS root.
         let tmp_path = composefs.dir.path().join("tmp");
@@ -1253,7 +1259,7 @@ impl MountedImageRoot {
         .context("Mounting tmpfs into composefs root")?;
 
         Ok(Self {
-            _esp: esp,
+            esp_device: esp_part.path().into(),
             _tmp: tmp,
             composefs,
             esp_subdir,
@@ -1271,12 +1277,29 @@ impl MountedImageRoot {
     }
 
     /// Open the mounted ESP as a capability-safe directory.
+    ///
+    /// This only succeeds while the ESP is actually mounted, i.e. from
+    /// within (or after) a call to [`Self::with_esp`].
     pub(crate) fn open_esp_dir(&self) -> Result<Dir> {
         self.composefs
             .fd
             .open_dir(self.esp_subdir)
             .with_context(|| format!("Opening ESP at /{}", self.esp_subdir))
     }
+
+    /// Mount the ESP at `<tmpdir>/<esp_subdir>` for the duration of `f`, then
+    /// unmount it again. Nothing is mounted there outside of calls to this
+    /// method, so callers that need to invoke bootloader-install tooling
+    /// that itself creates a private mount namespace (e.g. via `ChrootCmd`)
+    /// can safely do so without risking this mount being shadowed/duplicated
+    /// by that tooling's own mount-namespace setup.
+    pub(crate) fn with_esp<T>(&self, f: impl FnOnce(&Dir) -> Result<T>) -> Result<T> {
+        let esp_path = self.root_path().join(self.esp_subdir);
+        let _guard = mount_esp_at(self.esp_device.as_str(), esp_path)
+            .context("Mounting ESP into composefs root")?;
+        let dir = self.open_esp_dir()?;
+        f(&dir)
+    }
 }
 
 pub struct SecurebootKeys {
@@ -1381,56 +1404,75 @@ pub(crate) async fn setup_composefs_boot(
         postfetch.detected_bootloader,
         Bootloader::Grub | Bootloader::GrubCC
     ) {
+        let chroot_target = Utf8Path::from_path(mounted_root.root_path())
+            .ok_or_else(|| anyhow!("composefs tmpdir path is not valid UTF-8"))?;
+        // Like the ostree backend, bind the physical root's real /boot (an
+        // ordinary ext4/xfs/... directory, not yet populated with kernels at
+        // this point) into the chroot. This gives bootupd both a correct
+        // filesystem to inspect for `--write-uuid` (rather than the ESP,
+        // which is otherwise mounted at the composefs root's own /boot) and
+        // an empty `boot/efi` directory for its EFI component to discover
+        // and mount the real ESP into, exactly as it would on ostree.
+        let bind_boot_path = root_setup.physical_root_path.join("boot");
         crate::bootloader::install_via_bootupd(
             &root_setup.device_info,
             &root_setup.physical_root_path,
             &state.config_opts,
-            None,
+            Some(chroot_target),
+            Some(bind_boot_path.as_path()),
         )?;
 
         // FIXME: Remove this hack once we have support in bootupd
         if matches!(postfetch.detected_bootloader, Bootloader::GrubCC) {
+            // bootupctl wrote this under the physical root's real /boot (via
+            // the bind mount above), not under the composefs root.
             root_setup
                 .physical_root
-                .remove_dir_all("boot/grub2")
+                .remove_all_optional("boot/grub2")
                 .context("removing grub2")?;
 
-            let (os_id, ..) = parse_os_release(mounted_root.dir())?
-                .ok_or_else(|| anyhow::anyhow!("Failed to parse os-release"))?;
-
-            let dir = format!("EFI/{os_id}");
-
-            // Files are in EFI/<os-name>/
-            let efis_dir = mounted_root
-                .open_esp_dir()
-                .context("opening esp")?
-                .open_dir(&dir)
-                .with_context(|| format!("Opening {dir}"))?;
-
-            efis_dir
-                .remove_file_optional("bootuuid.cfg")
-                .context("Removing bootuuid.cfg")?;
-            efis_dir
-                .remove_file_optional("grub.cfg")
-                .context("Removing grub.cfg")?;
-
-            let final_name = match std::env::consts::ARCH {
-                "x86_64" => "grubx64.efi",
-                "aarch64" => "grubaa64-cc.efi",
-                arch => anyhow::bail!("GrubCC not supported for: {arch}"),
-            };
+            // install_via_bootupd above has already returned, so it's safe
+            // to mount the ESP here for the duration of this cleanup.
+            mounted_root.with_esp(|esp_dir| {
+                let (os_id, ..) = parse_os_release(mounted_root.dir())?
+                    .ok_or_else(|| anyhow::anyhow!("Failed to parse os-release"))?;
+
+                let dir = format!("EFI/{os_id}");
+
+                // Files are in EFI/<os-name>/
+                let efis_dir = esp_dir
+                    .open_dir(&dir)
+                    .with_context(|| format!("Opening {dir}"))?;
+
+                efis_dir
+                    .remove_file_optional("bootuuid.cfg")
+                    .context("Removing bootuuid.cfg")?;
+                efis_dir
+                    .remove_file_optional("grub.cfg")
+                    .context("Removing grub.cfg")?;
+
+                let final_name = match std::env::consts::ARCH {
+                    "x86_64" => "grubx64.efi",
+                    "aarch64" => "grubaa64-cc.efi",
+                    arch => anyhow::bail!("GrubCC not supported for: {arch}"),
+                };
+
+                mounted_root
+                    .dir()
+                    .copy("usr/lib/grub-cc/grub-cc.efi", &efis_dir, final_name)
+                    .context("Copying grub-cc binary")?;
 
-            mounted_root
-                .dir()
-                .copy("usr/lib/grub-cc/grub-cc.efi", &efis_dir, final_name)
-                .context("Copying grub-cc binary")?;
+                Ok(())
+            })?;
         }
     } else {
-        crate::bootloader::install_systemd_boot(
-            &mounted_root,
-            &state.config_opts,
-            get_secureboot_keys(mounted_root.dir(), BOOTC_AUTOENROLL_PATH)?,
-        )?;
+        mounted_root.with_esp(|_esp_dir| {
+            crate::bootloader::install_systemd_boot(
+                &mounted_root,
+                &state.config_opts,
+                get_secureboot_keys(mounted_root.dir(), BOOTC_AUTOENROLL_PATH)?,
+            )
+        })?;
     }
 
     let Some(entry) = entries.iter().next() else {

crates/lib/src/bootloader.rs

@@ -134,12 +134,23 @@ fn bootupd_supports_filesystem(chroot_target: Option<&Utf8Path>) -> Result<bool>
 ///
 /// For older bootupd versions that lack `--filesystem` we fall back to the
 /// legacy `--device <device_path> <rootfs>` invocation.
+///
+/// If `bind_boot_path` is set, the given host path is bind-mounted onto
+/// `/boot` inside the chroot.  Both the ostree and composefs backends use
+/// this to expose the physical root's real `/boot` inside their respective
+/// chroots, since neither chroot target (an ostree deployment, or a mounted
+/// composefs image) has a `/boot` backed by the real root filesystem on its
+/// own.  This matters because bootupd derives the UUID it writes for
+/// `--write-uuid` from whatever filesystem is mounted at `<chroot>/boot`,
+/// and looks for an empty `boot/efi` directory there to discover and mount
+/// the real ESP into.
 #[context("Installing bootloader")]
 pub(crate) fn install_via_bootupd(
     device: &bootc_blockdev::Device,
     rootfs: &Utf8Path,
     configopts: &crate::install::InstallConfigOpts,
     chroot_target: Option<&Utf8Path>,
+    bind_boot_path: Option<&Utf8Path>,
 ) -> Result<()> {
     let verbose = std::env::var_os("BOOTC_BOOTLOADER_DEBUG").map(|_| "-vvvv");
     // bootc defaults to only targeting the platform boot method.
@@ -192,7 +203,16 @@ pub(crate) fn install_via_bootupd(
         bootupd_args.push(rootfs_mount);
     } else {
         tracing::debug!("bootupd supports --filesystem");
-        bootupd_args.extend(["--filesystem", rootfs_mount]);
+        // Inside a chroot the physical root is bind-mounted at /sysroot (see
+        // below) so bootupd's own device resolution (via lsblk) sees a real
+        // block-backed path. This matters for composefs, where the chroot's
+        // own "/" is a virtual composefs mount with no backing block device.
+        let filesystem_path = if chroot_target.is_some() {
+            "/sysroot"
+        } else {
+            rootfs_mount
+        };
+        bootupd_args.extend(["--filesystem", filesystem_path]);
         bootupd_args.push(rootfs_mount);
     }
 
@@ -201,7 +221,6 @@ pub(crate) fn install_via_bootupd(
     // deployment, without requiring a user namespace (which fails under
     // qemu-user — see <https://github.com/bootc-dev/bootc/issues/2111>).
     if let Some(target_root) = chroot_target {
-        let boot_path = rootfs.join("boot");
         let rootfs_path = rootfs.to_path_buf();
 
         tracing::debug!("Running bootupctl via chroot in {}", target_root);
@@ -211,10 +230,19 @@ pub(crate) fn install_via_bootupd(
         let mut chroot_args = vec!["bootupctl"];
         chroot_args.extend(bootupd_args);
 
-        let mut cmd = ChrootCmd::new(target_root)
-            // Bind mount /boot from the physical target root so bootupctl can find
-            // the boot partition and install the bootloader there
-            .bind(&boot_path, &"/boot");
+        let mut cmd = ChrootCmd::new(target_root);
+        // Bind mount /boot from the physical target root so bootupctl can find
+        // the boot partition and install the bootloader there. This is a
+        // non-recursive bind: the physical root's /boot may itself have the
+        // ESP mounted at boot/efi (see clean_boot_directories()), and we
+        // don't want that mount to be dragged along, since bootupd's own EFI
+        // component expects to find an empty boot/efi directory to mount the
+        // real ESP onto itself (see MountedImageRoot::with_esp()). A stray
+        // nested ESP mount there has also been observed to confuse grub-probe
+        // into embedding the wrong root device in the BIOS boot prefix.
+        if let Some(boot_path) = &bind_boot_path {
+            cmd = cmd.bind_flat(boot_path, &"/boot");
+        }
 
         // Only bind mount the physical root at /sysroot when using --filesystem;
         // bootupd needs it to resolve backing block devices via lsblk.

crates/lib/src/install.rs

@@ -1873,11 +1873,13 @@ async fn install_with_sysroot(
                     .clone()
                     .unwrap_or(rootfs.physical_root_path.clone());
                 let chroot_target = root_path.join(deployment_path.as_str());
+                let bind_boot_path = root_path.join("boot");
                 crate::bootloader::install_via_bootupd(
                     &rootfs.device_info,
                     &root_path,
                     &state.config_opts,
                     Some(chroot_target.as_path()),
+                    Some(bind_boot_path.as_path()),
                 )?;
             }
             Bootloader::Systemd | Bootloader::GrubCC => {

crates/utils/src/chroot.rs

@@ -9,7 +9,9 @@ use std::process::Command;
 
 use anyhow::{Context, Result};
 use cap_std_ext::camino::Utf8Path;
-use rustix::mount::{MountFlags, MountPropagationFlags, mount, mount_bind_recursive, mount_change};
+use rustix::mount::{
+    MountFlags, MountPropagationFlags, mount, mount_bind, mount_bind_recursive, mount_change,
+};
 use rustix::process::{chdir, chroot};
 use rustix::thread::{UnshareFlags, unshare_unsafe};
 
@@ -21,8 +23,8 @@ use crate::CommandRunExt;
 pub struct ChrootCmd<'a> {
     /// The target directory to use as root for the chroot.
     chroot_path: Cow<'a, Utf8Path>,
-    /// Bind mounts in format (host source, chroot-relative target).
-    bind_mounts: Vec<(&'a str, &'a str)>,
+    /// Bind mounts in format (host source, chroot-relative target, recursive).
+    bind_mounts: Vec<(&'a str, &'a str, bool)>,
     /// Environment variables to set on the spawned command.
     env_vars: Vec<(&'a str, &'a str)>,
 }
@@ -37,15 +39,31 @@ impl<'a> ChrootCmd<'a> {
         }
     }
 
-    /// Add a bind mount from `source` (on the host) to `target` (a path
-    /// inside the chroot, e.g. `/boot`).
+    /// Add a recursive bind mount from `source` (on the host) to `target` (a
+    /// path inside the chroot, e.g. `/sysroot`). Any mounts nested under
+    /// `source` on the host (e.g. an ESP mounted under a `/boot` source) are
+    /// also visible under `target` inside the chroot.
     pub fn bind(
         mut self,
         source: &'a impl AsRef<Utf8Path>,
         target: &'a impl AsRef<Utf8Path>,
     ) -> Self {
         self.bind_mounts
-            .push((source.as_ref().as_str(), target.as_ref().as_str()));
+            .push((source.as_ref().as_str(), target.as_ref().as_str(), true));
+        self
+    }
+
+    /// Add a non-recursive bind mount from `source` (on the host) to
+    /// `target` (a path inside the chroot). Unlike [`Self::bind`], mounts
+    /// nested under `source` on the host are *not* carried over into the
+    /// chroot, leaving any mountpoint directories beneath `target` empty.
+    pub fn bind_flat(
+        mut self,
+        source: &'a impl AsRef<Utf8Path>,
+        target: &'a impl AsRef<Utf8Path>,
+    ) -> Self {
+        self.bind_mounts
+            .push((source.as_ref().as_str(), target.as_ref().as_str(), false));
         self
     }
 
@@ -91,14 +109,18 @@ impl<'a> ChrootCmd<'a> {
         let sys_target = CString::new(sys_target.as_str())?;
         let run_target = CString::new(run_target.as_str())?;
 
-        let user_binds: Vec<(CString, CString)> = self
+        let user_binds: Vec<(CString, CString, bool)> = self
             .bind_mounts
             .iter()
-            .map(|(src, tgt)| -> Result<_> {
+            .map(|(src, tgt, recursive)| -> Result<_> {
                 let tgt_in_chroot = self.chroot_path.join(tgt.trim_start_matches('/'));
                 create_dir_all(&tgt_in_chroot)
                     .with_context(|| format!("Creating bind target {tgt_in_chroot}"))?;
-                Ok((CString::new(*src)?, CString::new(tgt_in_chroot.as_str())?))
+                Ok((
+                    CString::new(*src)?,
+                    CString::new(tgt_in_chroot.as_str())?,
+                    *recursive,
+                ))
             })
             .collect::<Result<_>>()?;
 
@@ -150,8 +172,12 @@ impl<'a> ChrootCmd<'a> {
                 // properties.
                 mount_bind_recursive(c"/run", run_target.as_c_str())?;
 
-                for (src, tgt) in &user_binds {
-                    mount_bind_recursive(src.as_c_str(), tgt.as_c_str())?;
+                for (src, tgt, recursive) in &user_binds {
+                    if *recursive {
+                        mount_bind_recursive(src.as_c_str(), tgt.as_c_str())?;
+                    } else {
+                        mount_bind(src.as_c_str(), tgt.as_c_str())?;
+                    }
                 }
 
                 chroot(chroot_cstr.as_c_str())?;
@@ -198,12 +224,15 @@ mod tests {
     fn builder_accumulates_binds_and_env() {
         let (_keep, root) = tmp_root();
         let src = root.join("src");
+        let flat_src = root.join("flat-src");
         let cmd = ChrootCmd::new(&root)
             .bind(&src, &"/boot")
+            .bind_flat(&flat_src, &"/sysroot")
             .setenv("FOO", "bar")
             .set_default_path();
-        assert_eq!(cmd.bind_mounts.len(), 1);
-        assert_eq!(cmd.bind_mounts[0].1, "/boot");
+        assert_eq!(cmd.bind_mounts.len(), 2);
+        assert_eq!(cmd.bind_mounts[0], (src.as_str(), "/boot", true));
+        assert_eq!(cmd.bind_mounts[1], (flat_src.as_str(), "/sysroot", false));
         // setenv + set_default_path
         assert_eq!(cmd.env_vars.len(), 2);
         assert!(cmd.env_vars.iter().any(|(k, _)| *k == "PATH"));