From 207eb14f588d358aac2430111560254c62283d16 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 14:49:26 -0700 Subject: [PATCH 1/2] fix(controller): detect and warn on a shared writable rootfs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Firecracker serialises the rootfs drive's `path_on_host` and `is_read_only` into the vmstate and reopens both verbatim at restore. Neither can be overridden at load time: the API has no `drive_overrides` (searched upstream v1.12 — no such field exists) and the vmstate is a binary blob, not JSON, so forkd's JSON path rewriting cannot reach it. A snapshot baked with a writable rootfs therefore gives every restored child the SAME ext4 opened read-write. Two concurrent children are two guest kernels writing one filesystem with no coordinator: `/usr/bin/uname` picks up another file's bytes (still a plausible ELF, so an "it executes" check passes), directory entries return EBADMSG, and the damage surfaces as a random compile failure rather than a sandbox error. This is the bridge, not the destination. Per-child writable layers need the guest to know it is a child, which is a design decision (issue #317) because the boot path cannot tell it: the command line is frozen in the vmstate too. What lands here is the part every option needs and that is safe on its own: - `Snapshot.rootfs_read_only: Option` — purely additive, serde defaulted and skipped when absent, so existing snapshot.json files stay byte-identical. The CLI bake records it. - `Snapshot::rootfs_is_read_only()` resolves the recorded flag, and for pre-flag snapshots infers an `.ext4` rootfs as writable (the same convention the boot path uses to pick `ext4_rw`), leaving anything else unknown rather than asserting it is safe. - The controller warns on a restore that would add a second writer to a writable-rootfs snapshot, naming the live holders and both remedies. `FORKD_REFUSE_SHARED_RW=1` turns that into a 409. The refusal is opt-in on purpose: it rejects `fork -n N` on a writable snapshot, which is the flow `from-image` prints as its next step, and that behaviour change should be the operator's call rather than a surprise in a patch release. Signed-off-by: jrimmer --- CHANGELOG.md | 27 ++++++++++ crates/forkd-cli/src/hub.rs | 3 ++ crates/forkd-cli/src/main.rs | 7 +++ crates/forkd-controller/src/http.rs | 78 +++++++++++++++++++++++++++++ crates/forkd-vmm/src/chain.rs | 1 + crates/forkd-vmm/src/lib.rs | 76 ++++++++++++++++++++++++++++ 6 files changed, 192 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00594206..7cf632b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,33 @@ snapshot is fully published, so a tag always holds a consistent src == dst guard comment duplication and makes the chain-unpack tail bail instead of resolving an empty path. +### Snapshots record the rootfs drive's read-only flag + +`snapshot.json` now carries `rootfs_read_only`, the flag in effect when +the vmstate was frozen. It matters because Firecracker serialises the +drive's `path_on_host` and `is_read_only` into the vmstate and reopens +both verbatim at restore, with no way to override either at load time — +the API has no `drive_overrides` and the vmstate is a binary blob, not +JSON. A snapshot baked with a writable rootfs therefore hands *every* +restored child the same ext4 opened read-write, and two concurrent +children are two guest kernels writing one filesystem with no +coordinator: package files pick up other files' bytes, directory entries +go `EBADMSG`, and the damage surfaces as a random build failure rather +than a sandbox error. + +The controller detects this and warns when a restore would add a second +writer to a writable-rootfs snapshot, naming the live holders and the +remedies (re-bake read-only, or restore one child at a time). With +`FORKD_REFUSE_SHARED_RW=1` the warning becomes a `409`. Refusing is +opt-in because it rejects `fork -n N` on a writable snapshot — the flow +`from-image` prints as its next step — and that behaviour change should +be the operator's choice. + +Snapshots written before this field existed carry no flag; +`Snapshot::rootfs_is_read_only` infers an `.ext4` rootfs as writable, +the same convention the boot path uses, and leaves anything else unknown +rather than asserting it is safe. + ### Rootfs sidecar placement: recorded absolute path, validated Packs record the rootfs sidecar's target as the vmstate-frozen ABSOLUTE diff --git a/crates/forkd-cli/src/hub.rs b/crates/forkd-cli/src/hub.rs index f2b9f13f..46eb814f 100644 --- a/crates/forkd-cli/src/hub.rs +++ b/crates/forkd-cli/src/hub.rs @@ -1486,6 +1486,7 @@ mod tests { parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }; std::fs::write( base_dir.join("snapshot.json"), @@ -1505,6 +1506,7 @@ mod tests { parent_tag: Some("py-base".to_string()), parent_content_hash: Some(base_hash), rootfs: None, + rootfs_read_only: None, }; std::fs::write( head_dir.join("snapshot.json"), @@ -1530,6 +1532,7 @@ mod tests { parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }; std::fs::write( base_dir.join("snapshot.json"), diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index 58232022..84e798f5 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -3046,6 +3046,12 @@ fn snapshot_cmd( .unwrap_or_else(|_| cfg.rootfs.clone()), ); } + // Record the drive's read-only flag alongside its path. The flag is + // frozen into the binary vmstate and reopens verbatim for every + // restored child, so a snapshot with a writable rootfs cannot be + // restored concurrently without those children sharing one ext4. The + // daemon reads this to warn — or refuse — instead of corrupting. + snap.rootfs_read_only = Some(!rw); eprintln!(" snapshot took {} ms", t.elapsed().as_millis()); // Persist Snapshot metadata so subsequent `forkd fork` / `forkd run` @@ -3306,6 +3312,7 @@ fn load_snapshot_meta(snap_dir: &std::path::Path) -> Result { parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }) } diff --git a/crates/forkd-controller/src/http.rs b/crates/forkd-controller/src/http.rs index 1b559995..9d34b169 100644 --- a/crates/forkd-controller/src/http.rs +++ b/crates/forkd-controller/src/http.rs @@ -749,6 +749,7 @@ async fn compact_snapshot( // Compaction flattens but doesn't change the rootfs — inherit // the head's so the compacted base stays hub-portable (#242). rootfs: head_snapshot.rootfs.clone(), + rootfs_read_only: None, }; let staging_meta_path = staging.join("snapshot.json"); let new_meta_json = match serde_json::to_vec_pretty(&new_meta) { @@ -828,6 +829,7 @@ fn load_snapshot_with_fallback(snap_dir: &std::path::Path) -> forkd_vmm::Snapsho parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }) } @@ -850,6 +852,7 @@ fn load_snapshot_for_restore(snap_dir: &std::path::Path) -> Result Option { } } +/// Opt-in: refuse (409) instead of warning when a snapshot's rootfs is +/// shared writable and a live sandbox already has it open. +/// +/// Off by default because refusing rejects `fork -n N` on a writable +/// snapshot, which is the flow `from-image` prints as the next step — +/// making that a hard error is a behaviour change the operator should +/// choose, not one to spring on them. On, it converts the current +/// "one writer at a time" discipline into an enforced invariant instead +/// of scheduling luck. +fn refuse_shared_writable_rootfs() -> bool { + matches!( + std::env::var("FORKD_REFUSE_SHARED_RW").as_deref(), + Ok("1") | Ok("true") | Ok("yes") + ) +} + +/// `Some(reason)` when restoring this snapshot would add a second +/// concurrent writer to a rootfs that is not read-only. +/// +/// A restored child reopens the rootfs drive at the path AND the +/// read-only flag frozen into the vmstate, and Firecracker offers no way +/// to override either at load time — there is no `drive_overrides` on +/// `/snapshot/load`, and the vmstate is a binary blob rather than JSON. +/// So children of a writable-rootfs snapshot share one ext4: two of them +/// are two guest kernels writing one filesystem with no coordinator, +/// which corrupts package files and directory entries at random and +/// surfaces as a broken build rather than a broken sandbox. +fn shared_writable_rootfs_conflict( + s: &AppState, + tag: &str, + snapshot: &forkd_vmm::Snapshot, +) -> Option { + // `None` (not recorded, not inferable) cannot be asserted writable, + // so it does not block. Only a definite writable rootfs does. + if snapshot.rootfs_is_read_only() != Some(false) { + return None; + } + let live: std::collections::HashSet = s.live_vms.lock().keys().cloned().collect(); + let holders: Vec = s + .registry + .list_sandboxes() + .into_iter() + .filter(|sb| sb.snapshot_tag == tag && live.contains(&sb.id)) + .map(|sb| sb.id) + .collect(); + if holders.is_empty() { + return None; + } + Some(format!( + "snapshot `{tag}` was baked with a WRITABLE rootfs, so every restored child reopens the \ + same ext4 read-write and concurrent children corrupt each other's files. {} live \ + sandbox(es) already hold it ({}). Re-bake the snapshot read-only, or restore one child \ + at a time; set FORKD_REFUSE_SHARED_RW=1 to make this a hard error.", + holders.len(), + holders.join(", ") + )) +} + fn snapshot_info_from_disk_dir(tag: String, dir: PathBuf) -> SnapshotInfo { let created_at_unix = dir .metadata() @@ -1165,6 +1226,16 @@ async fn create_sandbox( req.snapshot_tag )); } + // A writable-rootfs snapshot restored while another child already + // holds it means two writers on one ext4 — corruption, not a warning + // (see `shared_writable_rootfs_conflict`). Warn by default; refuse + // when the operator has opted in. + if let Some(reason) = shared_writable_rootfs_conflict(&s, &req.snapshot_tag, &snapshot) { + if refuse_shared_writable_rootfs() { + return conflict(&reason); + } + tracing::warn!(snapshot = %req.snapshot_tag, "{reason}"); + } // v0.5: if the loaded snapshot has parent_tag set, this is a // chained snapshot. Resolve the chain, verify parent content @@ -2008,6 +2079,7 @@ async fn branch_sandbox( parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }); } #[cfg(not(target_os = "linux"))] @@ -2139,6 +2211,7 @@ async fn branch_sandbox( parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, } } else { let snap = vm.snapshot_to( @@ -2939,6 +3012,7 @@ fn spawn_one_for_workspace( parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }, }; let netns_reservation = if per_child_netns { @@ -3242,6 +3316,7 @@ async fn suspend_workspace( parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, } } else { let snap = vm.snapshot_to( @@ -3508,6 +3583,7 @@ mod tests { parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }; std::fs::write( dir.join("snapshot.json"), @@ -3535,6 +3611,7 @@ mod tests { parent_tag: Some(parent_tag.to_string()), parent_content_hash: Some(parent_hash.to_string()), rootfs: None, + rootfs_read_only: None, }; std::fs::write( dir.join("snapshot.json"), @@ -4335,6 +4412,7 @@ mod tests { parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }; std::fs::write( state_dir.join("snapshot.json"), diff --git a/crates/forkd-vmm/src/chain.rs b/crates/forkd-vmm/src/chain.rs index e1e62c3e..ec70ca58 100644 --- a/crates/forkd-vmm/src/chain.rs +++ b/crates/forkd-vmm/src/chain.rs @@ -396,6 +396,7 @@ mod tests { parent_tag: parent.map(String::from), parent_content_hash: hash.map(String::from), rootfs: None, + rootfs_read_only: None, } } diff --git a/crates/forkd-vmm/src/lib.rs b/crates/forkd-vmm/src/lib.rs index e94a3829..076d8c7d 100644 --- a/crates/forkd-vmm/src/lib.rs +++ b/crates/forkd-vmm/src/lib.rs @@ -615,6 +615,44 @@ pub struct Snapshot { /// the source's rootfs. #[serde(default, skip_serializing_if = "Option::is_none")] pub rootfs: Option, + /// Whether the rootfs drive was opened read-only when this snapshot's + /// vmstate was frozen. Firecracker serialises the drive's + /// `path_on_host` + `is_read_only` into the vmstate and reopens both + /// verbatim at restore, and neither can be overridden at load time + /// (Firecracker has no `drive_overrides` on `/snapshot/load`, and the + /// vmstate is a binary blob, not JSON). So this flag is what every + /// restored child inherits, and it decides whether concurrent + /// children share one writable ext4 — which is mutual corruption, not + /// a warning. + /// + /// Recorded by the CLI bake. `None` for snapshots written before the + /// field existed and for daemon-side branches that inherit the + /// source's rootfs; see [`Snapshot::rootfs_is_read_only`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub rootfs_read_only: Option, +} + +impl Snapshot { + /// Whether restored children open the rootfs read-write. + /// + /// `None` means "not recorded and not inferable" — a snapshot with no + /// rootfs path at all (daemon-side branches inherit the source's), or + /// one from before this field existed. Callers must not read `None` + /// as safe; it means they cannot vouch for the drive. + /// + /// For snapshots predating the field, an `.ext4` rootfs is inferred + /// writable, because that is exactly the convention the boot path uses + /// to choose `BootConfig::ext4_rw` (`rw_flag || extension == + /// "ext4"`). Anything else stays unknown rather than being asserted + /// read-only. + pub fn rootfs_is_read_only(&self) -> Option { + self.rootfs_read_only.or_else(|| { + self.rootfs + .as_ref() + .filter(|p| p.extension().is_some_and(|e| e == "ext4")) + .map(|_| false) + }) + } } /// Result of a Diff snapshot. `memory_diff` is a sparse file the same @@ -1867,6 +1905,7 @@ impl Vm { parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }) } @@ -1976,6 +2015,7 @@ impl Vm { parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }) } @@ -2652,6 +2692,39 @@ mod tests { } } + /// The read-only flag decides whether a restored child is a second + /// writer on a shared ext4, so the resolution rules matter: a + /// recorded flag wins, and a pre-flag snapshot is inferred only where + /// the `.ext4` convention makes it unambiguous. + #[test] + fn rootfs_read_only_prefers_recorded_flag_then_infers_ext4() { + let snap = |rootfs: Option<&str>, flag: Option| Snapshot { + vmstate: PathBuf::from("/s/vmstate"), + memory: PathBuf::from("/s/memory.bin"), + volumes: Vec::new(), + parent_tag: None, + parent_content_hash: None, + rootfs: rootfs.map(PathBuf::from), + rootfs_read_only: flag, + }; + // Recorded wins over the extension convention, both ways. + assert_eq!( + snap(Some("/r.ext4"), Some(true)).rootfs_is_read_only(), + Some(true) + ); + assert_eq!(snap(None, Some(false)).rootfs_is_read_only(), Some(false)); + // Pre-flag snapshot: `.ext4` is exactly the case the boot path + // treats as writable, so infer it rather than leaving it unknown. + assert_eq!( + snap(Some("/r.ext4"), None).rootfs_is_read_only(), + Some(false) + ); + // Anything else stays unknown — never asserted read-only, and + // never assumed safe. + assert_eq!(snap(Some("/r.img"), None).rootfs_is_read_only(), None); + assert_eq!(snap(None, None).rootfs_is_read_only(), None); + } + #[test] fn kernel_supports_kvm_clock_realtime_parses_versions() { assert!(!kernel_supports_kvm_clock_realtime("5.10.0-foo")); @@ -2995,6 +3068,7 @@ mod tests { parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }; let opts = ForkOpts { n: 2, @@ -3028,6 +3102,7 @@ mod tests { parent_tag: None, parent_content_hash: None, rootfs: None, + rootfs_read_only: None, }; let json = serde_json::to_string(&s).unwrap(); let back: Snapshot = serde_json::from_str(&json).unwrap(); @@ -3055,6 +3130,7 @@ mod tests { parent_tag: Some("python-numpy".to_string()), parent_content_hash: Some("a".repeat(64)), rootfs: None, + rootfs_read_only: None, }; let json = serde_json::to_string(&s).unwrap(); let back: Snapshot = serde_json::from_str(&json).unwrap(); From 62179072eb08801d44e5cecb63405509cc4cc83a Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 11 Sep 2026 15:09:44 -0700 Subject: [PATCH 2/2] fix(controller): the refusal message should not tell operators to set the flag it is already honouring Signed-off-by: jrimmer --- crates/forkd-controller/src/http.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/forkd-controller/src/http.rs b/crates/forkd-controller/src/http.rs index 9d34b169..3d9f5bbc 100644 --- a/crates/forkd-controller/src/http.rs +++ b/crates/forkd-controller/src/http.rs @@ -938,7 +938,7 @@ fn shared_writable_rootfs_conflict( "snapshot `{tag}` was baked with a WRITABLE rootfs, so every restored child reopens the \ same ext4 read-write and concurrent children corrupt each other's files. {} live \ sandbox(es) already hold it ({}). Re-bake the snapshot read-only, or restore one child \ - at a time; set FORKD_REFUSE_SHARED_RW=1 to make this a hard error.", + at a time.", holders.len(), holders.join(", ") )) @@ -1234,7 +1234,10 @@ async fn create_sandbox( if refuse_shared_writable_rootfs() { return conflict(&reason); } - tracing::warn!(snapshot = %req.snapshot_tag, "{reason}"); + tracing::warn!( + snapshot = %req.snapshot_tag, + "{reason} Set FORKD_REFUSE_SHARED_RW=1 to refuse instead." + ); } // v0.5: if the loaded snapshot has parent_tag set, this is a