From dbc5bf2f779ec05f254c3b36e64045e34ec6e537 Mon Sep 17 00:00:00 2001 From: Jannes Stubbemann Date: Sat, 4 Jul 2026 09:14:59 +0200 Subject: [PATCH] fix(guest): init skips kernel-premounted targets, ending the false /dev ERROR Every VM boot logged 'ERROR mount /dev: Resource busy': the guest kernel automounts devtmpfs on /dev (CONFIG_DEVTMPFS_MOUNT) before init runs, so init's own devtmpfs mount always returned EBUSY, was logged at ERROR, and execution continued. The line was benign and deterministic (14 occurrences in a fully green main firecracker-test run) but it got real CI failures misattributed to the guest; issue #668 itself was filed against it while the actual failures were the PTY stdin race (#670) and PR-local bugs. init now checks /proc/mounts before each mount-table entry: a target already carrying the expected fstype is skipped at info level; a target mounted with a different fstype is left in place and logged at ERROR (that state is genuinely unexpected); an unmounted target is mounted as before. is_mounted is refactored onto the same parser, which now returns the LAST matching /proc/mounts line so overmounts resolve to the visible mount. The firecracker-test boot smoke gains a regression gate: the serial log must not contain the false /dev ERROR. Verified: cargo test in rust:1-bookworm (privileged) passes 134 tests with the same 10 environment-dependent failures as origin/main baseline; the fork::volumes mount-namespace tests exercise the refactored is_mounted against real mounts. Closes #668. Co-Authored-By: Claude Fable 5 Signed-off-by: Jannes Stubbemann --- .github/workflows/kvm-test.yaml | 10 ++++ guest/agent-rs/src/init/mod.rs | 75 ++++++++++++++++++++++++-- guest/agent-rs/src/sys/mount.rs | 95 ++++++++++++++++++++++++++++----- 3 files changed, 163 insertions(+), 17 deletions(-) diff --git a/.github/workflows/kvm-test.yaml b/.github/workflows/kvm-test.yaml index 10a78246..612a8106 100644 --- a/.github/workflows/kvm-test.yaml +++ b/.github/workflows/kvm-test.yaml @@ -528,6 +528,16 @@ jobs: exit 1 } + # The guest kernel automounts devtmpfs on /dev (CONFIG_DEVTMPFS_MOUNT), + # so a correct init must skip that mount instead of logging an ERROR + # for the EBUSY (issue #668): that ERROR appeared in every boot and got + # real CI failures misattributed to the guest. Regression-gate it. + if grep -q "mount /dev: Resource busy" $WORKDIR/serial.log; then + echo "FAIL: guest init logged a false ERROR for the kernel-premounted /dev (issue #668)" + exit 1 + fi + echo "OK: no false 'mount /dev' ERROR in the boot serial log (issue #668)" + kill $FC_PID 2>/dev/null || true # gRPC runtime protocol proof (issue #24 stage 5, Task 5.4). This phase diff --git a/guest/agent-rs/src/init/mod.rs b/guest/agent-rs/src/init/mod.rs index 754a468c..4bb7b530 100644 --- a/guest/agent-rs/src/init/mod.rs +++ b/guest/agent-rs/src/init/mod.rs @@ -5,9 +5,36 @@ // continues, because a PID-1 init failure must not halt the guest entirely. // This matches the Go agent's behavior (fmt.Fprintf(os.Stderr, ...) + continue). -use crate::sys::mount::{mount, sethostname}; +use crate::sys::mount::{mount, mounted_fstype, sethostname}; use tracing::error; +/// What to do for a mount-table entry given what is already mounted at its +/// target. The kernel automounts devtmpfs on /dev before init runs +/// (CONFIG_DEVTMPFS_MOUNT), so a blind mount there always returns EBUSY; +/// logging that as an ERROR is boot-log noise that gets real CI failures +/// misattributed to the guest (issue #668). +#[derive(Debug, PartialEq, Eq)] +enum PremountAction { + /// Nothing mounted at the target: perform the mount. + Mount, + /// The target already carries the expected fstype: skip silently + /// (info-level), the work is done. + SkipAlreadyMounted, + /// The target is mounted with a DIFFERENT fstype: skip but log an error, + /// because the guest image is in a state init does not expect. + SkipUnexpectedFstype, +} + +/// Decide the premount action from the fstype currently mounted at the +/// target (None = not mounted) and the fstype the mount table wants. +fn premount_action(existing: Option<&str>, want_fstype: &str) -> PremountAction { + match existing { + None => PremountAction::Mount, + Some(fs) if fs == want_fstype => PremountAction::SkipAlreadyMounted, + Some(_) => PremountAction::SkipUnexpectedFstype, + } +} + /// Describes a single filesystem mount. /// /// The source, target, and fstype fields mirror the corresponding arguments to @@ -50,8 +77,24 @@ pub fn init_system() { if let Err(e) = std::fs::create_dir_all(m.target) { error!("mkdir {}: {}", m.target, e); } - if let Err(e) = mount(m.source, m.target, m.fstype, 0) { - error!("mount {}: {}", m.target, e); + let existing = mounted_fstype(m.target); + match premount_action(existing.as_deref(), m.fstype) { + PremountAction::SkipAlreadyMounted => { + tracing::info!("{} already mounted ({}), skipping", m.target, m.fstype); + } + PremountAction::SkipUnexpectedFstype => { + error!( + "{}: already mounted as {} (expected {}); leaving it in place", + m.target, + existing.as_deref().unwrap_or("?"), + m.fstype + ); + } + PremountAction::Mount => { + if let Err(e) = mount(m.source, m.target, m.fstype, 0) { + error!("mount {}: {}", m.target, e); + } + } } } if let Err(e) = std::fs::create_dir_all("/workspace") { @@ -132,4 +175,30 @@ mod tests { fn mount_table_has_six_entries() { assert_eq!(MOUNT_TABLE.len(), 6); } + + // Premount decision: the kernel automounts devtmpfs on /dev before init + // runs (CONFIG_DEVTMPFS_MOUNT), so init must skip an already-mounted + // target instead of logging a false ERROR on the EBUSY (issue #668). + use super::{PremountAction, premount_action}; + + #[test] + fn premount_skips_target_already_mounted_with_expected_fstype() { + assert_eq!( + premount_action(Some("devtmpfs"), "devtmpfs"), + PremountAction::SkipAlreadyMounted + ); + } + + #[test] + fn premount_flags_target_mounted_with_unexpected_fstype() { + assert_eq!( + premount_action(Some("tmpfs"), "devtmpfs"), + PremountAction::SkipUnexpectedFstype + ); + } + + #[test] + fn premount_mounts_when_target_not_mounted() { + assert_eq!(premount_action(None, "devtmpfs"), PremountAction::Mount); + } } diff --git a/guest/agent-rs/src/sys/mount.rs b/guest/agent-rs/src/sys/mount.rs index 85597365..3b617238 100644 --- a/guest/agent-rs/src/sys/mount.rs +++ b/guest/agent-rs/src/sys/mount.rs @@ -48,28 +48,51 @@ pub fn is_mounted(mount_path: &str) -> bool { #[cfg(target_os = "linux")] fn is_mounted_linux(mount_path: &str) -> bool { - use std::io::BufRead; + mounted_fstype(mount_path).is_some() +} - let f = match std::fs::File::open("/proc/mounts") { - Ok(f) => f, - Err(_) => return false, - }; - let reader = std::io::BufReader::new(f); - for line in reader.lines() { - let line = match line { - Ok(l) => l, - Err(_) => return false, - }; +/// Return the filesystem type mounted at `mount_path`, or None when nothing +/// is mounted there (or /proc/mounts cannot be read, so the caller falls +/// through to a mount attempt that fails loudly rather than silently skipping). +/// +/// PID-1 init uses this to detect targets the kernel already mounted before +/// the agent ran: with CONFIG_DEVTMPFS_MOUNT the kernel automounts devtmpfs +/// on /dev, so init's own devtmpfs mount would return EBUSY (issue #668). +/// +/// On non-Linux platforms always returns None (mount is a no-op there). +pub fn mounted_fstype(mount_path: &str) -> Option { + let content = read_proc_mounts()?; + fstype_from_mounts(&content, mount_path) +} + +#[cfg(target_os = "linux")] +fn read_proc_mounts() -> Option { + std::fs::read_to_string("/proc/mounts").ok() +} + +#[cfg(not(target_os = "linux"))] +fn read_proc_mounts() -> Option { + None +} + +/// Parse /proc/mounts content: return the fstype (field 3) of the line whose +/// mount target (field 2) equals `mount_path` exactly. When several lines +/// match (overmounts), the LAST one wins: /proc/mounts is ordered oldest +/// first and the most recent mount is the visible one. Separated from the +/// /proc/mounts read so it is unit-testable on any host. +fn fstype_from_mounts(content: &str, mount_path: &str) -> Option { + let mut found = None; + for line in content.lines() { let mut fields = line.split_ascii_whitespace(); - // Field 0: device, field 1: mount target. + // Field 0: device, field 1: mount target, field 2: fstype. let _device = fields.next(); if let Some(target) = fields.next() && target == mount_path { - return true; + found = fields.next().map(str::to_string); } } - false + found } /// Mount a filesystem. @@ -157,6 +180,50 @@ fn sethostname_linux(name: &str) -> io::Result<()> { mod tests { use super::*; + const MOUNTS: &str = "\ +proc /proc proc rw,nosuid,nodev,noexec,relatime 0 0 +sysfs /sys sysfs rw,nosuid,nodev,noexec,relatime 0 0 +devtmpfs /dev devtmpfs rw,nosuid,size=12448k,nr_inodes=31112,mode=755 0 0 +/dev/vda / ext4 rw,relatime 0 0 +tmpfs /tmp tmpfs rw,relatime 0 0 +"; + + #[test] + fn fstype_from_mounts_finds_dev_devtmpfs() { + assert_eq!( + fstype_from_mounts(MOUNTS, "/dev"), + Some("devtmpfs".to_string()) + ); + } + + #[test] + fn fstype_from_mounts_none_for_unmounted_path() { + assert_eq!(fstype_from_mounts(MOUNTS, "/run"), None); + } + + #[test] + fn fstype_from_mounts_matches_exact_target_only() { + // "/de" and "/dev/pts" must not match the "/dev" entry. + assert_eq!(fstype_from_mounts(MOUNTS, "/de"), None); + assert_eq!(fstype_from_mounts(MOUNTS, "/dev/pts"), None); + } + + #[test] + fn fstype_from_mounts_ignores_malformed_lines() { + assert_eq!(fstype_from_mounts("devtmpfs /dev\n\n", "/dev"), None); + } + + #[test] + fn fstype_from_mounts_last_overmount_wins() { + // /proc/mounts is ordered oldest first; the visible mount at a + // target is the most recent line. + let content = "devtmpfs /dev devtmpfs rw 0 0\ntmpfs /dev tmpfs rw 0 0\n"; + assert_eq!( + fstype_from_mounts(content, "/dev"), + Some("tmpfs".to_string()) + ); + } + #[test] fn mount_no_op_on_non_linux_or_bad_target() { // On macOS this is a compile-out no-op and always returns Ok.