diff --git a/crates/lib/src/bootc_composefs/boot.rs b/crates/lib/src/bootc_composefs/boot.rs index ecd333735..29abe36ad 100644 --- a/crates/lib/src/bootc_composefs/boot.rs +++ b/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; @@ -1201,11 +1202,28 @@ 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. + 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, + // Unmounted before `composefs` on drop. _tmp: bootc_mount::tempmount::MountGuard, composefs: TempMount, pub(crate) esp_subdir: &'static str, @@ -1241,10 +1259,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"); @@ -1258,7 +1272,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, @@ -1276,12 +1291,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 { + 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 `/` 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")?; + + 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); + 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 { @@ -1386,56 +1436,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 4109e4aff..0a2836f88 100644 --- a/crates/lib/src/bootloader.rs +++ b/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; @@ -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,17 +128,29 @@ 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. +/// +/// 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, - deployment_path: Option<&str>, + 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. @@ -150,7 +161,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 +190,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 @@ -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); } @@ -200,9 +220,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); - let boot_path = rootfs.join("boot"); + if let Some(target_root) = chroot_target { let rootfs_path = rootfs.to_path_buf(); tracing::debug!("Running bootupctl via chroot in {}", target_root); @@ -212,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 diff --git a/crates/lib/src/install.rs b/crates/lib/src/install.rs index 78974fef5..3c5182fd7 100644 --- a/crates/lib/src/install.rs +++ b/crates/lib/src/install.rs @@ -1868,14 +1868,18 @@ 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()); + let bind_boot_path = root_path.join("boot"); 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()), + 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..6609de954 100644 --- a/crates/utils/src/chroot.rs +++ b/crates/utils/src/chroot.rs @@ -9,20 +9,36 @@ 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}; use crate::CommandRunExt; +/// Whether a [`ChrootCmd::bind`] mount also carries over mounts nested +/// under its source. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BindMode { + /// A plain bind mount: only `source` itself is bound. Mounts nested + /// under `source` on the host (e.g. an ESP mounted under a `/boot` + /// source) are *not* carried over, leaving the corresponding + /// mountpoint directories under `target` empty. + Default, + /// A recursive bind mount: mounts nested under `source` on the host + /// are also visible under `target` inside the chroot. + Recursive, +} + /// Builder for running commands inside a target directory using a /// mount namespace + chroot. #[derive(Debug)] 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, mode). + bind_mounts: Vec<(&'a str, &'a str, BindMode)>, /// Environment variables to set on the spawned command. env_vars: Vec<(&'a str, &'a str)>, } @@ -38,14 +54,16 @@ impl<'a> ChrootCmd<'a> { } /// Add a bind mount from `source` (on the host) to `target` (a path - /// inside the chroot, e.g. `/boot`). + /// inside the chroot, e.g. `/sysroot`). See [`BindMode`] for the + /// difference between recursive and non-recursive bind mounts. pub fn bind( mut self, source: &'a impl AsRef, target: &'a impl AsRef, + mode: BindMode, ) -> 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(), mode)); 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, BindMode)> = self .bind_mounts .iter() - .map(|(src, tgt)| -> Result<_> { + .map(|(src, tgt, mode)| -> 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())?, + *mode, + )) }) .collect::>()?; @@ -150,8 +172,13 @@ 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, mode) in &user_binds { + match mode { + BindMode::Recursive => { + mount_bind_recursive(src.as_c_str(), tgt.as_c_str())? + } + BindMode::Default => mount_bind(src.as_c_str(), tgt.as_c_str())?, + } } chroot(chroot_cstr.as_c_str())?; @@ -198,12 +225,21 @@ mod tests { fn builder_accumulates_binds_and_env() { let (_keep, root) = tmp_root(); let src = root.join("src"); + let non_recursive_src = root.join("non-recursive-src"); let cmd = ChrootCmd::new(&root) - .bind(&src, &"/boot") + .bind(&src, &"/boot", BindMode::Recursive) + .bind(&non_recursive_src, &"/sysroot", BindMode::Default) .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", BindMode::Recursive) + ); + assert_eq!( + cmd.bind_mounts[1], + (non_recursive_src.as_str(), "/sysroot", BindMode::Default) + ); // setenv + set_default_path assert_eq!(cmd.env_vars.len(), 2); assert!(cmd.env_vars.iter().any(|(k, _)| *k == "PATH")); @@ -229,7 +265,7 @@ mod tests { let (_keep, root) = tmp_root(); let (_keep2, src_root) = tmp_root(); ChrootCmd::new(&root) - .bind(&src_root, &"/sysroot") + .bind(&src_root, &"/sysroot", BindMode::Recursive) .build_command(["/bin/true"]) .unwrap(); assert!(root.join("sysroot").is_dir());