diff --git a/crates/lib/src/fsck.rs b/crates/lib/src/fsck.rs index 25e95e33cf..5457d126de 100644 --- a/crates/lib/src/fsck.rs +++ b/crates/lib/src/fsck.rs @@ -6,7 +6,6 @@ // Unfortunately needed here to work with linkme #![allow(unsafe_code)] -use std::fmt::Write as _; use std::future::Future; use std::num::NonZeroUsize; use std::pin::Pin; @@ -26,37 +25,55 @@ use crate::store::Storage; use std::os::fd::AsFd; -/// A lint check has failed. -#[derive(thiserror::Error, Debug)] -pub(crate) struct FsckError(String); - -/// The outer error is for unexpected fatal runtime problems; the -/// inner error is for the check failing in an expected way. -pub(crate) type FsckResult = anyhow::Result>; +/// The kind of problem a check found. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum CheckFindingCode { + /// `/usr/etc/resolv.conf` is a zero-sized file. + ResolvconfZeroSized, + /// fsverity is enabled for the ostree repository, but not on an object. + ObjectWithoutFsverity, +} -/// Everything is OK - we didn't encounter a runtime error, and -/// the targeted check passed. -pub(crate) fn fsck_ok() -> FsckResult { - Ok(Ok(())) +impl std::fmt::Display for CheckFindingCode { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Debug::fmt(self, f) + } } -/// We successfully found a failure. -pub(crate) fn fsck_err(msg: impl AsRef) -> FsckResult { - Ok(Err(FsckError::new(msg))) +/// A problem a check found in the system. +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct CheckFinding { + code: CheckFindingCode, + /// What the problem is in, e.g. a path or an object. + subject: String, + detail: String, } -impl std::fmt::Display for FsckError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(&self.0) +impl CheckFinding { + fn new(code: CheckFindingCode, subject: impl Into, detail: impl Into) -> Self { + Self { + code, + subject: subject.into(), + detail: detail.into(), + } } } -impl FsckError { - fn new(msg: impl AsRef) -> Self { - Self(msg.as_ref().to_owned()) +impl std::fmt::Display for CheckFinding { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}: {}: {}", self.code, self.subject, self.detail) } } +/// Like the lints, checks have two levels of errors: the outer `Result` is +/// for unexpected runtime errors, which stop the check; every problem the +/// check finds in the system is collected as a finding, and the check runs +/// to completion. No findings means the check passed. +pub(crate) type FsckResult = anyhow::Result>; + +/// How many findings of one check are printed; the rest are counted. +const MAX_PRINTED_FINDINGS: NonZeroUsize = NonZeroUsize::new(5).unwrap(); + pub(crate) type FsckFn = fn(&Storage) -> FsckResult; pub(crate) type AsyncFsckFn = fn(&Storage) -> Pin + '_>>; #[derive(Debug)] @@ -108,21 +125,33 @@ static CHECK_RESOLVCONF: FsckCheck = fn check_resolvconf(storage: &Storage) -> FsckResult { let ostree = match storage.get_ostree() { Ok(o) => o, - Err(_) => return fsck_ok(), // Not an ostree system (e.g. composefs-only) + Err(_) => return Ok(Vec::new()), // Not an ostree system (e.g. composefs-only) }; // For now we only check the booted deployment. if ostree.booted_deployment().is_none() { - return fsck_ok(); + return Ok(Vec::new()); } - // Read usr/etc/resolv.conf directly. let usr = Dir::open_ambient_dir("/usr", cap_std::ambient_authority())?; + check_resolvconf_in(&usr) +} + +/// The subject of the resolv.conf check's finding. +const RESOLVCONF_SUBJECT: &str = "usr/etc/resolv.conf"; + +/// The resolv.conf check proper, on the `/usr` directory `usr`. +fn check_resolvconf_in(usr: &Dir) -> FsckResult { + // Read usr/etc/resolv.conf directly. let Some(meta) = usr.symlink_metadata_optional("etc/resolv.conf")? else { - return fsck_ok(); + return Ok(Vec::new()); }; if meta.is_file() && meta.size() == 0 { - return fsck_err("Found usr/etc/resolv.conf as zero-sized file"); + return Ok(vec![CheckFinding::new( + CheckFindingCode::ResolvconfZeroSized, + RESOLVCONF_SUBJECT, + "zero-sized file", + )]); } - fsck_ok() + Ok(Vec::new()) } #[derive(Debug, Default)] @@ -237,7 +266,7 @@ fn check_fsverity(storage: &Storage) -> Pin async fn check_fsverity_inner(storage: &Storage) -> FsckResult { let ostree = match storage.get_ostree() { Ok(o) => o, - Err(_) => return fsck_ok(), // Not an ostree system (e.g. composefs-only) + Err(_) => return Ok(Vec::new()), // Not an ostree system (e.g. composefs-only) }; let repo = &ostree.repo(); let verity_state = ostree_ext::fsverity::is_verity_enabled(repo)?; @@ -250,22 +279,44 @@ async fn check_fsverity_inner(storage: &Storage) -> FsckResult { let verity_found_state = verity_state_of_all_objects(&ostree.repo(), verity_state.desired == Tristate::Enabled) .await?; - let Some((missing, rest)) = collect_until( - verity_found_state.missing.iter(), - const { NonZeroUsize::new(5).unwrap() }, - ) else { - return fsck_ok(); + let findings = verity_found_state + .missing + .into_iter() + .map(|obj| { + CheckFinding::new( + CheckFindingCode::ObjectWithoutFsverity, + obj, + "missing fsverity, which is enabled for the repository", + ) + }) + .collect(); + Ok(findings) +} + +/// Print the result of the check `name`, returning whether it passed. +fn print_check_result( + name: &str, + result: &FsckResult, + mut output: impl std::io::Write, +) -> std::io::Result { + let findings = match result { + Ok(findings) => findings, + Err(e) => { + writeln!(output, "Unexpected runtime error in check {name}: {e:#}")?; + return Ok(false); + } }; - let mut err = String::from("fsverity enabled, but objects without fsverity:\n"); - for obj in missing { - // SAFETY: Writing into a String - writeln!(err, " {obj}").unwrap(); + let Some((shown, rest)) = collect_until(findings.iter(), MAX_PRINTED_FINDINGS) else { + writeln!(output, "ok: {name}")?; + return Ok(true); + }; + for finding in shown { + writeln!(output, "fsck error: {name}: {finding}")?; } if rest > 0 { - // SAFETY: Writing into a String - writeln!(err, " ...and {rest} more").unwrap(); + writeln!(output, "fsck error: {name}: ...and {rest} more")?; } - fsck_err(err) + Ok(false) } pub(crate) async fn fsck(storage: &Storage, mut output: impl std::io::Write) -> anyhow::Result<()> { @@ -279,18 +330,8 @@ pub(crate) async fn fsck(storage: &Storage, mut output: impl std::io::Write) -> FsckFnImpl::Sync(f) => f(&storage), FsckFnImpl::Async(f) => f(&storage).await, }; - match r { - Ok(Ok(())) => { - println!("ok: {name}"); - } - Ok(Err(e)) => { - errors = true; - writeln!(output, "fsck error: {name}: {e}")?; - } - Err(e) => { - errors = true; - writeln!(output, "Unexpected runtime error in check {name}: {e}")?; - } + if !print_check_result(name, &r, &mut output)? { + errors = true; } } if errors { @@ -311,3 +352,109 @@ pub(crate) async fn fsck(storage: &Storage, mut output: impl std::io::Write) -> Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use cap_std_ext::cap_tempfile; + + #[test] + fn test_check_resolvconf() -> anyhow::Result<()> { + let zero_sized = || { + vec![CheckFinding::new( + CheckFindingCode::ResolvconfZeroSized, + RESOLVCONF_SUBJECT, + "zero-sized file", + )] + }; + // Sets up etc/resolv.conf (if anything) in a fresh /usr + type Setup = fn(&Dir) -> std::io::Result<()>; + let cases: &[(&str, Setup, Vec)] = &[ + ("missing", |_| Ok(()), Vec::new()), + ( + "zero-sized", + |d| d.write("etc/resolv.conf", ""), + zero_sized(), + ), + ( + "nonempty", + |d| d.write("etc/resolv.conf", "nameserver 192.0.2.1\n"), + Vec::new(), + ), + ( + "symlink", + |d| d.symlink("../run/resolv.conf", "etc/resolv.conf"), + Vec::new(), + ), + ("directory", |d| d.create_dir("etc/resolv.conf"), Vec::new()), + ]; + for (name, setup, expected) in cases { + let usr = cap_tempfile::tempdir(cap_std::ambient_authority())?; + usr.create_dir("etc")?; + setup(&usr)?; + assert_eq!(&check_resolvconf_in(&usr)?, expected, "case {name}"); + } + Ok(()) + } + + #[test] + fn test_print_check_result() { + let objects = |n: usize| -> FsckResult { + Ok((0..n) + .map(|i| { + CheckFinding::new( + CheckFindingCode::ObjectWithoutFsverity, + format!("{i:02}.file"), + "detail", + ) + }) + .collect()) + }; + let cases: &[(&str, FsckResult, bool, &str)] = &[ + ("no findings", Ok(Vec::new()), true, "ok: c\n"), + ( + "one finding", + objects(1), + false, + "fsck error: c: ObjectWithoutFsverity: 00.file: detail\n", + ), + ( + "at the limit", + objects(5), + false, + indoc::indoc! {" + fsck error: c: ObjectWithoutFsverity: 00.file: detail + fsck error: c: ObjectWithoutFsverity: 01.file: detail + fsck error: c: ObjectWithoutFsverity: 02.file: detail + fsck error: c: ObjectWithoutFsverity: 03.file: detail + fsck error: c: ObjectWithoutFsverity: 04.file: detail + "}, + ), + ( + "over the limit", + objects(8), + false, + indoc::indoc! {" + fsck error: c: ObjectWithoutFsverity: 00.file: detail + fsck error: c: ObjectWithoutFsverity: 01.file: detail + fsck error: c: ObjectWithoutFsverity: 02.file: detail + fsck error: c: ObjectWithoutFsverity: 03.file: detail + fsck error: c: ObjectWithoutFsverity: 04.file: detail + fsck error: c: ...and 3 more + "}, + ), + ( + "runtime error", + Err(anyhow::anyhow!("inner").context("outer")), + false, + "Unexpected runtime error in check c: outer: inner\n", + ), + ]; + for (name, result, passed, expected) in cases { + let mut output = Vec::new(); + let r = print_check_result("c", result, &mut output).unwrap(); + assert_eq!(r, *passed, "case {name}"); + assert_eq!(String::from_utf8(output).unwrap(), *expected, "case {name}"); + } + } +} 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