From e88bfb1f2fa6993727ded718f2f3edf81e4eab6d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 8 Jul 2026 10:35:24 -0400 Subject: [PATCH 1/2] bootloader: Generalize chroot parameter in install_via_bootupd This is less ostree specific; prep work for issue #2270, which wants the composefs backend to use the same chroot mechanism. Signed-off-by: Colin Walters --- crates/lib/src/bootloader.rs | 25 ++++++++++++------------- crates/lib/src/install.rs | 12 +++++++----- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/crates/lib/src/bootloader.rs b/crates/lib/src/bootloader.rs index 4109e4aff..32bb9d773 100644 --- a/crates/lib/src/bootloader.rs +++ b/crates/lib/src/bootloader.rs @@ -98,13 +98,12 @@ pub(crate) fn supports_bootupd(root: &Dir) -> Result { /// Check whether the target bootupd supports `--filesystem`. /// /// Runs `bootupctl backend install --help` and looks for `--filesystem` in the -/// output. When `deployment_path` is set the command runs inside a chroot +/// output. When `chroot_target` is set the command runs inside a chroot /// (via [`ChrootCmd`]) so we probe the binary from the target image. -fn bootupd_supports_filesystem(rootfs: &Utf8Path, deployment_path: Option<&str>) -> Result { +fn bootupd_supports_filesystem(chroot_target: Option<&Utf8Path>) -> Result { let help_args = ["bootupctl", "backend", "install", "--help"]; - let output = if let Some(deploy) = deployment_path { - let target_root = rootfs.join(deploy); - ChrootCmd::new(&target_root) + let output = if let Some(target_root) = chroot_target { + ChrootCmd::new(target_root) .set_default_path() .run_get_string(help_args)? } else { @@ -129,8 +128,9 @@ fn bootupd_supports_filesystem(rootfs: &Utf8Path, deployment_path: Option<&str>) /// /// When the target bootupd supports `--filesystem` we pass it pointing at a /// block-backed mount so that bootupd can resolve the backing device(s) itself -/// via `lsblk`. In the chroot path we bind-mount the physical root at -/// `/sysroot` to give `lsblk` a real block-backed path. +/// via `lsblk`. When `chroot_target` is set, bootupctl is executed inside the +/// given chroot root via [`ChrootCmd`], with the physical root bind-mounted at +/// `/sysroot` so `lsblk` can resolve a real block-backed path. /// /// For older bootupd versions that lack `--filesystem` we fall back to the /// legacy `--device ` invocation. @@ -139,7 +139,7 @@ pub(crate) fn install_via_bootupd( device: &bootc_blockdev::Device, rootfs: &Utf8Path, configopts: &crate::install::InstallConfigOpts, - deployment_path: Option<&str>, + chroot_target: Option<&Utf8Path>, ) -> Result<()> { let verbose = std::env::var_os("BOOTC_BOOTLOADER_DEBUG").map(|_| "-vvvv"); // bootc defaults to only targeting the platform boot method. @@ -150,7 +150,7 @@ pub(crate) fn install_via_bootupd( // This makes sure we use binaries from the target image rather than the buildroot. // In that case, the target rootfs is replaced with `/` because this is just used by // bootupd to find the backing device. - let rootfs_mount = if deployment_path.is_none() { + let rootfs_mount = if chroot_target.is_none() { rootfs.as_str() } else { "/" @@ -179,7 +179,7 @@ pub(crate) fn install_via_bootupd( // parent via require_single_root(). (Older bootupd doesn't support // multiple backing devices anyway.) // Computed before building bootupd_args so the String lives long enough. - let root_device_path = if bootupd_supports_filesystem(rootfs, deployment_path) + let root_device_path = if bootupd_supports_filesystem(chroot_target) .context("Probing bootupd --filesystem support")? { None @@ -200,8 +200,7 @@ pub(crate) fn install_via_bootupd( // namespace and the necessary API filesystems in the target // deployment, without requiring a user namespace (which fails under // qemu-user — see ). - if let Some(deploy) = deployment_path { - let target_root = rootfs.join(deploy); + if let Some(target_root) = chroot_target { let boot_path = rootfs.join("boot"); let rootfs_path = rootfs.to_path_buf(); @@ -212,7 +211,7 @@ pub(crate) fn install_via_bootupd( let mut chroot_args = vec!["bootupctl"]; chroot_args.extend(bootupd_args); - let mut cmd = ChrootCmd::new(&target_root) + 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"); diff --git a/crates/lib/src/install.rs b/crates/lib/src/install.rs index 78974fef5..7b74f4c05 100644 --- a/crates/lib/src/install.rs +++ b/crates/lib/src/install.rs @@ -1868,14 +1868,16 @@ async fn install_with_sysroot( } else { match postfetch.detected_bootloader { Bootloader::Grub => { + let root_path = rootfs + .target_root_path + .clone() + .unwrap_or(rootfs.physical_root_path.clone()); + let chroot_target = root_path.join(deployment_path.as_str()); crate::bootloader::install_via_bootupd( &rootfs.device_info, - &rootfs - .target_root_path - .clone() - .unwrap_or(rootfs.physical_root_path.clone()), + &root_path, &state.config_opts, - Some(&deployment_path.as_str()), + Some(chroot_target.as_path()), )?; } Bootloader::Systemd | Bootloader::GrubCC => { From 09153fb422721f4b4f67ca5ac62907d83f34ce3c Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 8 Jul 2026 11:02:46 -0400 Subject: [PATCH 2/2] 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 --- crates/lib/src/bootc_composefs/boot.rs | 130 ++++++++++++++++--------- crates/lib/src/bootloader.rs | 40 ++++++-- crates/lib/src/install.rs | 2 + crates/utils/src/chroot.rs | 55 ++++++++--- 4 files changed, 164 insertions(+), 63 deletions(-) diff --git a/crates/lib/src/bootc_composefs/boot.rs b/crates/lib/src/bootc_composefs/boot.rs index dd4079c7a..007a15dfc 100644 --- a/crates/lib/src/bootc_composefs/boot.rs +++ b/crates/lib/src/bootc_composefs/boot.rs @@ -1196,11 +1196,21 @@ pub(crate) fn setup_composefs_uki_boot( /// `/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 `/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 { self.composefs .fd .open_dir(self.esp_subdir) .with_context(|| format!("Opening ESP at /{}", self.esp_subdir)) } + + /// Mount the ESP at `/` 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(&self, f: impl FnOnce(&Dir) -> Result) -> Result { + 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// - 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// + 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 { diff --git a/crates/lib/src/bootloader.rs b/crates/lib/src/bootloader.rs index 32bb9d773..6ae7b7faa 100644 --- a/crates/lib/src/bootloader.rs +++ b/crates/lib/src/bootloader.rs @@ -134,12 +134,23 @@ fn bootupd_supports_filesystem(chroot_target: Option<&Utf8Path>) -> Result /// /// For older bootupd versions that lack `--filesystem` we fall back to the /// legacy `--device ` 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 `/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 ). 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. diff --git a/crates/lib/src/install.rs b/crates/lib/src/install.rs index 7b74f4c05..3c5182fd7 100644 --- a/crates/lib/src/install.rs +++ b/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 => { diff --git a/crates/utils/src/chroot.rs b/crates/utils/src/chroot.rs index de497eaa9..98ce20da2 100644 --- a/crates/utils/src/chroot.rs +++ b/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, target: &'a impl AsRef, ) -> 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, + target: &'a impl AsRef, + ) -> 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::>()?; @@ -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"));