From 68585a85ec1fd5bb2406d5374ea858c524692d6b Mon Sep 17 00:00:00 2001 From: Yasmin de Souza Date: Wed, 1 Apr 2026 11:26:48 -0300 Subject: [PATCH 1/7] Add copy-to-boot feature and add copy-to-boot cli command This is part of Fedora BootLoaderUpdatesPhase1: https://fedoraproject.org/wiki/Changes/BootLoaderUpdatesPhase1 --- src/bios.rs | 5 +++++ src/bootupd.rs | 16 ++++++++++++++++ src/cli/bootupctl.rs | 5 +++++ src/cli/bootupd.rs | 11 +++++++++++ src/component.rs | 3 +++ 5 files changed, 40 insertions(+) diff --git a/src/bios.rs b/src/bios.rs index f341f994..1012dd5e 100644 --- a/src/bios.rs +++ b/src/bios.rs @@ -277,4 +277,9 @@ impl Component for Bios { fn get_efi_vendor(&self, _: &Path) -> Result> { Ok(None) } + + /// Package mode copy is EFI-only + fn package_mode_copy_to_boot(&self, _root: &Path) -> Result<()> { + Ok(()) + } } diff --git a/src/bootupd.rs b/src/bootupd.rs index e01ce923..e6094659 100644 --- a/src/bootupd.rs +++ b/src/bootupd.rs @@ -802,6 +802,22 @@ pub(crate) fn client_run_migrate_static_grub_config() -> Result<()> { Ok(()) } +/// Copy bootloader files from /usr/lib/efi to boot/ESP for package mode installations. +pub(crate) fn copy_to_boot(root: &Path) -> Result<()> { + let all_components = get_components_impl(false); + if all_components.is_empty() { + anyhow::bail!("No components available for this platform."); + } + + for component in all_components.values() { + component + .package_mode_copy_to_boot(root) + .with_context(|| format!("Failed to copy component {} to boot", component.name()))?; + } + + Ok(()) +} + /// Writes a stripped GRUB config to `stripped_config_name`, removing lines between /// `### BEGIN /etc/grub.d/15_ostree ###` and `### END /etc/grub.d/15_ostree ###`. fn strip_grub_config_file( diff --git a/src/cli/bootupctl.rs b/src/cli/bootupctl.rs index 1c30b24e..34f0572b 100644 --- a/src/cli/bootupctl.rs +++ b/src/cli/bootupctl.rs @@ -73,6 +73,8 @@ pub enum CtlBackend { Generate(super::bootupd::GenerateOpts), #[clap(name = "install", hide = true)] Install(super::bootupd::InstallOpts), + #[clap(name = "copy-to-boot", hide = true)] + CopyToBoot, } #[derive(Debug, Parser)] @@ -109,6 +111,9 @@ impl CtlCommand { CtlVerb::Backend(CtlBackend::Install(opts)) => { super::bootupd::DCommand::run_install(opts) } + CtlVerb::Backend(CtlBackend::CopyToBoot) => { + super::bootupd::DCommand::run_copy_to_boot() + } CtlVerb::MigrateStaticGrubConfig => Self::run_migrate_static_grub_config(), } } diff --git a/src/cli/bootupd.rs b/src/cli/bootupd.rs index 10e0f256..cab6b580 100644 --- a/src/cli/bootupd.rs +++ b/src/cli/bootupd.rs @@ -38,6 +38,11 @@ pub enum DVerb { GenerateUpdateMetadata(GenerateOpts), #[clap(name = "install", about = "Install components")] Install(InstallOpts), + #[clap( + name = "copy-to-boot", + about = "Copy bootloader files from /usr/lib/efi to ESP (package mode), EFI only" + )] + CopyToBoot, } #[derive(Debug, Parser)] @@ -97,6 +102,7 @@ impl DCommand { match self.cmd { DVerb::Install(opts) => Self::run_install(opts), DVerb::GenerateUpdateMetadata(opts) => Self::run_generate_meta(opts), + DVerb::CopyToBoot => Self::run_copy_to_boot(), } } @@ -146,4 +152,9 @@ impl DCommand { .context("boot data installation failed")?; Ok(()) } + + pub(crate) fn run_copy_to_boot() -> Result<()> { + bootupd::copy_to_boot(std::path::Path::new("/")).context("copying to boot failed")?; + Ok(()) + } } diff --git a/src/component.rs b/src/component.rs index dabcea97..80a241a7 100644 --- a/src/component.rs +++ b/src/component.rs @@ -85,6 +85,9 @@ pub(crate) trait Component { /// Locating efi vendor dir fn get_efi_vendor(&self, sysroot: &Path) -> Result>; + + /// Copy from /usr/lib/efi to boot/ESP (package mode) + fn package_mode_copy_to_boot(&self, root: &Path) -> Result<()>; } /// Given a component name, create an implementation. From 547002d3b559e24f5db70e57877df5f50a2afa3b Mon Sep 17 00:00:00 2001 From: Yasmin de Souza Date: Wed, 1 Apr 2026 11:30:51 -0300 Subject: [PATCH 2/7] Check ESP space to avoid errors --- src/filetree.rs | 10 ++++++++++ src/util.rs | 13 ++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/filetree.rs b/src/filetree.rs index a9eee816..b6eb8bc1 100644 --- a/src/filetree.rs +++ b/src/filetree.rs @@ -207,6 +207,16 @@ impl FileTree { Ok(Self { children }) } + /// Total size in bytes of all files in the tree (for space checks). + #[cfg(any( + target_arch = "x86_64", + target_arch = "aarch64", + target_arch = "riscv64" + ))] + pub(crate) fn total_size(&self) -> u64 { + self.children.values().map(|m| m.size).sum() + } + /// Determine the changes *from* self to the updated tree #[cfg(any( target_arch = "x86_64", diff --git a/src/util.rs b/src/util.rs index 2fda1778..4d36edca 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,9 +1,11 @@ use std::collections::HashSet; +use std::os::unix::io::AsRawFd; use std::path::Path; use std::process::Command; use anyhow::{bail, Context, Result}; use openat_ext::OpenatDirExt; +use rustix::fd::BorrowedFd; /// Parse an environment variable as UTF-8 #[allow(dead_code)] @@ -51,9 +53,18 @@ pub(crate) fn filenames(dir: &openat::Dir) -> Result> { Ok(ret) } +/// Return the available space in bytes on the filesystem containing the given directory. +/// Uses f_bavail * f_frsize from fstatvfs to avoid partial updates when the partition is full. +pub(crate) fn available_space_bytes(dir: &openat::Dir) -> Result { + let fd = unsafe { BorrowedFd::borrow_raw(dir.as_raw_fd()) }; + let st = rustix::fs::fstatvfs(fd)?; + Ok((st.f_bavail as u64) * (st.f_frsize as u64)) +} + pub(crate) fn ensure_writable_mount>(p: P) -> Result<()> { let p = p.as_ref(); - let stat = rustix::fs::statvfs(p)?; + let stat = + rustix::fs::statvfs(p).map_err(|e| std::io::Error::from_raw_os_error(e.raw_os_error()))?; if !stat.f_flag.contains(rustix::fs::StatVfsMountFlags::RDONLY) { return Ok(()); } From e4205a5a8fde11e6c99a297a87e1c78068f4a5f1 Mon Sep 17 00:00:00 2001 From: Yasmin de Souza Date: Wed, 1 Apr 2026 11:32:29 -0300 Subject: [PATCH 3/7] Implement copy_efi_components_to_esp and add unit tests --- src/efi.rs | 380 ++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 348 insertions(+), 32 deletions(-) diff --git a/src/efi.rs b/src/efi.rs index 1022f4af..c2170987 100644 --- a/src/efi.rs +++ b/src/efi.rs @@ -19,6 +19,10 @@ use chrono::prelude::*; use fn_error_context::context; use openat_ext::OpenatDirExt; use os_release::OsRelease; +use rustix::mount::{ + fsconfig_create, fsconfig_set_path, fsmount, fsopen, move_mount, FsMountFlags, FsOpenFlags, + MountAttrFlags, MoveMountFlags, UnmountFlags, +}; use rustix::{fd::AsFd, fd::BorrowedFd, fs::StatVfsMountFlags}; use walkdir::WalkDir; use widestring::U16CString; @@ -87,9 +91,40 @@ pub(crate) fn is_efi_booted() -> Result { .map_err(Into::into) } +fn mount_esp(esp_device: &Path, target: &Path) -> Result<()> { + use rustix::fs::CWD; + + let fs_fd = fsopen("vfat", FsOpenFlags::empty()).context("fsopen vfat")?; + fsconfig_set_path(fs_fd.as_fd(), "source", esp_device, CWD) + .context("fsconfig_set_path source")?; + fsconfig_create(fs_fd.as_fd()).context("fsconfig_create")?; + let mount_fd = fsmount( + fs_fd.as_fd(), + FsMountFlags::empty(), + MountAttrFlags::empty(), + ) + .context("fsmount")?; + let target_dir = std::fs::File::open(target).context("open target dir for move_mount")?; + let target_fd = unsafe { BorrowedFd::borrow_raw(target_dir.as_raw_fd()) }; + move_mount( + mount_fd.as_fd(), + "", + target_fd, + ".", + MoveMountFlags::MOVE_MOUNT_F_EMPTY_PATH, + ) + .context("move_mount")?; + Ok(()) +} + +struct Mount { + path: PathBuf, + owned: bool, +} + #[derive(Default)] pub(crate) struct Efi { - mountpoint: RefCell>, + mountpoint: RefCell>, } impl Efi { @@ -120,20 +155,21 @@ impl Efi { break; } } - - // Only borrow mutably if we found a mount point if let Some(mnt) = found_mount { log::debug!("Reusing existing mount point {mnt:?}"); - *self.mountpoint.borrow_mut() = Some(mnt.clone()); + *self.mountpoint.borrow_mut() = Some(Mount { + path: mnt.clone(), + owned: false, + }); Ok(Some(mnt)) } else { Ok(None) } } - // Mount the passed esp_device, return mount point pub(crate) fn mount_esp_device(&self, root: &Path, esp_device: &Path) -> Result { - let mut mountpoint = None; + // (mount path, whether bootupd performed the mount and must unmount) + let mut mountpoint: Option<(PathBuf, bool)> = None; for &mnt in ESP_MOUNTS.iter() { let mnt = root.join(mnt); @@ -147,31 +183,40 @@ impl Efi { if st.f_type == libc::MSDOS_SUPER_MAGIC { if is_mount_point(&mnt)? { log::debug!("ESP already mounted at {mnt:?}, reusing"); - mountpoint = Some(mnt); + mountpoint = Some((mnt, false)); break; } } + if mount_esp(esp_device, &mnt).is_ok() { + log::debug!("Mounted at {mnt:?}"); + mountpoint = Some((mnt, true)); + break; + } + log::trace!("Mount failed, falling back to mount(8)"); std::process::Command::new("mount") - .arg(&esp_device) + .arg(esp_device) .arg(&mnt) .run_inherited() .with_context(|| format!("Failed to mount {:?}", esp_device))?; log::debug!("Mounted at {mnt:?}"); - mountpoint = Some(mnt); + mountpoint = Some((mnt, true)); break; } - let mnt = mountpoint.ok_or_else(|| anyhow::anyhow!("No mount point found"))?; - *self.mountpoint.borrow_mut() = Some(mnt.clone()); + let (mnt, owned) = mountpoint.ok_or_else(|| anyhow::anyhow!("No mount point found"))?; + *self.mountpoint.borrow_mut() = Some(Mount { + path: mnt.clone(), + owned, + }); Ok(mnt) } // Firstly check if esp is already mounted, then mount the passed esp device pub(crate) fn ensure_mounted_esp(&self, root: &Path, esp_device: &Path) -> Result { - if let Some(mountpoint) = self.mountpoint.borrow().as_deref() { - return Ok(mountpoint.to_owned()); + if let Some(mount) = self.mountpoint.borrow().as_ref() { + return Ok(mount.path.clone()); } - let destdir = if let Some(destdir) = self.get_mounted_esp(Path::new(root))? { + let destdir = if let Some(destdir) = self.get_mounted_esp(root)? { destdir } else { self.mount_esp_device(root, esp_device)? @@ -180,12 +225,25 @@ impl Efi { } fn unmount(&self) -> Result<()> { - if let Some(mount) = self.mountpoint.borrow_mut().take() { - Command::new("umount") - .arg(&mount) - .run_inherited() - .with_context(|| format!("Failed to unmount {mount:?}"))?; - log::trace!("Unmounted"); + // Only unmount if we mounted it ourselves + let should_unmount = self + .mountpoint + .borrow() + .as_ref() + .map(|m| m.owned) + .unwrap_or(false); + if should_unmount { + if let Some(mount) = self.mountpoint.borrow_mut().take() { + if rustix::mount::unmount(&mount.path, UnmountFlags::empty()).is_ok() { + log::trace!("Unmounted (new mount API)"); + } else { + Command::new("umount") + .arg(&mount.path) + .run_inherited() + .with_context(|| format!("Failed to unmount {:?}", mount.path))?; + log::trace!("Unmounted"); + } + } } Ok(()) } @@ -236,6 +294,119 @@ impl Efi { let device_path = device.path(); create_efi_boot_entry(&device_path, esp_part_num.trim(), &loader, &product_name) } + /// Copy EFI components to ESP using the same "write alongside + atomic rename" pattern + /// as bootable container updates, so the system stays bootable if any step fails. + fn copy_efi_components_to_esp( + &self, + sysroot_dir: &openat::Dir, + esp_dir: &openat::Dir, + _esp_path: &Path, + efi_components: &[EFIComponent], + ) -> Result<()> { + // Build a merged source tree in a temp dir (same layout as desired ESP/EFI) + let temp_dir = tempfile::tempdir().context("Creating temp dir for EFI merge")?; + let temp_efi_path = temp_dir.path().join("EFI"); + std::fs::create_dir_all(&temp_efi_path) + .with_context(|| format!("Creating {}", temp_efi_path.display()))?; + let temp_efi_str = temp_efi_path + .to_str() + .context("Temp EFI path is not valid UTF-8")?; + + for efi_comp in efi_components { + log::info!( + "Merging EFI component {} version {} into update tree", + efi_comp.name, + efi_comp.version + ); + // Copy contents of component's EFI dir (e.g. fedora/) into temp_efi_path so merged + // layout is EFI/fedora/..., not EFI/EFI/fedora/... + let src_efi_contents = format!("{}/.", efi_comp.path); + filetree::copy_dir_with_args( + sysroot_dir, + src_efi_contents.as_str(), + temp_efi_str, + OPTIONS, + ) + .with_context(|| format!("Copying {} to merge dir", efi_comp.path))?; + } + + // Ensure ESP/EFI exists (e.g. first install) + esp_dir.ensure_dir_all(std::path::Path::new("EFI"), 0o755)?; + let esp_efi_dir = esp_dir.sub_dir("EFI").context("Opening ESP EFI dir")?; + + let source_dir = + openat::Dir::open(&temp_efi_path).context("Opening merged EFI source dir")?; + let source_filetree = + filetree::FileTree::new_from_dir(&source_dir).context("Building source filetree")?; + let current_filetree = + filetree::FileTree::new_from_dir(&esp_efi_dir).context("Building current filetree")?; + let mut diff = current_filetree + .diff(&source_filetree) + .context("Computing EFI diff")?; + diff.removals.clear(); + + // Check available space before writing to prevent partial updates when the partition is full + let required_bytes = current_filetree.total_size() + source_filetree.total_size(); + let available_bytes = util::available_space_bytes(&esp_efi_dir)?; + if available_bytes < required_bytes { + anyhow::bail!( + "ESP has insufficient free space for update: need {} MiB, have {} MiB", + required_bytes / (1024 * 1024), + available_bytes / (1024 * 1024) + ); + } + + // Same logic as bootable container: write to .btmp.* then atomic rename + filetree::apply_diff(&source_dir, &esp_efi_dir, &diff, None) + .context("Applying EFI update (write alongside + atomic rename)")?; + + // Sync the whole ESP filesystem + fsfreeze_thaw_cycle(esp_dir.open_file(".")?)?; + + Ok(()) + } + + /// Copy from /usr/lib/efi to boot/ESP. Caller provides sysroot (e.g. for recovery or tests). + fn package_mode_copy_to_boot_impl(&self, sysroot: &Path) -> Result<()> { + let sysroot_path = Utf8Path::from_path(sysroot) + .with_context(|| format!("Invalid UTF-8: {}", sysroot.display()))?; + let sysroot_dir = openat::Dir::open(sysroot).context("Opening sysroot for reading")?; + + let efi_comps = match get_efi_component_from_usr(sysroot_path, EFILIB)? { + Some(comps) if !comps.is_empty() => comps, + _ => anyhow::bail!("No EFI components found in /usr/lib/efi"), + }; + + // First try to use an already mounted ESP + let esp_path = if let Some(mounted_esp) = self.get_mounted_esp(sysroot)? { + mounted_esp + } else { + let sysroot_cap = Dir::open_ambient_dir(sysroot, cap_std::ambient_authority()) + .with_context(|| format!("Opening sysroot {}", sysroot.display()))?; + let device = bootc_internal_blockdev::list_dev_by_dir(&sysroot_cap) + .with_context(|| format!("Resolving block device for {}", sysroot.display()))?; + let Some(esp_devices) = device.find_colocated_esps()? else { + anyhow::bail!("No ESP found"); + }; + let esp = esp_devices + .first() + .ok_or_else(|| anyhow::anyhow!("No ESP partition found"))?; + self.ensure_mounted_esp(sysroot, Path::new(&esp.path()))? + }; + + let esp_dir = openat::Dir::open(&esp_path) + .with_context(|| format!("Opening ESP at {}", esp_path.display()))?; + validate_esp_fstype(&esp_dir)?; + + self.copy_efi_components_to_esp(&sysroot_dir, &esp_dir, &esp_path, &efi_comps)?; + + log::info!( + "Successfully copied {} EFI component(s) to ESP at {}", + efi_comps.len(), + esp_path.display() + ); + Ok(()) + } } #[context("Get product name")] @@ -473,23 +644,19 @@ impl Component for Efi { } else { None }; - let dest = destpath.to_str().with_context(|| { - format!( - "Include invalid UTF-8 characters in dest {}", - destpath.display() - ) - })?; let efi_path = if let Some(efi_components) = efi_comps { - for efi in efi_components { - filetree::copy_dir_with_args(&src_dir, efi.path.as_str(), dest, OPTIONS)?; - } + // Use shared helper to copy components from /usr/lib/efi + self.copy_efi_components_to_esp(&src_dir, destd, &destpath, &efi_components)?; EFILIB } else { let updates = component_updatedirname(self); let src = updates .to_str() - .context("Include invalid UTF-8 characters in path")?; + .with_context(|| format!("Invalid UTF-8: {}", updates.display()))?; + let dest = destpath + .to_str() + .with_context(|| format!("Invalid UTF-8: {}", destpath.display()))?; filetree::copy_dir_with_args(&src_dir, src, dest, OPTIONS)?; &src.to_owned() }; @@ -686,6 +853,11 @@ impl Component for Efi { anyhow::bail!("Failed to find {SHIM} in the image") } } + + /// Package mode copy: Simple copy from /usr/lib/efi to boot/ESP. + fn package_mode_copy_to_boot(&self, root: &Path) -> Result<()> { + self.package_mode_copy_to_boot_impl(root) + } } impl Drop for Efi { @@ -981,7 +1153,6 @@ Boot0003* test"; ); Ok(()) } - #[cfg(test)] fn fixture() -> Result { let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; tempdir.create_dir("etc")?; @@ -1011,7 +1182,7 @@ Boot0003* test"; { tmpd.atomic_write( "etc/system-release", - "Red Hat Enterprise Linux CoreOS release 4 + r"Red Hat Enterprise Linux CoreOS release 4 ", )?; let name = get_product_name(&tmpd)?; @@ -1057,4 +1228,149 @@ Boot0003* test"; assert_eq!(efi_comps, None); Ok(()) } + + #[test] + fn test_package_mode_copy_to_boot_discovery() -> Result<()> { + // Test that we can discover components from /usr/lib/efi + let tmpdir: &tempfile::TempDir = &tempfile::tempdir()?; + let tpath = tmpdir.path(); + let efi_path = tpath.join("usr/lib/efi"); + + // Create mock EFI components + std::fs::create_dir_all(efi_path.join("shim/15.8-3/EFI/fedora"))?; + std::fs::create_dir_all(efi_path.join("grub2/2.12-28/EFI/fedora"))?; + + // Write some test files + std::fs::write( + efi_path.join("shim/15.8-3/EFI/fedora/shimx64.efi"), + b"shim content", + )?; + std::fs::write( + efi_path.join("grub2/2.12-28/EFI/fedora/grubx64.efi"), + b"grub content", + )?; + + let utf8_tpath = + Utf8Path::from_path(tpath).ok_or_else(|| anyhow::anyhow!("Path is not valid UTF-8"))?; + + // Test component discovery + let efi_comps = match get_efi_component_from_usr(utf8_tpath, EFILIB)? { + Some(comps) if !comps.is_empty() => comps, + _ => { + anyhow::bail!("Should have found components"); + } + }; + + // Verify we found the expected components + assert_eq!(efi_comps.len(), 2); + let names: Vec<_> = efi_comps.iter().map(|c| c.name.as_str()).collect(); + assert!(names.contains(&"shim")); + assert!(names.contains(&"grub2")); + + // Verify paths are correct + for comp in &efi_comps { + assert!(comp.path.starts_with("usr/lib/efi")); + assert!(comp.path.ends_with("EFI")); + } + + Ok(()) + } + + #[test] + fn test_package_mode_shim_installation() -> Result<()> { + // Test that shim can be installed from /usr/lib/efi to ESP + let tmpdir: &tempfile::TempDir = &tempfile::tempdir()?; + let tpath = tmpdir.path(); + + // Create mock /usr/lib/efi structure with shim + let efi_path = tpath.join("usr/lib/efi"); + let shim_path = efi_path.join("shim/15.8-3/EFI/fedora"); + std::fs::create_dir_all(&shim_path)?; + + // Write shim binary + let shim_content = b"mock shim binary content"; + std::fs::write(shim_path.join(SHIM), shim_content)?; + + // Create additional shim files that might be present + std::fs::write(shim_path.join("MokManager.efi"), b"mok manager content")?; + std::fs::write(shim_path.join("fbx64.efi"), b"fallback content")?; + + // Create mock ESP directory structure (simulating /boot/efi in container) + let esp_path = tpath.join("boot/efi"); + std::fs::create_dir_all(&esp_path)?; + + // Create EFI directory in ESP + let esp_efi_path = esp_path.join("EFI"); + std::fs::create_dir_all(&esp_efi_path)?; + + // Set up sysroot directory + let sysroot_dir = openat::Dir::open(tpath)?; + + // Get EFI components from usr/lib/efi + let utf8_tpath = + Utf8Path::from_path(tpath).ok_or_else(|| anyhow::anyhow!("Path is not valid UTF-8"))?; + let efi_comps = get_efi_component_from_usr(utf8_tpath, EFILIB)?; + assert!(efi_comps.is_some(), "Should find shim component"); + let efi_comps = efi_comps.unwrap(); + assert_eq!(efi_comps.len(), 1, "Should find exactly one component"); + assert_eq!(efi_comps[0].name, "shim"); + assert_eq!(efi_comps[0].version, "15.8-3"); + + // Create Efi instance and copy components to ESP + let esp_dir = openat::Dir::open(&esp_path).context("Opening ESP dir for test")?; + let efi = Efi::default(); + efi.copy_efi_components_to_esp(&sysroot_dir, &esp_dir, &esp_path, &efi_comps)?; + + // Expected path: /boot/efi/EFI/fedora/shimx64.efi (or shimaa64.efi, etc.) + let copied_shim_path = esp_path.join("EFI/fedora").join(SHIM); + assert!( + copied_shim_path.exists(), + "Shim should be copied to ESP at {}", + copied_shim_path.display() + ); + + // Verify the shim file is actually a file, not a directory + assert!( + copied_shim_path.is_file(), + "Shim should be a file at {}", + copied_shim_path.display() + ); + + // Verify the content matches exactly + let copied_content = std::fs::read(&copied_shim_path)?; + assert_eq!( + copied_content, shim_content, + "Shim content should match exactly" + ); + + // Verify the directory structure is correct + assert!( + esp_path.join("EFI").exists(), + "EFI directory should exist in ESP at {}", + esp_path.join("EFI").display() + ); + assert!(esp_path.join("EFI").is_dir(), "EFI should be a directory"); + + assert!( + esp_path.join("EFI/fedora").exists(), + "Vendor directory (fedora) should exist in ESP at {}", + esp_path.join("EFI/fedora").display() + ); + assert!( + esp_path.join("EFI/fedora").is_dir(), + "EFI/fedora should be a directory" + ); + + // Verify the path structure matches expected package mode layout + // Source: /usr/lib/efi/shim/15.8-3/EFI/fedora/shimx64.efi + // Dest: /boot/efi/EFI/fedora/shimx64.efi + let expected_base = esp_path.join("EFI/fedora"); + assert_eq!( + copied_shim_path.parent(), + Some(expected_base.as_path()), + "Shim should be directly under EFI/fedora/, not in a subdirectory" + ); + + Ok(()) + } } From 3a1f691dcc3a054793455d8cace6429c1c9503a3 Mon Sep 17 00:00:00 2001 From: Yasmin de Souza Date: Tue, 7 Apr 2026 11:06:14 -0300 Subject: [PATCH 4/7] docs: clarify package_mode_copy_to_boot; better statvfs errors. Use anyhow context for statvfs failures instead of raw_os_error. AI-assisted: implementation reviewed by Claude. Some suggestions was added. --- src/component.rs | 2 +- src/efi.rs | 2 +- src/util.rs | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/component.rs b/src/component.rs index 80a241a7..67399c39 100644 --- a/src/component.rs +++ b/src/component.rs @@ -86,7 +86,7 @@ pub(crate) trait Component { /// Locating efi vendor dir fn get_efi_vendor(&self, sysroot: &Path) -> Result>; - /// Copy from /usr/lib/efi to boot/ESP (package mode) + /// Merge `/usr/lib/efi` onto the ESP for package-mode systems (EFI components only). fn package_mode_copy_to_boot(&self, root: &Path) -> Result<()>; } diff --git a/src/efi.rs b/src/efi.rs index c2170987..52d377e6 100644 --- a/src/efi.rs +++ b/src/efi.rs @@ -854,7 +854,7 @@ impl Component for Efi { } } - /// Package mode copy: Simple copy from /usr/lib/efi to boot/ESP. + /// Package mode: merge `/usr/lib/efi` onto the ESP (write alongside, then atomic rename). fn package_mode_copy_to_boot(&self, root: &Path) -> Result<()> { self.package_mode_copy_to_boot_impl(root) } diff --git a/src/util.rs b/src/util.rs index 4d36edca..91544365 100644 --- a/src/util.rs +++ b/src/util.rs @@ -63,8 +63,7 @@ pub(crate) fn available_space_bytes(dir: &openat::Dir) -> Result { pub(crate) fn ensure_writable_mount>(p: P) -> Result<()> { let p = p.as_ref(); - let stat = - rustix::fs::statvfs(p).map_err(|e| std::io::Error::from_raw_os_error(e.raw_os_error()))?; + let stat = rustix::fs::statvfs(p).with_context(|| format!("statvfs {:?}", p))?; if !stat.f_flag.contains(rustix::fs::StatVfsMountFlags::RDONLY) { return Ok(()); } From 42286bbae2dd992ecd67fd406b6d3dfb0ba27c42 Mon Sep 17 00:00:00 2001 From: Yasmin de Souza Date: Thu, 7 May 2026 15:42:02 -0300 Subject: [PATCH 5/7] Add reviewers suggestions --- src/util.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/util.rs b/src/util.rs index 91544365..8ec26efd 100644 --- a/src/util.rs +++ b/src/util.rs @@ -5,7 +5,19 @@ use std::process::Command; use anyhow::{bail, Context, Result}; use openat_ext::OpenatDirExt; -use rustix::fd::BorrowedFd; +use rustix::fd::{AsFd, BorrowedFd}; + +/// [`openat::Dir`] only implements [`AsRawFd`]; this bridges to [`rustix::fd::AsFd`] so we can call +/// [`rustix::fs::fstatvfs`] with `.as_fd()`, consistent with other rustix call sites in this crate +/// (for example `cap_std::fs::Dir` in `efi.rs`). +struct OpenatDirAsFd<'a>(&'a openat::Dir); + +impl AsFd for OpenatDirAsFd<'_> { + fn as_fd(&self) -> BorrowedFd<'_> { + // SAFETY: `openat::Dir` owns the fd; the borrow is tied to `&openat::Dir`. + unsafe { BorrowedFd::borrow_raw(self.0.as_raw_fd()) } + } +} /// Parse an environment variable as UTF-8 #[allow(dead_code)] @@ -56,8 +68,7 @@ pub(crate) fn filenames(dir: &openat::Dir) -> Result> { /// Return the available space in bytes on the filesystem containing the given directory. /// Uses f_bavail * f_frsize from fstatvfs to avoid partial updates when the partition is full. pub(crate) fn available_space_bytes(dir: &openat::Dir) -> Result { - let fd = unsafe { BorrowedFd::borrow_raw(dir.as_raw_fd()) }; - let st = rustix::fs::fstatvfs(fd)?; + let st = rustix::fs::fstatvfs(OpenatDirAsFd(dir).as_fd())?; Ok((st.f_bavail as u64) * (st.f_frsize as u64)) } From 824a5c61967b592525739d3b00ef865a67cab3a5 Mon Sep 17 00:00:00 2001 From: Yasmin de Souza Date: Wed, 13 May 2026 12:01:03 -0300 Subject: [PATCH 6/7] mend --- src/bootupd.rs | 4 +- src/component.rs | 3 +- src/efi.rs | 251 +++++++++++++++++++++++++++-------------------- 3 files changed, 151 insertions(+), 107 deletions(-) diff --git a/src/bootupd.rs b/src/bootupd.rs index e6094659..c83090d7 100644 --- a/src/bootupd.rs +++ b/src/bootupd.rs @@ -802,7 +802,9 @@ pub(crate) fn client_run_migrate_static_grub_config() -> Result<()> { Ok(()) } -/// Copy bootloader files from /usr/lib/efi to boot/ESP for package mode installations. +/// Copy EFI payloads from `/usr/lib/efi` onto the ESP for package-mode installs. Removals are +/// scoped to `EFI//` trees present in this merge; other top-level ESP dirs are untouched. +/// BIOS components are no-ops. pub(crate) fn copy_to_boot(root: &Path) -> Result<()> { let all_components = get_components_impl(false); if all_components.is_empty() { diff --git a/src/component.rs b/src/component.rs index 67399c39..43d19607 100644 --- a/src/component.rs +++ b/src/component.rs @@ -86,7 +86,8 @@ pub(crate) trait Component { /// Locating efi vendor dir fn get_efi_vendor(&self, sysroot: &Path) -> Result>; - /// Merge `/usr/lib/efi` onto the ESP for package-mode systems (EFI components only). + /// Merge `/usr/lib/efi` onto the ESP for package-mode systems (EFI only). Deletes only under + /// `EFI//` for vendors shipped in this merge; other ESP subtrees are left alone. fn package_mode_copy_to_boot(&self, root: &Path) -> Result<()>; } diff --git a/src/efi.rs b/src/efi.rs index 52d377e6..e1f19184 100644 --- a/src/efi.rs +++ b/src/efi.rs @@ -5,6 +5,7 @@ */ use std::cell::RefCell; +use std::collections::HashSet; use std::os::unix::io::AsRawFd; use std::path::{Path, PathBuf}; use std::process::Command; @@ -19,10 +20,7 @@ use chrono::prelude::*; use fn_error_context::context; use openat_ext::OpenatDirExt; use os_release::OsRelease; -use rustix::mount::{ - fsconfig_create, fsconfig_set_path, fsmount, fsopen, move_mount, FsMountFlags, FsOpenFlags, - MountAttrFlags, MoveMountFlags, UnmountFlags, -}; +use rustix::mount::UnmountFlags; use rustix::{fd::AsFd, fd::BorrowedFd, fs::StatVfsMountFlags}; use walkdir::WalkDir; use widestring::U16CString; @@ -91,32 +89,6 @@ pub(crate) fn is_efi_booted() -> Result { .map_err(Into::into) } -fn mount_esp(esp_device: &Path, target: &Path) -> Result<()> { - use rustix::fs::CWD; - - let fs_fd = fsopen("vfat", FsOpenFlags::empty()).context("fsopen vfat")?; - fsconfig_set_path(fs_fd.as_fd(), "source", esp_device, CWD) - .context("fsconfig_set_path source")?; - fsconfig_create(fs_fd.as_fd()).context("fsconfig_create")?; - let mount_fd = fsmount( - fs_fd.as_fd(), - FsMountFlags::empty(), - MountAttrFlags::empty(), - ) - .context("fsmount")?; - let target_dir = std::fs::File::open(target).context("open target dir for move_mount")?; - let target_fd = unsafe { BorrowedFd::borrow_raw(target_dir.as_raw_fd()) }; - move_mount( - mount_fd.as_fd(), - "", - target_fd, - ".", - MoveMountFlags::MOVE_MOUNT_F_EMPTY_PATH, - ) - .context("move_mount")?; - Ok(()) -} - struct Mount { path: PathBuf, owned: bool, @@ -167,6 +139,16 @@ impl Efi { } } + /// Attach `esp_device` at a well-known mount under `root` when the ESP is not already there. + /// + /// This uses **`mount(8)`** only—the same approach as the rest of bootupd’s ESP handling—not a + /// second parallel stack (e.g. `fsopen`/`fsmount`/`move_mount`). Review feedback was that duplicating + /// the kernel mount API without a clear win was confusing; RPM `%posttrans` also benefits from a + /// single, predictable mount story. + /// + /// A **different** enhancement would be Linux’s newer mount APIs to obtain a **`mount_fd`** and + /// do all ESP I/O via **`cap_std::fs::Dir`** without attaching to the global mount namespace. That + /// would require refactoring callers to work entirely through that fd; it is **not** implemented here. pub(crate) fn mount_esp_device(&self, root: &Path, esp_device: &Path) -> Result { // (mount path, whether bootupd performed the mount and must unmount) let mut mountpoint: Option<(PathBuf, bool)> = None; @@ -188,12 +170,6 @@ impl Efi { } } - if mount_esp(esp_device, &mnt).is_ok() { - log::debug!("Mounted at {mnt:?}"); - mountpoint = Some((mnt, true)); - break; - } - log::trace!("Mount failed, falling back to mount(8)"); std::process::Command::new("mount") .arg(esp_device) .arg(&mnt) @@ -234,15 +210,9 @@ impl Efi { .unwrap_or(false); if should_unmount { if let Some(mount) = self.mountpoint.borrow_mut().take() { - if rustix::mount::unmount(&mount.path, UnmountFlags::empty()).is_ok() { - log::trace!("Unmounted (new mount API)"); - } else { - Command::new("umount") - .arg(&mount.path) - .run_inherited() - .with_context(|| format!("Failed to unmount {:?}", mount.path))?; - log::trace!("Unmounted"); - } + rustix::mount::unmount(&mount.path, UnmountFlags::empty()) + .with_context(|| format!("Failed to unmount {:?}", mount.path))?; + log::trace!("Unmounted"); } } Ok(()) @@ -300,17 +270,21 @@ impl Efi { &self, sysroot_dir: &openat::Dir, esp_dir: &openat::Dir, - _esp_path: &Path, + esp_path: &Path, efi_components: &[EFIComponent], ) -> Result<()> { - // Build a merged source tree in a temp dir (same layout as desired ESP/EFI) - let temp_dir = tempfile::tempdir().context("Creating temp dir for EFI merge")?; - let temp_efi_path = temp_dir.path().join("EFI"); - std::fs::create_dir_all(&temp_efi_path) - .with_context(|| format!("Creating {}", temp_efi_path.display()))?; - let temp_efi_str = temp_efi_path + // Staging directory on the ESP (same filesystem as the destination) avoids merging under + // /tmp and copying across filesystems twice. Each component path is `...///EFI`; + // copy that directory into the staging root so the merged tree is `EFI//…` without + // using `{path}/.` tricks. + let temp_dir = tempfile::Builder::new() + .prefix(".bootupd-efi-merge-") + .tempdir_in(esp_path) + .with_context(|| format!("Creating EFI merge temp dir under {}", esp_path.display()))?; + let temp_root_str = temp_dir + .path() .to_str() - .context("Temp EFI path is not valid UTF-8")?; + .context("Temp merge path is not valid UTF-8")?; for efi_comp in efi_components { log::info!( @@ -318,13 +292,10 @@ impl Efi { efi_comp.name, efi_comp.version ); - // Copy contents of component's EFI dir (e.g. fedora/) into temp_efi_path so merged - // layout is EFI/fedora/..., not EFI/EFI/fedora/... - let src_efi_contents = format!("{}/.", efi_comp.path); filetree::copy_dir_with_args( sysroot_dir, - src_efi_contents.as_str(), - temp_efi_str, + efi_comp.path.as_str(), + temp_root_str, OPTIONS, ) .with_context(|| format!("Copying {} to merge dir", efi_comp.path))?; @@ -334,8 +305,8 @@ impl Efi { esp_dir.ensure_dir_all(std::path::Path::new("EFI"), 0o755)?; let esp_efi_dir = esp_dir.sub_dir("EFI").context("Opening ESP EFI dir")?; - let source_dir = - openat::Dir::open(&temp_efi_path).context("Opening merged EFI source dir")?; + let source_dir = openat::Dir::open(&temp_dir.path().join("EFI")) + .context("Opening merged EFI source dir")?; let source_filetree = filetree::FileTree::new_from_dir(&source_dir).context("Building source filetree")?; let current_filetree = @@ -343,7 +314,37 @@ impl Efi { let mut diff = current_filetree .diff(&source_filetree) .context("Computing EFI diff")?; - diff.removals.clear(); + + // Scoped removals (option 2): only delete paths under `EFI//` for `` present in + // this merge. Stale files under those trees are removed; other top-level ESP dirs are untouched. + let mut managed_prefixes: HashSet = source_filetree + .children + .keys() + .filter_map(|k| k.split('/').next().map(str::to_string)) + .collect(); + for entry in source_dir.list_dir(".")? { + let entry = entry?; + if matches!(source_dir.get_file_type(&entry)?, openat::SimpleType::Dir) { + if let Some(name) = entry.file_name().to_str() { + managed_prefixes.insert(name.to_string()); + } + } + } + let removals_before = diff.removals.len(); + diff.removals.retain(|path| { + path.split('/') + .next() + .map(|first| managed_prefixes.contains(first)) + .unwrap_or(false) + }); + let skipped = removals_before.saturating_sub(diff.removals.len()); + if skipped > 0 { + log::debug!( + "Skipped {} ESP removal(s) outside shipped EFI prefixes (managed: {:?})", + skipped, + managed_prefixes + ); + } // Check available space before writing to prevent partial updates when the partition is full let required_bytes = current_filetree.total_size() + source_filetree.total_size(); @@ -356,7 +357,7 @@ impl Efi { ); } - // Same logic as bootable container: write to .btmp.* then atomic rename + // Same logic as bootable container: write-aside + rename; removals only under managed dirs. filetree::apply_diff(&source_dir, &esp_efi_dir, &diff, None) .context("Applying EFI update (write alongside + atomic rename)")?; @@ -365,48 +366,6 @@ impl Efi { Ok(()) } - - /// Copy from /usr/lib/efi to boot/ESP. Caller provides sysroot (e.g. for recovery or tests). - fn package_mode_copy_to_boot_impl(&self, sysroot: &Path) -> Result<()> { - let sysroot_path = Utf8Path::from_path(sysroot) - .with_context(|| format!("Invalid UTF-8: {}", sysroot.display()))?; - let sysroot_dir = openat::Dir::open(sysroot).context("Opening sysroot for reading")?; - - let efi_comps = match get_efi_component_from_usr(sysroot_path, EFILIB)? { - Some(comps) if !comps.is_empty() => comps, - _ => anyhow::bail!("No EFI components found in /usr/lib/efi"), - }; - - // First try to use an already mounted ESP - let esp_path = if let Some(mounted_esp) = self.get_mounted_esp(sysroot)? { - mounted_esp - } else { - let sysroot_cap = Dir::open_ambient_dir(sysroot, cap_std::ambient_authority()) - .with_context(|| format!("Opening sysroot {}", sysroot.display()))?; - let device = bootc_internal_blockdev::list_dev_by_dir(&sysroot_cap) - .with_context(|| format!("Resolving block device for {}", sysroot.display()))?; - let Some(esp_devices) = device.find_colocated_esps()? else { - anyhow::bail!("No ESP found"); - }; - let esp = esp_devices - .first() - .ok_or_else(|| anyhow::anyhow!("No ESP partition found"))?; - self.ensure_mounted_esp(sysroot, Path::new(&esp.path()))? - }; - - let esp_dir = openat::Dir::open(&esp_path) - .with_context(|| format!("Opening ESP at {}", esp_path.display()))?; - validate_esp_fstype(&esp_dir)?; - - self.copy_efi_components_to_esp(&sysroot_dir, &esp_dir, &esp_path, &efi_comps)?; - - log::info!( - "Successfully copied {} EFI component(s) to ESP at {}", - efi_comps.len(), - esp_path.display() - ); - Ok(()) - } } #[context("Get product name")] @@ -855,8 +814,46 @@ impl Component for Efi { } /// Package mode: merge `/usr/lib/efi` onto the ESP (write alongside, then atomic rename). + /// Prefer an ESP already mounted at a well-known path; otherwise mount via `mount(8)` only. fn package_mode_copy_to_boot(&self, root: &Path) -> Result<()> { - self.package_mode_copy_to_boot_impl(root) + let sysroot_path = Utf8Path::from_path(root) + .with_context(|| format!("Invalid UTF-8: {}", root.display()))?; + let sysroot_dir = openat::Dir::open(root).context("Opening sysroot for reading")?; + + let efi_comps = match get_efi_component_from_usr(sysroot_path, EFILIB)? { + Some(comps) if !comps.is_empty() => comps, + _ => anyhow::bail!("No EFI components found in /usr/lib/efi"), + }; + + // Reuse existing ESP mount when present (RPM posttrans must not rely on extra mount APIs). + let esp_path = if let Some(mounted_esp) = self.get_mounted_esp(root)? { + mounted_esp + } else { + let sysroot_cap = Dir::open_ambient_dir(root, cap_std::ambient_authority()) + .with_context(|| format!("Opening sysroot {}", root.display()))?; + let device = bootc_internal_blockdev::list_dev_by_dir(&sysroot_cap) + .with_context(|| format!("Resolving block device for {}", root.display()))?; + let Some(esp_devices) = device.find_colocated_esps()? else { + anyhow::bail!("No ESP found"); + }; + let esp = esp_devices + .first() + .ok_or_else(|| anyhow::anyhow!("No ESP partition found"))?; + self.ensure_mounted_esp(root, Path::new(&esp.path()))? + }; + + let esp_dir = openat::Dir::open(&esp_path) + .with_context(|| format!("Opening ESP at {}", esp_path.display()))?; + validate_esp_fstype(&esp_dir)?; + + self.copy_efi_components_to_esp(&sysroot_dir, &esp_dir, &esp_path, &efi_comps)?; + + log::info!( + "Successfully copied {} EFI component(s) to ESP at {}", + efi_comps.len(), + esp_path.display() + ); + Ok(()) } } @@ -1373,4 +1370,48 @@ Boot0003* test"; Ok(()) } + + #[test] + fn test_copy_efi_scoped_removals_under_shipped_vendor_only() -> Result<()> { + let tmpdir: &tempfile::TempDir = &tempfile::tempdir()?; + let tpath = tmpdir.path(); + let efi_path = tpath.join("usr/lib/efi"); + let shim_path = efi_path.join("shim/15.8-3/EFI/fedora"); + std::fs::create_dir_all(&shim_path)?; + std::fs::write(shim_path.join(SHIM), b"shim v2")?; + + let esp_path = tpath.join("boot/efi"); + std::fs::create_dir_all(esp_path.join("EFI/fedora"))?; + std::fs::create_dir_all(esp_path.join("EFI/othervendor"))?; + std::fs::write(esp_path.join("EFI/fedora/stale.efi"), b"old")?; + std::fs::write(esp_path.join("EFI/othervendor/keep.efi"), b"keep")?; + + let sysroot_dir = openat::Dir::open(tpath)?; + let utf8_tpath = + Utf8Path::from_path(tpath).ok_or_else(|| anyhow::anyhow!("Path is not valid UTF-8"))?; + let efi_comps = get_efi_component_from_usr(utf8_tpath, EFILIB)?.unwrap(); + + let esp_dir = openat::Dir::open(&esp_path).context("Opening ESP dir for test")?; + let efi = Efi::default(); + efi.copy_efi_components_to_esp(&sysroot_dir, &esp_dir, &esp_path, &efi_comps)?; + + assert!( + !esp_path.join("EFI/fedora/stale.efi").exists(), + "stale file under shipped vendor should be removed" + ); + assert!( + esp_path.join("EFI/fedora").join(SHIM).exists(), + "shim should be present" + ); + assert!( + esp_path.join("EFI/othervendor/keep.efi").exists(), + "unmanaged vendor tree must be untouched" + ); + assert_eq!( + std::fs::read(esp_path.join("EFI/othervendor/keep.efi"))?, + b"keep" + ); + + Ok(()) + } } From 53dfd0ec3ee32296d09e41b57cdb58b561422127 Mon Sep 17 00:00:00 2001 From: Yasmin de Souza Date: Wed, 13 May 2026 12:08:50 -0300 Subject: [PATCH 7/7] mend --- src/bootupd.rs | 4 +--- src/component.rs | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/bootupd.rs b/src/bootupd.rs index c83090d7..6f177d17 100644 --- a/src/bootupd.rs +++ b/src/bootupd.rs @@ -802,9 +802,7 @@ pub(crate) fn client_run_migrate_static_grub_config() -> Result<()> { Ok(()) } -/// Copy EFI payloads from `/usr/lib/efi` onto the ESP for package-mode installs. Removals are -/// scoped to `EFI//` trees present in this merge; other top-level ESP dirs are untouched. -/// BIOS components are no-ops. +/// Copy EFI payloads from `/usr/lib/efi` onto the ESP for package-mode installs. pub(crate) fn copy_to_boot(root: &Path) -> Result<()> { let all_components = get_components_impl(false); if all_components.is_empty() { diff --git a/src/component.rs b/src/component.rs index 43d19607..0b8f5eb0 100644 --- a/src/component.rs +++ b/src/component.rs @@ -87,7 +87,6 @@ pub(crate) trait Component { fn get_efi_vendor(&self, sysroot: &Path) -> Result>; /// Merge `/usr/lib/efi` onto the ESP for package-mode systems (EFI only). Deletes only under - /// `EFI//` for vendors shipped in this merge; other ESP subtrees are left alone. fn package_mode_copy_to_boot(&self, root: &Path) -> Result<()>; }