From 54078f2b8cf2f326bfb5c34bb6c5359b43e9068f Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Sat, 19 Sep 2026 22:38:20 +0100 Subject: [PATCH 1/2] install: Only remove bootloader dirs from the ESP to-existing-root emptied the whole ESP, which can hold content bootc cannot recreate. On Asahi this includes m1n1/boot.bin, vendorfw/ and ubootefi.var; wiping them leaves the machine unbootable. Only remove EFI/ and loader/, which we reinstall. Factor out remove_dir_no_xdev() to share the recursive removal. Generated-by: AI Signed-off-by: Eric Curtin --- crates/lib/src/install.rs | 73 ++++++++++++++++++----- docs/src/bootc-installation.7.md | 5 +- tmt/tests/booted/test-multi-device-esp.nu | 31 +++++++++- 3 files changed, 90 insertions(+), 19 deletions(-) diff --git a/crates/lib/src/install.rs b/crates/lib/src/install.rs index 35acce7ee7..8f9f80fc47 100644 --- a/crates/lib/src/install.rs +++ b/crates/lib/src/install.rs @@ -2448,12 +2448,7 @@ fn remove_all_in_dir_no_xdev(d: &Dir, mount_err: bool) -> Result<()> { let name = entry.file_name(); let etype = entry.file_type()?; if etype == FileType::dir() { - if let Some(subdir) = d.open_dir_noxdev(&name)? { - remove_all_in_dir_no_xdev(&subdir, mount_err)?; - d.remove_dir(&name)?; - } else if mount_err { - anyhow::bail!("Found unexpected mount point {name:?}"); - } + remove_dir_no_xdev(d, &name, mount_err)?; } else { d.remove_file_optional(&name)?; } @@ -2461,6 +2456,19 @@ fn remove_all_in_dir_no_xdev(d: &Dir, mount_err: bool) -> Result<()> { anyhow::Ok(()) } +/// Recursively remove the directory `name` in `d`, without crossing devices. +/// A mount point is left alone, or is an error if `mount_err` is true. +fn remove_dir_no_xdev(d: &Dir, name: impl AsRef, mount_err: bool) -> Result<()> { + let name = name.as_ref(); + if let Some(subdir) = d.open_dir_noxdev(name)? { + remove_all_in_dir_no_xdev(&subdir, mount_err)?; + d.remove_dir(name)?; + } else if mount_err { + anyhow::bail!("Found unexpected mount point {name:?}"); + } + Ok(()) +} + #[context("Removing boot directory content except loader dir on ostree")] fn remove_all_except_loader_dirs(bootdir: &Dir, is_ostree: bool) -> Result<()> { let entries = bootdir @@ -2485,12 +2493,8 @@ fn remove_all_except_loader_dirs(bootdir: &Dir, is_ostree: bool) -> Result<()> { let etype = entry.file_type()?; if etype == FileType::dir() { - // Open the directory and remove its contents - if let Some(subdir) = bootdir.open_dir_noxdev(&file_name)? { - remove_all_in_dir_no_xdev(&subdir, false) - .with_context(|| format!("Removing directory contents: {}", file_name))?; - bootdir.remove_dir(&file_name)?; - } + remove_dir_no_xdev(bootdir, file_name, false) + .with_context(|| format!("Removing directory: {file_name}"))?; } else { bootdir .remove_file_optional(&file_name) @@ -2500,6 +2504,22 @@ fn remove_all_except_loader_dirs(bootdir: &Dir, is_ostree: bool) -> Result<()> { Ok(()) } +/// Remove the bootloader dirs (bootupd/grub, systemd-boot) from the ESP. +/// Other content, e.g. Asahi's `m1n1/` and `vendorfw/`, may be firmware +/// or earlier boot stages we cannot recreate, so it is preserved. +// TODO: be more selective, e.g. keep other OSes' `EFI/` and +// non-bootc `loader/` entries, and drop old Type #1 kernels (see #2243). +#[context("Removing bootloader content from EFI system partition")] +fn clean_esp_bootloader_dirs(efidir: &Dir) -> Result<()> { + for name in ["EFI", "loader"] { + if efidir.try_exists(name)? { + remove_dir_no_xdev(efidir, name, false) + .with_context(|| format!("Removing directory: {name}"))?; + } + } + Ok(()) +} + #[context("Removing boot directory content")] fn clean_boot_directories(rootfs: &Dir, rootfs_path: &Utf8Path, is_ostree: bool) -> Result<()> { let bootdir = @@ -2514,13 +2534,12 @@ fn clean_boot_directories(rootfs: &Dir, rootfs_path: &Utf8Path, is_ostree: bool) // This should not remove /boot/efi note. remove_all_except_loader_dirs(&bootdir, is_ostree).context("Emptying /boot")?; - // TODO: we should also support not wiping the ESP. if ARCH_USES_EFI { if let Some(efidir) = bootdir .open_dir_optional(crate::bootloader::EFI_DIR) .context("Opening /boot/efi")? { - remove_all_in_dir_no_xdev(&efidir, false).context("Emptying EFI system partition")?; + clean_esp_bootloader_dirs(&efidir)?; } } @@ -3249,6 +3268,32 @@ mod tests { Ok(()) } + #[test] + fn test_clean_esp_bootloader_dirs() -> Result<()> { + let td = cap_std_ext::cap_tempfile::TempDir::new(cap_std::ambient_authority())?; + + td.create_dir_all("EFI/BOOT")?; + td.write("EFI/BOOT/BOOTAA64.EFI", b"shim")?; + td.create_dir_all("loader/entries")?; + td.write("loader/entries/foo.conf", b"entry")?; + // Asahi content which must survive + td.create_dir_all("m1n1")?; + td.write("m1n1/boot.bin", b"m1n1")?; + td.write("ubootefi.var", b"efivars")?; + + clean_esp_bootloader_dirs(&td)?; + assert!(!td.exists("EFI")); + assert!(!td.exists("loader")); + assert_eq!(td.read("m1n1/boot.bin")?, b"m1n1"); + assert_eq!(td.read("ubootefi.var")?, b"efivars"); + + // Idempotent + clean_esp_bootloader_dirs(&td)?; + assert_eq!(td.entries()?.count(), 2); + + Ok(()) + } + #[test] fn test_read_boot_fstab_entry() -> Result<()> { let td = cap_std_ext::cap_tempfile::TempDir::new(cap_std::ambient_authority())?; diff --git a/docs/src/bootc-installation.7.md b/docs/src/bootc-installation.7.md index 3a87689c34..dbd576da34 100644 --- a/docs/src/bootc-installation.7.md +++ b/docs/src/bootc-installation.7.md @@ -233,8 +233,9 @@ Set the environment variable `BOOTC_DIRECT_IO=on` to create the loopback device This is a variant of `install to-filesystem`, which maximizes convenience for using an existing Linux system, converting it into the target container image. Note that -the `/boot` (and `/boot/efi`) partitions *will be reinitialized* - so this is a -somewhat destructive operation for the existing Linux installation. +the `/boot` partition and the `EFI/` and `loader/` directories of the ESP +*will be reinitialized* - so this is a somewhat destructive operation for the +existing Linux installation. Other ESP content (e.g. Asahi's `m1n1/`) is preserved. Also, because the filesystem is reused, it's required that the target system kernel support the root storage setup already initialized. diff --git a/tmt/tests/booted/test-multi-device-esp.nu b/tmt/tests/booted/test-multi-device-esp.nu index f972b6b35d..acf5bff4ff 100644 --- a/tmt/tests/booted/test-multi-device-esp.nu +++ b/tmt/tests/booted/test-multi-device-esp.nu @@ -16,7 +16,8 @@ # 3. Three devices, partial ESP: Three disks, ESP on disk1+disk3 only # # Reboot 2: -# 4. Single device (no LVM): ESP + root partition on a single disk +# 4. Single device (no LVM): ESP + root partition on a single disk; +# also checks that non-bootloader ESP content is preserved # 5. No ESP anywhere: Two disks with no ESP; install should fail gracefully # # This validates the fix for https://github.com/bootc-dev/bootc/issues/481 @@ -157,6 +158,17 @@ def validate_esp [esp_partition: string] { } } +# Mount an ESP partition, run a closure on the mountpoint, and unmount it +def with_esp [esp_partition: string, f: closure] { + let esp_mount = "/var/mnt/esp_seed" + mkdir $esp_mount + mount $esp_partition $esp_mount + let r = (do $f $esp_mount) + umount $esp_mount + rmdir $esp_mount + $r +} + # Run bootc install to-existing-root from within the container image under test def run_install [mountpoint: string] { (podman run @@ -356,8 +368,15 @@ def test_single_device_no_lvm [] { mkdir $mountpoint mount $"($loop1)p2" $mountpoint - # Create boot directory - mkdir $"($mountpoint)/boot" + # Create /boot/efi so the ESP gets mounted and cleaned + mkdir $"($mountpoint)/boot/efi" + + # Seed non-bootloader content (as on Asahi) and a stale bootloader dir + with_esp $"($loop1)p1" {|esp| + mkdir $"($esp)/m1n1" $"($esp)/EFI/stale" + "m1n1" | save $"($esp)/m1n1/boot.bin" + "stale" | save $"($esp)/EFI/stale/x.efi" + } # Show block device hierarchy lsblk --pairs --paths --inverse --output NAME,TYPE $"($loop1)p2" @@ -366,6 +385,12 @@ def test_single_device_no_lvm [] { # Validate ESP was installed correctly validate_esp $"($loop1)p1" + let r = (with_esp $"($loop1)p1" {|esp| { + m1n1: ($"($esp)/m1n1/boot.bin" | path exists) + stale: ($"($esp)/EFI/stale" | path exists) + }}) + assert $r.m1n1 "m1n1/boot.bin was removed from the ESP" + assert (not $r.stale) "EFI/stale was not removed from the ESP" } catch {|e| cleanup_simple $loop1 $mountpoint rm -f $disk1 From 3448338c91fb8ce5f8c29333e8c3976f9a808e2b Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 24 Sep 2026 21:48:57 -0400 Subject: [PATCH 2/2] xtask: Fail packaging if the vendor archive has PCRE2 C sources The vendor tarball excludes pcre2-sys's bundled sources via exclude-crate-paths, but that only matches a directory name. If a pcre2-sys update moved them, the exclusion would silently stop applying and we would be back to shipping (and possibly building) a second copy of PCRE2. Inspect the generated archive against a small denylist instead, so that shows up as a packaging failure. Longer term vendor-filterer itself should support an allowlist for C sources. Generated-by: AI Signed-off-by: Colin Walters --- crates/xtask/src/xtask.rs | 123 +++++++++++++++++++++++++++++++++++++- 1 file changed, 121 insertions(+), 2 deletions(-) diff --git a/crates/xtask/src/xtask.rs b/crates/xtask/src/xtask.rs index 654983ea1f..f5c47d88cb 100644 --- a/crates/xtask/src/xtask.rs +++ b/crates/xtask/src/xtask.rs @@ -8,7 +8,7 @@ use std::borrow::Cow; use std::fmt::Display; use std::fs::File; use std::io::{BufRead, BufReader, BufWriter, Write}; -use std::process::Command; +use std::process::{Command, Stdio}; use anyhow::{Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; @@ -32,6 +32,15 @@ const JSON_SCHEMAS: &[(&str, &str)] = &[ ]; /// File used to identify the bootc source tree toplevel. const TOPLEVEL_MARKER: &str = "ADOPTERS.md"; +/// Directory prefix of the vendored crates in the vendor archive. +const VENDOR_DIR: &str = "vendor"; +/// Files that must not appear in the vendor archive, as (crate, file extensions). +/// These crates link against system libraries, so their bundled C sources are +/// excluded via `exclude-crate-paths` in the toplevel Cargo.toml; this catches +/// a crate update that moves them somewhere the exclusion no longer covers. +const VENDOR_DENYLIST: &[(&str, &[&str])] = &[("pcre2-sys", &["c", "h"])]; +/// Maximum number of denied files to list in the error message. +const VENDOR_DENIED_SHOWN: usize = 10; const TAR_REPRODUCIBLE_OPTS: &[&str] = &[ "--sort=name", "--owner=0", @@ -500,6 +509,72 @@ fn edit_vendor_config(config: &str) -> Result { Ok(config.to_string()) } +/// If `path` (relative to the vendor directory) matches [`VENDOR_DENYLIST`], +/// return the name of the crate it belongs to. +fn vendor_denied_crate(path: &Utf8Path) -> Option<&'static str> { + let crate_dir = path.components().next()?.as_str(); + let ext = path.extension()?; + VENDOR_DENYLIST.iter().find_map(|&(name, exts)| { + // cargo vendor uses `-` when several versions are vendored + let is_crate = crate_dir == name + || crate_dir + .strip_prefix(name) + .and_then(|v| v.strip_prefix('-')) + .is_some_and(|v| v.starts_with(|c: char| c.is_ascii_digit())); + (is_crate && exts.contains(&ext)).then_some(name) + }) +} + +/// List the entries of the (uncompressed) vendor tar stream that match +/// [`VENDOR_DENYLIST`]. +fn vendor_denied_entries(archive: impl std::io::Read) -> Result> { + let mut denied = Vec::new(); + for entry in tar::Archive::new(archive).entries()? { + let entry = entry?; + let path = entry.path()?; + let path = path.to_string_lossy(); + let path = Utf8Path::new(&*path); + let relpath = path.strip_prefix(VENDOR_DIR).unwrap_or(path); + if let Some(name) = vendor_denied_crate(relpath) { + denied.push(format!("{path} (from {name})")); + } + } + Ok(denied) +} + +/// Verify that the vendor archive contains no files from [`VENDOR_DENYLIST`]. +#[context("Checking vendor archive {vendorpath}")] +fn check_vendor_archive(vendorpath: &Utf8Path) -> Result<()> { + let mut child = Command::new("zstd") + .args(["-dc", "--"]) + .arg(vendorpath) + .stdout(Stdio::piped()) + .spawn() + .context("Spawning zstd")?; + let stdout = child.stdout.take().expect("stdout is piped"); + let denied = vendor_denied_entries(stdout); + // Wait for zstd even if reading failed, so its error is not lost + let status = child.wait().context("Waiting for zstd")?; + let denied = denied.context("Reading tar entries")?; + if !status.success() { + anyhow::bail!("zstd failed: {status}"); + } + if !denied.is_empty() { + let n = denied.len(); + let shown = denied[..n.min(VENDOR_DENIED_SHOWN)].join("\n "); + let more = n.saturating_sub(VENDOR_DENIED_SHOWN); + let more = if more > 0 { + format!("\n ... and {more} more") + } else { + String::new() + }; + anyhow::bail!( + "Found {n} denylisted files; update exclude-crate-paths in Cargo.toml:\n {shown}{more}" + ); + } + Ok(()) +} + #[context("Packaging")] fn impl_package(sh: &Shell) -> Result { let source_date_epoch = git_source_date_epoch(".".into())?; @@ -514,9 +589,10 @@ fn impl_package(sh: &Shell) -> Result { let vendorpath = Utf8Path::new("target").join(format!("{namev}-vendor.tar.zstd")); let vendor_config = cmd!( sh, - "cargo vendor-filterer --prefix=vendor --format=tar.zstd {vendorpath}" + "cargo vendor-filterer --prefix={VENDOR_DIR} --format=tar.zstd {vendorpath}" ) .read()?; + check_vendor_archive(&vendorpath)?; let vendor_config = edit_vendor_config(&vendor_config)?; // Append .cargo/vendor-config.toml (a made up filename) into the tar archive. { @@ -840,4 +916,47 @@ mod tests { assert!(parse_cli_bool("").is_err()); assert!(parse_cli_bool("maybe").is_err()); } + + #[test] + fn test_vendor_denied_crate() { + let cases = [ + ("pcre2-sys/upstream/src/pcre2_compile.c", Some("pcre2-sys")), + ("pcre2-sys/upstream/src/pcre2_internal.h", Some("pcre2-sys")), + ("pcre2-sys/pcre2.h", Some("pcre2-sys")), + ( + "pcre2-sys-0.2.9/upstream/src/pcre2_compile.c", + Some("pcre2-sys"), + ), + ("pcre2-sys/src/lib.rs", None), + ("pcre2-sys/upstream/src/pcre2.h.generic", None), + ("pcre2-sys", None), + ("pcre2/src/ffi.c", None), + ("pcre2-sysx/foo.c", None), + ("pcre2-sys-extra/foo.c", None), + ("libz-sys/src/zlib/adler32.c", None), + ]; + for (path, expected) in cases { + assert_eq!(vendor_denied_crate(Utf8Path::new(path)), expected, "{path}"); + } + } + + #[test] + fn test_vendor_denied_entries() -> Result<()> { + let mut builder = tar::Builder::new(Vec::new()); + for path in [ + "vendor/pcre2-sys/src/lib.rs", + "vendor/pcre2-sys/upstream/src/pcre2_compile.c", + "vendor/libz-sys/src/zlib/adler32.c", + ] { + let mut header = tar::Header::new_gnu(); + header.set_size(0); + builder.append_data(&mut header, path, std::io::empty())?; + } + let archive = builder.into_inner()?; + assert_eq!( + vendor_denied_entries(archive.as_slice())?, + ["vendor/pcre2-sys/upstream/src/pcre2_compile.c (from pcre2-sys)"] + ); + Ok(()) + } }