Skip to content

Commit c3671e0

Browse files
dustinkirklanddustinkirkland
committed
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 <dustin.kirkland@chainguard.dev>
1 parent a5e60a9 commit c3671e0

1 file changed

Lines changed: 82 additions & 6 deletions

File tree

‎crates/lib/src/bootc_composefs/status.rs‎

Lines changed: 82 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,14 @@ pub(crate) async fn get_container_manifest_and_config(
445445
/// nevertheless uses the BLS layout (see [`classify_bootloader`]).
446446
const BLS_ENTRIES_DIR: &str = "/boot/loader/entries";
447447

448+
/// Directories where GRUB keeps its own configuration and modules. Their
449+
/// presence means GRUB owns the boot flow even if BLS Type 1 entries also
450+
/// exist, because GRUB can consume those entries itself via the `blscfg`
451+
/// module — Fedora and RHEL enable exactly that with
452+
/// `GRUB_ENABLE_BLSCFG=true`. `/boot/grub2` is the Fedora/RHEL path,
453+
/// `/boot/grub` the Debian/Ubuntu one.
454+
const GRUB_DIRS: [&str; 2] = ["/boot/grub2", "/boot/grub"];
455+
448456
/// Pure classifier for the bootloader kind, split from I/O for testability.
449457
///
450458
/// - When `EFI_LOADER_INFO` is present, its content selects between systemd-
@@ -458,10 +466,18 @@ const BLS_ENTRIES_DIR: &str = "/boot/loader/entries";
458466
/// picks the ESP mount as `boot_dir` rather than `/sysroot/boot/`. Only
459467
/// fall back to GRUB when neither an EFI system nor a BLS layout is
460468
/// present.
469+
///
470+
/// A BLS entries directory alone is not sufficient evidence, because GRUB
471+
/// with `blscfg` reads the same directory. So GRUB's own directory wins
472+
/// when both are present: a legacy-BIOS Fedora/RHEL install has
473+
/// `/boot/grub2/` *and* `/boot/loader/entries/`, and is unambiguously
474+
/// GRUB. Only a BLS layout with no GRUB directory implies a BLS-native
475+
/// bootloader.
461476
/// - Other EFI read errors propagate.
462477
fn classify_bootloader(
463478
efi_loader_info: Result<String, EfiError>,
464479
bls_entries_dir_present: bool,
480+
grub_dir_present: bool,
465481
) -> Result<Bootloader> {
466482
match efi_loader_info {
467483
Ok(loader) => {
@@ -475,10 +491,18 @@ fn classify_bootloader(
475491
}
476492
}
477493
Err(EfiError::SystemNotUEFI) | Err(EfiError::MissingVar) => {
478-
if bls_entries_dir_present {
494+
if grub_dir_present {
495+
tracing::debug!(
496+
"No EFI vars and a GRUB directory is present; treating \
497+
bootloader as GRUB even if BLS entries also exist \
498+
(GRUB reads them via blscfg)"
499+
);
500+
Ok(Bootloader::Grub)
501+
} else if bls_entries_dir_present {
479502
tracing::debug!(
480-
"No EFI vars but {BLS_ENTRIES_DIR} is a directory; \
481-
treating bootloader as BLS-compatible (systemd-boot)"
503+
"No EFI vars, no GRUB directory, and {BLS_ENTRIES_DIR} is \
504+
a directory; treating bootloader as BLS-compatible \
505+
(systemd-boot)"
482506
);
483507
Ok(Bootloader::Systemd)
484508
} else {
@@ -509,9 +533,10 @@ pub(crate) fn get_bootloader() -> Result<Bootloader> {
509533

510534
let bootloader = classify_bootloader(
511535
efi_result,
512-
// The FS probe is only consulted in the non-EFI classification
513-
// branch; skip the `stat(2)` on EFI systems.
536+
// The FS probes are only consulted in the non-EFI classification
537+
// branch; skip the `stat(2)`s on EFI systems.
514538
non_efi && std::path::Path::new(BLS_ENTRIES_DIR).is_dir(),
539+
non_efi && GRUB_DIRS.iter().any(|d| std::path::Path::new(d).is_dir()),
515540
)?;
516541

517542
if non_efi {
@@ -1144,54 +1169,104 @@ mod tests {
11441169
desc: &'static str,
11451170
efi: Result<String, EfiError>,
11461171
bls: bool,
1172+
grub_dir: bool,
11471173
expected: Bootloader,
11481174
}
11491175
let cases = [
11501176
Case {
11511177
desc: "UEFI, EFI_LOADER_INFO advertises systemd-boot",
11521178
efi: Ok("systemd-boot 261.2".into()),
11531179
bls: false,
1180+
grub_dir: false,
11541181
expected: Bootloader::Systemd,
11551182
},
11561183
Case {
11571184
desc: "UEFI, EFI_LOADER_INFO advertises GRUB CC",
11581185
efi: Ok("GRUB CC 2.12".into()),
11591186
bls: false,
1187+
grub_dir: false,
11601188
expected: Bootloader::GrubCC,
11611189
},
11621190
Case {
11631191
desc: "UEFI, EFI_LOADER_INFO advertises unknown; default GRUB",
11641192
efi: Ok("something else 1.0".into()),
11651193
bls: false,
1194+
grub_dir: false,
11661195
expected: Bootloader::Grub,
11671196
},
11681197
Case {
11691198
desc: "Non-EFI + BLS layout present: BLS (regression fix)",
11701199
efi: Err(EfiError::SystemNotUEFI),
11711200
bls: true,
1201+
grub_dir: false,
11721202
expected: Bootloader::Systemd,
11731203
},
11741204
Case {
11751205
desc: "Non-EFI + no BLS layout: fall back to GRUB",
11761206
efi: Err(EfiError::SystemNotUEFI),
11771207
bls: false,
1208+
grub_dir: false,
11781209
expected: Bootloader::Grub,
11791210
},
11801211
Case {
11811212
desc: "EFI mounted but EFI_LOADER_INFO missing, BLS present",
11821213
efi: Err(EfiError::MissingVar),
11831214
bls: true,
1215+
grub_dir: false,
11841216
expected: Bootloader::Systemd,
11851217
},
11861218
Case {
11871219
desc: "EFI mounted but EFI_LOADER_INFO missing, no BLS: GRUB",
11881220
efi: Err(EfiError::MissingVar),
11891221
bls: false,
1222+
grub_dir: false,
1223+
expected: Bootloader::Grub,
1224+
},
1225+
// A legacy-BIOS Fedora/RHEL install with GRUB_ENABLE_BLSCFG=true
1226+
// has both directories and is unambiguously GRUB. Without the
1227+
// GRUB probe this case returned Systemd, which is the
1228+
// misclassification raised in review on #2376.
1229+
Case {
1230+
desc: "Non-EFI + BLS layout + GRUB dir: GRUB wins (blscfg)",
1231+
efi: Err(EfiError::SystemNotUEFI),
1232+
bls: true,
1233+
grub_dir: true,
1234+
expected: Bootloader::Grub,
1235+
},
1236+
Case {
1237+
desc: "Non-EFI + GRUB dir, no BLS: GRUB",
1238+
efi: Err(EfiError::SystemNotUEFI),
1239+
bls: false,
1240+
grub_dir: true,
11901241
expected: Bootloader::Grub,
11911242
},
1243+
Case {
1244+
desc: "EFI_LOADER_INFO missing + BLS + GRUB dir: GRUB wins",
1245+
efi: Err(EfiError::MissingVar),
1246+
bls: true,
1247+
grub_dir: true,
1248+
expected: Bootloader::Grub,
1249+
},
1250+
// The regression this PR fixes must survive the new probe: a
1251+
// BLS layout with no GRUB directory is still BLS-native.
1252+
Case {
1253+
desc: "Non-EFI + BLS, no GRUB dir: still BLS (Pi 5, U-Boot)",
1254+
efi: Err(EfiError::SystemNotUEFI),
1255+
bls: true,
1256+
grub_dir: false,
1257+
expected: Bootloader::Systemd,
1258+
},
1259+
// UEFI classification must ignore both probes entirely.
1260+
Case {
1261+
desc: "UEFI systemd-boot with a stray GRUB dir: still systemd",
1262+
efi: Ok("systemd-boot 261.2".into()),
1263+
bls: true,
1264+
grub_dir: true,
1265+
expected: Bootloader::Systemd,
1266+
},
11921267
];
11931268
for case in cases {
1194-
let got = classify_bootloader(case.efi, case.bls)
1269+
let got = classify_bootloader(case.efi, case.bls, case.grub_dir)
11951270
.unwrap_or_else(|e| panic!("{}: {e}", case.desc));
11961271
assert_eq!(got, case.expected, "{}", case.desc);
11971272
}
@@ -1202,6 +1277,7 @@ mod tests {
12021277
let result = classify_bootloader(
12031278
Err(EfiError::InvalidData("test-only synthetic error")),
12041279
false,
1280+
false,
12051281
);
12061282
assert!(result.is_err(), "InvalidData should propagate as an error");
12071283
}

0 commit comments

Comments
 (0)