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

@@ -61,6 +61,7 @@
 //! 1. **Primary**: New/upgraded deployment (default boot target)
 //! 2. **Secondary**: Currently booted deployment (rollback option)
 
+use std::cell::Cell;
 use std::fs::create_dir_all;
 use std::io::{Read, Seek, SeekFrom, Write};
 use std::path::Path;
@@ -1207,11 +1208,28 @@ 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.
+    esp_device: Utf8PathBuf,
+    // Tracks whether the ESP is currently mounted, i.e. whether we're
+    // inside a call to `with_esp()`. Guards `open_esp_dir()` against
+    // returning the wrong thing: without this, calling it while the ESP
+    // isn't mounted would silently open the empty `esp_subdir` directory
+    // that already exists directly in the composefs image, rather than
+    // failing.
+    esp_mounted: Cell<bool>,
+    // Unmounted before `composefs` on drop.
     _tmp: bootc_mount::tempmount::MountGuard,
     composefs: TempMount,
     pub(crate) esp_subdir: &'static str,
@@ -1247,10 +1265,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");
@@ -1264,7 +1278,8 @@ impl MountedImageRoot {
         .context("Mounting tmpfs into composefs root")?;
 
         Ok(Self {
-            _esp: esp,
+            esp_device: esp_part.path().into(),
+            esp_mounted: Cell::new(false),
             _tmp: tmp,
             composefs,
             esp_subdir,
@@ -1282,12 +1297,47 @@ impl MountedImageRoot {
     }
 
     /// Open the mounted ESP as a capability-safe directory.
+    ///
+    /// Fails unless called from within a call to [`Self::with_esp`]: the ESP
+    /// is only actually mounted for the duration of that call, and
+    /// `esp_subdir` otherwise refers to an ordinary (empty) directory that
+    /// already exists directly in the composefs image.
     pub(crate) fn open_esp_dir(&self) -> Result<Dir> {
+        if !self.esp_mounted.get() {
+            bail!("BUG: attempted to open the ESP directory while it is not mounted");
+        }
         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")?;
+
+        self.esp_mounted.set(true);
+        // Reset `esp_mounted` when we return, including via `?` or panic,
+        // so a later `with_esp()` call (or a stray `open_esp_dir()` call
+        // after this one returns) doesn't see a stale "mounted" state.
+        struct ResetOnDrop<'a>(&'a Cell<bool>);
+        impl Drop for ResetOnDrop<'_> {
+            fn drop(&mut self) {
+                self.0.set(false);
+            }
+        }
+        let _reset = ResetOnDrop(&self.esp_mounted);
+
+        let dir = self.open_esp_dir()?;
+        f(&dir)
+    }
 }
 
 pub struct SecurebootKeys {
@@ -1392,56 +1442,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

@@ -3,7 +3,7 @@ use std::process::Command;
 use std::sync::OnceLock;
 
 use anyhow::{Context, Result, anyhow, bail};
-use bootc_utils::{ChrootCmd, CommandRunExt};
+use bootc_utils::{BindMode, ChrootCmd, CommandRunExt};
 use camino::Utf8Path;
 use cap_std_ext::cap_std::fs::Dir;
 use cap_std_ext::dirext::CapStdExtDirExt;
@@ -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,15 +230,24 @@ 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(boot_path, &"/boot", BindMode::Default);
+        }
 
         // Only bind mount the physical root at /sysroot when using --filesystem;
         // bootupd needs it to resolve backing block devices via lsblk.
         if root_device_path.is_none() {
-            cmd = cmd.bind(&rootfs_path, &"/sysroot");
+            cmd = cmd.bind(&rootfs_path, &"/sysroot", BindMode::Recursive);
         }
 
         // ChrootCmd starts the child with a cleared environment, so we

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 => {