From 2b107d8eb35809651d569177fc8a0c0c560d9043 Mon Sep 17 00:00:00 2001 From: Dustin Kirkland Date: Sun, 9 Aug 2026 15:44:54 -0500 Subject: [PATCH 1/2] composefs/status: Detect BLS layout on non-EFI systems MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `get_bootloader()` unconditionally returns `Bootloader::Grub` when there are no EFI variables to inspect (`SystemNotUEFI` / `MissingVar`). That's wrong for many non-EFI setups that use the Boot Loader Specification Type 1 entry layout at `/boot/loader/entries/` without any EFI vars to advertise it — Raspberry Pi 4/5 with direct- kernel boot from Pi firmware, U-Boot with the extlinux/BLS loader, coreboot chaining to a bare kernel, and various ARM/embedded boards. When bootc misclassifies these as `Bootloader::Grub` → `BootloaderKind::GRUBClassic`, `storage::new` sets `boot_dir = physical_root.open_dir("boot")` = `/sysroot/boot/`. On systems where `/boot` is a separate partition (the ESP mounted at `/boot` via the `systemd.mount-extra=UUID=:/boot:auto:ro` cmdline that `bootc install to-filesystem` itself writes), `/sysroot/boot/` is empty. Every subsequent code path that reads BLS entries via `boot_dir.read_dir("loader/entries")` then `ENOENT`s — including the idempotent `prepend_custom_prefix()` backwards-compat migration called from `storage::new` itself, which is why `bootc status`, `bootc upgrade`, and `bootc switch` all fail at storage init. This bug was masked before #2356 by an EBUSY on the pre-mounted ESP. With that fixed, execution now reaches `prepend_custom_prefix`, which is where the wrong `boot_dir` gets used. Fix: when there are no EFI vars, stat `/boot/loader/entries`. If it is a directory, treat the bootloader as BLS-compatible; otherwise fall back to `Bootloader::Grub` as before. The probe is a single `stat(2)` and the else-branch preserves prior behaviour on real grub-classic systems (where `/boot/grub2/` exists but `/boot/loader/entries/` does not). Split the inner match into a pure `classify_bootloader(efi_result, bls_present) -> Result` helper per REVIEW_RUST.md "separate parsing from I/O" guidance, and add a table-driven unit test covering both prior branches and both new branches. Preserve the pre-existing "don't cache on EFI" behavior of `get_bootloader()`: the old code had an early-return in the `Ok(loader)` branch that bypassed the `OnceLock` cache, and the grub-cc TMT plans observed bootloader-info changes over a run (discovered via `is_composefs` → `bootc status --json` in `tap.nu` after v1 of this PR unified the caching path). The new code caches only when the classification came from the FS probe (non-EFI, filesystem-stable state). Verified on aarch64 with `bootc` built from this branch: before the fix, `bootc status` errored at "Prepending custom prefix to EFI and BLS entries: Getting sorted Type1 boot entries: No such file or directory (os error 2)"; after, it returns a healthy `BootcHost` report with `bootType: Bls`, and `bootc switch --transport=registry` proceeds normally. Assisted-by: Claude (Opus 4) Signed-off-by: Dustin Kirkland Closes: #2375 --- crates/lib/src/bootc_composefs/status.rs | 159 ++++++++++++++++++++--- 1 file changed, 138 insertions(+), 21 deletions(-) diff --git a/crates/lib/src/bootc_composefs/status.rs b/crates/lib/src/bootc_composefs/status.rs index c2a984e794..1245df10ce 100644 --- a/crates/lib/src/bootc_composefs/status.rs +++ b/crates/lib/src/bootc_composefs/status.rs @@ -440,6 +440,55 @@ pub(crate) async fn get_container_manifest_and_config( Ok(ImgConfigManifest { manifest, config }) } +/// Directory where systemd-boot / BLS-compatible bootloaders expect Type 1 +/// boot entries. Its presence is used as a signal that a non-EFI system +/// nevertheless uses the BLS layout (see [`classify_bootloader`]). +const BLS_ENTRIES_DIR: &str = "/boot/loader/entries"; + +/// Pure classifier for the bootloader kind, split from I/O for testability. +/// +/// - When `EFI_LOADER_INFO` is present, its content selects between systemd- +/// boot, GRUB Confidential Compute, and generic GRUB (existing behavior). +/// - When there are no EFI variables to inspect (`SystemNotUEFI` / +/// `MissingVar`), fall back to a filesystem probe: many non-EFI systems +/// still lay down the BLS Type 1 entry layout at `/boot/loader/entries/` +/// (Raspberry Pi with direct-kernel boot from Pi firmware, U-Boot with +/// the extlinux/BLS loader, coreboot with a linux payload, various +/// ARM/embedded boards). Treat those as BLS-compatible so `storage::new` +/// picks the ESP mount as `boot_dir` rather than `/sysroot/boot/`. Only +/// fall back to GRUB when neither an EFI system nor a BLS layout is +/// present. +/// - Other EFI read errors propagate. +fn classify_bootloader( + efi_loader_info: Result, + bls_entries_dir_present: bool, +) -> Result { + match efi_loader_info { + Ok(loader) => { + let loader = loader.to_lowercase(); + if loader.contains("systemd-boot") { + Ok(Bootloader::Systemd) + } else if loader.contains("grub cc") { + Ok(Bootloader::GrubCC) + } else { + Ok(Bootloader::Grub) + } + } + Err(EfiError::SystemNotUEFI) | Err(EfiError::MissingVar) => { + if bls_entries_dir_present { + tracing::debug!( + "No EFI vars but {BLS_ENTRIES_DIR} is a directory; \ + treating bootloader as BLS-compatible (systemd-boot)" + ); + Ok(Bootloader::Systemd) + } else { + Ok(Bootloader::Grub) + } + } + Err(e) => anyhow::bail!("Failed to read EfiLoaderInfo: {e:?}"), + } +} + #[context("Getting bootloader")] pub(crate) fn get_bootloader() -> Result { static BOOTLOADER: OnceLock = OnceLock::new(); @@ -448,28 +497,28 @@ pub(crate) fn get_bootloader() -> Result { return Ok(*bootloader); } - let bootloader = match read_uefi_var(EFI_LOADER_INFO) { - Ok(loader) => { - if loader.to_lowercase().contains("systemd-boot") { - return Ok(Bootloader::Systemd); - } - - if loader.to_lowercase().contains("grub cc") { - return Ok(Bootloader::GrubCC); - } - - return Ok(Bootloader::Grub); - } - - Err(efi_error) => match efi_error { - EfiError::SystemNotUEFI | EfiError::MissingVar => Bootloader::Grub, - e => anyhow::bail!("Failed to read EfiLoaderInfo: {e:?}"), - }, - }; - - BOOTLOADER.get_or_init(|| bootloader); + let efi_result = read_uefi_var(EFI_LOADER_INFO); + // Non-EFI systems have a stable filesystem-based classification, so we + // can cache. EFI systems are left uncached to preserve the pre-existing + // behavior of re-reading `EFI_LOADER_INFO` on every call — some tests + // observe bootloader-info changes over the course of a run. + let non_efi = matches!( + &efi_result, + Err(EfiError::SystemNotUEFI) | Err(EfiError::MissingVar), + ); + + let bootloader = classify_bootloader( + efi_result, + // The FS probe is only consulted in the non-EFI classification + // branch; skip the `stat(2)` on EFI systems. + non_efi && std::path::Path::new(BLS_ENTRIES_DIR).is_dir(), + )?; + + if non_efi { + BOOTLOADER.get_or_init(|| bootloader); + } - return Ok(bootloader); + Ok(bootloader) } /// Retrieves the OCI manifest and config for a deployment from the composefs repository. @@ -1089,6 +1138,74 @@ mod tests { assert_eq!(v.digest.as_ref(), DIGEST); } + #[test] + fn classify_bootloader_cases() { + struct Case { + desc: &'static str, + efi: Result, + bls: bool, + expected: Bootloader, + } + let cases = [ + Case { + desc: "UEFI, EFI_LOADER_INFO advertises systemd-boot", + efi: Ok("systemd-boot 261.2".into()), + bls: false, + expected: Bootloader::Systemd, + }, + Case { + desc: "UEFI, EFI_LOADER_INFO advertises GRUB CC", + efi: Ok("GRUB CC 2.12".into()), + bls: false, + expected: Bootloader::GrubCC, + }, + Case { + desc: "UEFI, EFI_LOADER_INFO advertises unknown; default GRUB", + efi: Ok("something else 1.0".into()), + bls: false, + expected: Bootloader::Grub, + }, + Case { + desc: "Non-EFI + BLS layout present: BLS (regression fix)", + efi: Err(EfiError::SystemNotUEFI), + bls: true, + expected: Bootloader::Systemd, + }, + Case { + desc: "Non-EFI + no BLS layout: fall back to GRUB", + efi: Err(EfiError::SystemNotUEFI), + bls: false, + expected: Bootloader::Grub, + }, + Case { + desc: "EFI mounted but EFI_LOADER_INFO missing, BLS present", + efi: Err(EfiError::MissingVar), + bls: true, + expected: Bootloader::Systemd, + }, + Case { + desc: "EFI mounted but EFI_LOADER_INFO missing, no BLS: GRUB", + efi: Err(EfiError::MissingVar), + bls: false, + expected: Bootloader::Grub, + }, + ]; + for case in cases { + let got = classify_bootloader(case.efi, case.bls) + .unwrap_or_else(|e| panic!("{}: {e}", case.desc)); + assert_eq!(got, case.expected, "{}", case.desc); + } + } + + #[test] + fn classify_bootloader_propagates_other_efi_errors() { + let result = classify_bootloader( + Err(EfiError::InvalidData("test-only synthetic error")), + false, + ); + assert!(result.is_err(), "InvalidData should propagate as an error"); + } + #[test] fn test_sorted_bls_boot_entries() -> Result<()> { let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; From 7f35fc4d945d3b819525ef5ec2446fb1c9685c52 Mon Sep 17 00:00:00 2001 From: Dustin Kirkland Date: Sat, 15 Aug 2026 16:27:14 -0500 Subject: [PATCH 2/2] composefs/status: Disambiguate GRUB from BLS on non-EFI systems Addresses review feedback on #2376: a legacy BIOS system that has a BLS entries directory would be classified as `Bootloader::Systemd` even though GRUB may still own the boot flow. That is a real configuration, not a hypothetical one. GRUB reads Type 1 entries itself via the `blscfg` module, and Fedora and RHEL enable that by default with `GRUB_ENABLE_BLSCFG=true`. So a legacy-BIOS Fedora or RHEL install has BOTH `/boot/grub2/` and `/boot/loader/entries/`, and the BLS probe alone cannot tell it apart from a BLS-native bootloader. Probe for GRUB's own directory and let it win when both are present: grub dir + BLS entries -> Grub (Fedora/RHEL legacy BIOS, blscfg) grub dir, no BLS -> Grub (classic GRUB, unchanged) BLS entries, no grub -> Systemd (Pi 5 direct-kernel, U-Boot, coreboot) neither -> Grub (unchanged fallback) `/boot/grub2` is the Fedora/RHEL path and `/boot/grub` the Debian/Ubuntu one; both are checked. The probes still only run in the non-EFI branch, so EFI systems are unaffected and pay no extra `stat(2)`. Note the deliberate trade-off: a system migrated from GRUB to a BLS-native bootloader that left an empty `/boot/grub` behind now classifies as GRUB. That is strictly closer to correct than the behaviour on main, which returns `Bootloader::Grub` for every non-EFI system regardless, and a leftover GRUB directory is reasonable evidence that GRUB was installed. Probing for `grub.cfg` specifically would be narrower, at the cost of missing a GRUB install whose config has not been generated yet. Extends the table-driven test from 7 cases to 12, covering both new branches, the both-present disambiguation, and two regression guards: that a BLS layout with no GRUB directory is still detected as BLS (the Pi 5 case this PR exists to fix), and that UEFI classification ignores both filesystem probes entirely. Verified the new cases are not vacuous by temporarily disabling the grub-dir branch: `classify_bootloader_cases` then fails with `left: Systemd, right: Grub` on the both-present case. Full `bootc-lib` unit suite passes (233 tests), `cargo fmt --check` is clean, and clippy reports no findings in the changed regions. Assisted-by: Claude (Opus 5) Signed-off-by: Dustin Kirkland --- crates/lib/src/bootc_composefs/status.rs | 88 ++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 6 deletions(-) diff --git a/crates/lib/src/bootc_composefs/status.rs b/crates/lib/src/bootc_composefs/status.rs index 1245df10ce..58a084bced 100644 --- a/crates/lib/src/bootc_composefs/status.rs +++ b/crates/lib/src/bootc_composefs/status.rs @@ -445,6 +445,14 @@ pub(crate) async fn get_container_manifest_and_config( /// nevertheless uses the BLS layout (see [`classify_bootloader`]). const BLS_ENTRIES_DIR: &str = "/boot/loader/entries"; +/// Directories where GRUB keeps its own configuration and modules. Their +/// presence means GRUB owns the boot flow even if BLS Type 1 entries also +/// exist, because GRUB can consume those entries itself via the `blscfg` +/// module — Fedora and RHEL enable exactly that with +/// `GRUB_ENABLE_BLSCFG=true`. `/boot/grub2` is the Fedora/RHEL path, +/// `/boot/grub` the Debian/Ubuntu one. +const GRUB_DIRS: [&str; 2] = ["/boot/grub2", "/boot/grub"]; + /// Pure classifier for the bootloader kind, split from I/O for testability. /// /// - When `EFI_LOADER_INFO` is present, its content selects between systemd- @@ -458,10 +466,18 @@ const BLS_ENTRIES_DIR: &str = "/boot/loader/entries"; /// picks the ESP mount as `boot_dir` rather than `/sysroot/boot/`. Only /// fall back to GRUB when neither an EFI system nor a BLS layout is /// present. +/// +/// A BLS entries directory alone is not sufficient evidence, because GRUB +/// with `blscfg` reads the same directory. So GRUB's own directory wins +/// when both are present: a legacy-BIOS Fedora/RHEL install has +/// `/boot/grub2/` *and* `/boot/loader/entries/`, and is unambiguously +/// GRUB. Only a BLS layout with no GRUB directory implies a BLS-native +/// bootloader. /// - Other EFI read errors propagate. fn classify_bootloader( efi_loader_info: Result, bls_entries_dir_present: bool, + grub_dir_present: bool, ) -> Result { match efi_loader_info { Ok(loader) => { @@ -475,10 +491,18 @@ fn classify_bootloader( } } Err(EfiError::SystemNotUEFI) | Err(EfiError::MissingVar) => { - if bls_entries_dir_present { + if grub_dir_present { + tracing::debug!( + "No EFI vars and a GRUB directory is present; treating \ + bootloader as GRUB even if BLS entries also exist \ + (GRUB reads them via blscfg)" + ); + Ok(Bootloader::Grub) + } else if bls_entries_dir_present { tracing::debug!( - "No EFI vars but {BLS_ENTRIES_DIR} is a directory; \ - treating bootloader as BLS-compatible (systemd-boot)" + "No EFI vars, no GRUB directory, and {BLS_ENTRIES_DIR} is \ + a directory; treating bootloader as BLS-compatible \ + (systemd-boot)" ); Ok(Bootloader::Systemd) } else { @@ -509,9 +533,10 @@ pub(crate) fn get_bootloader() -> Result { let bootloader = classify_bootloader( efi_result, - // The FS probe is only consulted in the non-EFI classification - // branch; skip the `stat(2)` on EFI systems. + // The FS probes are only consulted in the non-EFI classification + // branch; skip the `stat(2)`s on EFI systems. non_efi && std::path::Path::new(BLS_ENTRIES_DIR).is_dir(), + non_efi && GRUB_DIRS.iter().any(|d| std::path::Path::new(d).is_dir()), )?; if non_efi { @@ -1144,6 +1169,7 @@ mod tests { desc: &'static str, efi: Result, bls: bool, + grub_dir: bool, expected: Bootloader, } let cases = [ @@ -1151,47 +1177,96 @@ mod tests { desc: "UEFI, EFI_LOADER_INFO advertises systemd-boot", efi: Ok("systemd-boot 261.2".into()), bls: false, + grub_dir: false, expected: Bootloader::Systemd, }, Case { desc: "UEFI, EFI_LOADER_INFO advertises GRUB CC", efi: Ok("GRUB CC 2.12".into()), bls: false, + grub_dir: false, expected: Bootloader::GrubCC, }, Case { desc: "UEFI, EFI_LOADER_INFO advertises unknown; default GRUB", efi: Ok("something else 1.0".into()), bls: false, + grub_dir: false, expected: Bootloader::Grub, }, Case { desc: "Non-EFI + BLS layout present: BLS (regression fix)", efi: Err(EfiError::SystemNotUEFI), bls: true, + grub_dir: false, expected: Bootloader::Systemd, }, Case { desc: "Non-EFI + no BLS layout: fall back to GRUB", efi: Err(EfiError::SystemNotUEFI), bls: false, + grub_dir: false, expected: Bootloader::Grub, }, Case { desc: "EFI mounted but EFI_LOADER_INFO missing, BLS present", efi: Err(EfiError::MissingVar), bls: true, + grub_dir: false, expected: Bootloader::Systemd, }, Case { desc: "EFI mounted but EFI_LOADER_INFO missing, no BLS: GRUB", efi: Err(EfiError::MissingVar), bls: false, + grub_dir: false, + expected: Bootloader::Grub, + }, + // A legacy-BIOS Fedora/RHEL install with GRUB_ENABLE_BLSCFG=true + // has both directories and is unambiguously GRUB. Without the + // GRUB probe this case returned Systemd, which is the + // misclassification raised in review on #2376. + Case { + desc: "Non-EFI + BLS layout + GRUB dir: GRUB wins (blscfg)", + efi: Err(EfiError::SystemNotUEFI), + bls: true, + grub_dir: true, + expected: Bootloader::Grub, + }, + Case { + desc: "Non-EFI + GRUB dir, no BLS: GRUB", + efi: Err(EfiError::SystemNotUEFI), + bls: false, + grub_dir: true, expected: Bootloader::Grub, }, + Case { + desc: "EFI_LOADER_INFO missing + BLS + GRUB dir: GRUB wins", + efi: Err(EfiError::MissingVar), + bls: true, + grub_dir: true, + expected: Bootloader::Grub, + }, + // The regression this PR fixes must survive the new probe: a + // BLS layout with no GRUB directory is still BLS-native. + Case { + desc: "Non-EFI + BLS, no GRUB dir: still BLS (Pi 5, U-Boot)", + efi: Err(EfiError::SystemNotUEFI), + bls: true, + grub_dir: false, + expected: Bootloader::Systemd, + }, + // UEFI classification must ignore both probes entirely. + Case { + desc: "UEFI systemd-boot with a stray GRUB dir: still systemd", + efi: Ok("systemd-boot 261.2".into()), + bls: true, + grub_dir: true, + expected: Bootloader::Systemd, + }, ]; for case in cases { - let got = classify_bootloader(case.efi, case.bls) + let got = classify_bootloader(case.efi, case.bls, case.grub_dir) .unwrap_or_else(|e| panic!("{}: {e}", case.desc)); assert_eq!(got, case.expected, "{}", case.desc); } @@ -1202,6 +1277,7 @@ mod tests { let result = classify_bootloader( Err(EfiError::InvalidData("test-only synthetic error")), false, + false, ); assert!(result.is_err(), "InvalidData should propagate as an error"); }