diff --git a/CHANGELOG.md b/CHANGELOG.md index 0059420..d0db86c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,49 @@ Versioning](https://semver.org/spec/v2.0.0.html) once it reaches ## Unreleased +### Upgrade note: Firecracker must be 1.15 or newer, and snapshots need re-baking + +Concurrent children of one snapshot now get their own rootfs backing (below), +which uses `PATCH /drives/{drive_id}` on a restored VM. That call is only +honoured from **Firecracker 1.15**; tested against **v1.17.0**. On older builds +the API accepts the request but the device keeps writing the original file, so +children would silently share the rootfs again — hence a hard minimum rather +than a warning. + +Because a vmstate is version-pinned, **upgrading Firecracker invalidates every +snapshot**: a new build refuses to load a vmstate written by an older one +(`Failed to load snapshot state from file`). Upgrading therefore means +re-baking every tag before its pool can serve again. + +Nothing else about the vendored fork changes: its MAP_SHARED patch is needed +only by `--live`, so a deployment that does not use live-fork can run stock +Firecracker. See docs/VENDORED-FIRECRACKER.md. + +### Per-child rootfs backings: concurrent children can no longer corrupt each other + +A snapshot's rootfs is one ext4 and Firecracker reopens that path verbatim for +every child, so children of a tag shared a filesystem. With the drive opened +read-write — how every tag was baked — two concurrent children were two guest +kernels writing one filesystem with no coordinator: package files picking up +other files' bytes, `EBADMSG` on `/var/lib/dpkg` entries, damage that reads as a +random build failure rather than a sandbox error. + +Each child now gets its own reflink clone of the tag's rootfs and is re-pointed +at it before it runs. The ordering is load-bearing: the child is loaded +`resume_vm: false`, re-pointed, and only then resumed, so no boot-time write +(journal replay, `/var/log`, agent startup) can reach the shared base first. +Measured on v1.17.0, the base stays byte-identical through boot while the +backing changes. + +A child whose rootfs cannot be re-pointed is **not** resumed — it would run +against the shared base — so the failure surfaces instead of drifting into +corruption. Backings live in the child's work dir and are reclaimed with it. + +Constraints worth knowing: a reflink clone is free only where the filesystem +supports it (XFS/btrfs, or ZFS 2.2+ block cloning); elsewhere it is a full copy. +Snapshots with no recorded rootfs (daemon-side branches inherit the source's) +keep the previous single-call behaviour. + ### Upgrade note: legacy sandbox rows block startup The controller now persists a boot identity (start time + boot id) for every diff --git a/crates/forkd-controller/src/http.rs b/crates/forkd-controller/src/http.rs index 1b55999..a66154a 100644 --- a/crates/forkd-controller/src/http.rs +++ b/crates/forkd-controller/src/http.rs @@ -482,11 +482,22 @@ async fn create_snapshot( // Volumes via daemon snapshot API will land in a follow-up commit; // for now snapshots created through the daemon are volume-less. // Use the CLI's `forkd snapshot --volume` for tag-shared caches. - let snap = vm.snapshot_to( + let mut snap = vm.snapshot_to( snap_dir_for_task.join("vmstate"), snap_dir_for_task.join("memory.bin"), Vec::new(), )?; + // Record the rootfs this parent booted from. Restore gives every child + // its own reflink backing cloned from this path, and with no path there + // is nothing to clone, so children fall back to sharing one file — the + // daemon's snapshots were the last ones doing that. Daemon-created + // branches inherit the field from their head snapshot, so this single + // site covers them as well. + snap.rootfs = Some( + cfg.rootfs + .canonicalize() + .unwrap_or_else(|_| cfg.rootfs.clone()), + ); // Persist Snapshot metadata so subsequent forks read back the same // (possibly volume-bearing) snapshot description. let meta = serde_json::to_vec_pretty(&snap)?; diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index b1997ac..fe02521 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -322,7 +322,128 @@ impl Registry { /// If some kills failed, those entries remain in the registry and /// the allocator may still collide with them — the caller should /// log the failure count and the operator should investigate. + /// The work dir a Firecracker process was started with, read from its own + /// `--api-sock` argument. `None` when the process is already gone or the + /// argument is unreadable — callers skip the reclaim rather than guess a + /// path, since removing the wrong directory would take a live VM's sockets + /// with it. + fn child_work_dir_from_cmdline(pid: u32) -> Option { + let raw = std::fs::read(format!("/proc/{pid}/cmdline")).ok()?; + let args: Vec = raw + .split(|b| *b == 0) + .filter(|s| !s.is_empty()) + .map(|s| String::from_utf8_lossy(s).into_owned()) + .collect(); + let sock = args + .iter() + .position(|a| a == "--api-sock") + .and_then(|i| args.get(i + 1))?; + Some(std::path::Path::new(sock).parent()?.to_path_buf()) + } + + /// Is `name` a restored child's rootfs backing (`child-.`)? + /// + /// The same dir also holds `child-.sock` and `child-.console`, and the + /// stem is the tag's rootfs filename — not always `rootfs.ext4` — so match + /// on shape rather than a fixed suffix. + fn is_child_backing(name: &str) -> bool { + let Some(rest) = name.strip_prefix("child-") else { + return false; + }; + let Some((index, ext)) = rest.split_once('.') else { + return false; + }; + !index.is_empty() + && index.chars().all(|c| c.is_ascii_digit()) + && !ext.is_empty() + && ext != "sock" + && ext != "console" + } + + /// Reclaim the rootfs backings a reaped child left behind. + /// + /// A child killed while the controller is up has its backing removed by + /// `Vm::drop`. This covers the other order — controller crash, then startup + /// reap — where nothing else collects them: the watchdog leaves the dir + /// alone because the socket is present, and the socket is present because + /// the orphan held it until now. Only backings in a directory belonging to + /// a process this reap has just confirmed dead are touched, so a live VM + /// cannot be caught; the dir itself is left to the watchdog's own sweep. + fn reclaim_child_backings(work_dir: &std::path::Path) { + let Ok(entries) = std::fs::read_dir(work_dir) else { + return; + }; + for entry in entries.flatten() { + let name = entry.file_name().to_string_lossy().into_owned(); + if Self::is_child_backing(&name) { + let _ = std::fs::remove_file(entry.path()); + } + } + } + + /// Remove backings left in directories no live Firecracker is using. + /// + /// Covers what the per-kill reclaim cannot: a row retired *without* a kill + /// because its process is already gone — the ordinary shape of a controller + /// restart, where systemd kills the unit's cgroup and takes the children with + /// it. Nothing reaches `Vm::drop` in that ordering, so nothing removes the + /// backing, and the watchdog leaves the directory alone because the socket + /// file is still there. + /// + /// The gate is process ownership, not file presence. A directory is skipped + /// while any live Firecracker's `--api-sock` points inside it, so a running + /// VM's backing cannot be caught; and if any live Firecracker cannot be read, + /// nothing is removed at all, because ownership can no longer be established. + /// Leaking a backing costs disk. Deleting a live VM's backing destroys that + /// VM's disk mid-job, so every ambiguity resolves toward keeping it. + fn sweep_stranded_backings() { + let mut owned = std::collections::HashSet::new(); + let Ok(pids) = std::fs::read_dir("/proc") else { + return; + }; + for entry in pids.flatten() { + let Ok(pid) = entry.file_name().to_string_lossy().parse::() else { + continue; + }; + let Ok(raw) = std::fs::read(format!("/proc/{pid}/cmdline")) else { + continue; + }; + if !raw.windows(12).any(|w| w == b"firecracker\0") { + continue; + } + match Self::child_work_dir_from_cmdline(pid) { + Some(dir) => { + owned.insert(dir); + } + // A live Firecracker whose directory we cannot read means we + // cannot say what is owned, so nothing is swept this time. + None => return, + } + } + let Ok(entries) = std::fs::read_dir(std::env::temp_dir()) else { + return; + }; + for entry in entries.flatten() { + if !entry.path().is_dir() { + continue; + } + let name = entry.file_name().to_string_lossy().into_owned(); + if !name.starts_with("forkd-daemon-") { + continue; + } + let dir = entry.path(); + if !owned.contains(&dir) { + Self::reclaim_child_backings(&dir); + } + } + } + pub(crate) fn kill_orphans(&self) -> Result { + // Sweep before the reap: in the restart case the children are already + // dead, so their rows are retired without a kill and `Vm::drop` never + // runs for them. Their directories are unowned by now, which is exactly + // what the sweep requires. + Self::sweep_stranded_backings(); // One collected orphan: (sandbox id, pid, recorded start time, // recorded boot id). type OrphanRow = (String, u32, Option, Option); @@ -471,6 +592,10 @@ impl Registry { pid = pid, "killing orphaned Firecracker process on startup (pidfd signal, identity verified)" ); + // Read the child's work dir now: once the process is gone + // there is nothing left in /proc to read it from, and the + // rootfs backing inside would be stranded for good. + let orphan_work_dir = Self::child_work_dir_from_cmdline(pid); match pidfd_send_kill(pidfd) { Ok(()) => { // Wait for the process to actually exit (bounded). @@ -478,6 +603,9 @@ impl Registry { // hold netns/tap resources past the kill return. if wait_for_death(pid, std::time::Duration::from_secs(5)) { self.inner.lock().sandboxes.remove(&id); + if let Some(dir) = &orphan_work_dir { + Self::reclaim_child_backings(dir); + } killed += 1; } else { tracing::error!( @@ -1138,6 +1266,53 @@ mod tests { ); } + /// The predicate decides what a startup reap deletes, and the work dir holds + /// sockets and consoles under the same `child-.` shape. The stem is the + /// tag's rootfs filename, so it is not always `rootfs.ext4`. + #[test] + fn child_backing_predicate_matches_backings_only() { + assert!(Registry::is_child_backing("child-1.rootfs.ext4")); + assert!(Registry::is_child_backing("child-12.python-3-12-slim.ext4")); + assert!( + !Registry::is_child_backing("child-1.sock"), + "sockets share the child-. shape and must survive the sweep" + ); + assert!( + !Registry::is_child_backing("child-1.console"), + "consoles share the child-. shape and must survive the sweep" + ); + assert!(!Registry::is_child_backing("memory-assembled.bin")); + assert!(!Registry::is_child_backing("child.rootfs.ext4")); + assert!(!Registry::is_child_backing("child-x.rootfs.ext4")); + assert!(!Registry::is_child_backing("child-1.")); + assert!(!Registry::is_child_backing("rootfs.ext4")); + + // Shape, not suffix: the comparison is against everything after the + // first dot, so these match and the sweep would delete them. Nothing + // creates such a file — the assertions are here so a reader knows the + // predicate is deliberately approximate rather than exact. + assert!(Registry::is_child_backing("child-1.sock.bak")); + assert!(Registry::is_child_backing("child-1.SOCK")); + + // The stem is the tag's rootfs filename, so a multi-dot or non-ext4 stem + // must keep matching: tightening this to `.ext4` would look like an + // improvement and would silently stop collecting those backings. + assert!(Registry::is_child_backing("child-1.tar.gz")); + assert!(Registry::is_child_backing("child-1.rootfs.squashfs")); + + // The index is never parsed, so leading zeros and absurd values pass. + assert!(Registry::is_child_backing("child-0.rootfs.ext4")); + assert!(Registry::is_child_backing("child-01.rootfs.ext4")); + assert!(Registry::is_child_backing("child-4294967296.rootfs.ext4")); + + // ASCII digits only. Generalising to `is_numeric()` would accept unicode + // digits and start deleting names we never created. + assert!(!Registry::is_child_backing("child-1\u{662}.rootfs.ext4")); + // Callers pass a file name, not a path. + assert!(!Registry::is_child_backing("child-1/rootfs.ext4")); + assert!(!Registry::is_child_backing("")); + } + #[test] #[cfg(target_os = "linux")] // PidReuse path requires /proc starttime comparison fn kill_orphans_prunes_alive_pid_entries() { diff --git a/crates/forkd-vmm/src/lib.rs b/crates/forkd-vmm/src/lib.rs index e94a382..53d1157 100644 --- a/crates/forkd-vmm/src/lib.rs +++ b/crates/forkd-vmm/src/lib.rs @@ -266,6 +266,12 @@ pub struct Vm { /// Drop. Phase 6's UFFD_WP arming will dup this fd to register a /// `userfaultfd` against the same VMA. pub memfd: Option, + /// This child's own writable rootfs backing, when it was restored from a + /// snapshot that recorded a rootfs (see `child_rootfs_backing`). Owned + /// here so it dies with the child: work dirs are not reclaimed on their + /// own, so a backing left inside one is a full rootfs copy that nothing + /// would ever collect. `None` for VMs booted directly. + backing: Option, } /// Accept one connection on an already-non-blocking `UnixListener`, @@ -1440,6 +1446,66 @@ const CHILD_OOM_SCORE_ADJ: i32 = 500; /// to an attacker-pre-created file under `/tmp/`. const VMSTATE_ONLY_MEM_PLACEHOLDER: &str = "/dev/null/forkd-vmstate-only-mem-ignored"; +/// Where a restored child's own rootfs backing lives. +/// +/// A snapshot's rootfs is one file, and Firecracker reopens it verbatim for +/// every child, so without a per-child copy two concurrent children write one +/// filesystem with no coordinator. The backing is a reflink clone of the +/// snapshot's rootfs (see `chain::reflink_copy`), which costs nothing where the +/// filesystem supports it. +/// +/// It lives in the child's work dir so that removing the dir reclaims it. A +/// crash that skips that cleanup leaves a straggler beside the sockets, which is +/// the same class the startup sweep already collects. +fn child_rootfs_backing(work_dir: &Path, child_index: usize, tag_rootfs: &Path) -> PathBuf { + let stem = tag_rootfs + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or("rootfs.ext4"); + work_dir.join(format!("child-{child_index}.{stem}")) +} + +/// Load a restored child **paused**, re-point its rootfs at `backing`, then +/// resume it. +/// +/// The order is load-bearing and was verified against Firecracker v1.17: +/// loading with `resume_vm: false` means the guest has not executed a single +/// instruction, so no boot-time write (journal replay, `/var/log`, agent +/// startup) can reach the shared base before the drive is re-pointed. Resuming +/// afterwards hands the guest a disk that is already its own — measured: the +/// base stays byte-identical through boot while the backing changes. +/// +/// **Fail closed.** A child whose rootfs could not be re-pointed must not be +/// resumed: it would run against the shared base, which is the corruption this +/// exists to prevent. Any failure here propagates, and the caller drops the +/// child (which kills its Firecracker) rather than run it unsafely. +fn load_paused_rebind_rootfs_and_resume( + sock: &Path, + load_body: &str, + backing: &Path, +) -> Result<()> { + api_call(sock, "PUT", "/snapshot/load", load_body) + .context("load child paused (resume_vm=false)")?; + // `drive_id` is required in the body — Firecracker rejects the override + // without it, which reads as a body error rather than a capability error. + api_call( + sock, + "PATCH", + "/drives/rootfs", + &serde_json::json!({"drive_id": "rootfs", "path_on_host": backing}).to_string(), + ) + .with_context(|| { + format!( + "re-point child rootfs at its own backing {} — refusing to resume a child \ + that would write the shared snapshot rootfs", + backing.display() + ) + })?; + api_call(sock, "PATCH", "/vm", r#"{"state":"Resumed"}"#) + .context("resume child after re-pointing its rootfs")?; + Ok(()) +} + fn api_call(sock: &Path, method: &str, path: &str, body: &str) -> Result<()> { api_call_with_timeout(sock, method, path, body, DEFAULT_API_TIMEOUT_SECS) } @@ -1753,6 +1819,7 @@ impl Vm { netns: None, cgroup: None, memfd: None, + backing: None, }) } @@ -2077,6 +2144,11 @@ impl Drop for Vm { if let Some(cg) = &self.cgroup { cgroup::cleanup(cg); } + // Reclaim this child's writable rootfs copy. Work dirs outlive their + // VMs, so leaving it to the dir would leak a full rootfs per spawn. + if let Some(backing) = self.backing.take() { + let _ = std::fs::remove_file(&backing); + } } } @@ -2172,6 +2244,9 @@ impl Snapshot { netns, cgroup: None, memfd: None, + // Set by the backing pass below, once the child + // exists to own it. + backing: None, })); } Err(e) => { @@ -2316,10 +2391,49 @@ impl Snapshot { } } + // Give each child its own rootfs backing before anything is loaded, and + // fail the child — not the batch — if one cannot be materialised. The + // snapshot's rootfs is shared by every child of this tag, so a child + // that runs without its own copy is exactly the corruption this exists + // to prevent. Snapshots with no recorded rootfs (daemon-side branches + // inherit the source's) keep the previous behaviour. + let mut backings: Vec> = vec![None; children.len()]; + if let Some(tag_rootfs) = self.rootfs.clone() { + for (i, slot) in children.iter_mut().enumerate() { + let Some(c) = slot else { continue }; + let pid = c.pid; + let backing = child_rootfs_backing(work_dir, i + 1, &tag_rootfs); + match crate::chain::reflink_copy(&tag_rootfs, &backing) { + Ok(_) => { + // The child owns it from here: Drop removes it, so a + // kill reclaims the copy instead of leaving a full + // rootfs in a work dir nothing sweeps. + c.backing = Some(backing.clone()); + backings[i] = Some(backing); + } + Err(e) => { + failures.push(RestoreFailure { + child_index: i + 1, + phase: RestorePhase::Restore, + pid: Some(pid), + error: format!("child rootfs backing {}: {e:#}", backing.display()), + }); + // Dropping the child kills its firecracker process. + *slot = None; + } + } + } + } + // Phase 2: parallel restore via threads. Each thread issues one // /snapshot/load PUT to its child's API socket. Body varies per // child only under MemfdShared (each child has its own memfd // path); for the File path, all children share the same JSON. + // + // With a backing the child is loaded PAUSED, re-pointed at that + // backing, and only then resumed, so no boot-time write can reach the + // shared base. Without one (no recorded rootfs) the previous + // single-call behaviour is preserved. let restore_start = Instant::now(); let mut handles: Vec<(usize, u32, thread::JoinHandle>)> = Vec::new(); for (i, slot) in children.iter().enumerate() { @@ -2328,6 +2442,8 @@ impl Snapshot { }; let sock = c.sock.clone(); let pid = c.pid; + let backing = backings[i].clone(); + let resume_on_load = backing.is_none(); let body = match &c.memfd { Some(region) => serde_json::json!({ "snapshot_path": &self.vmstate, @@ -2337,7 +2453,7 @@ impl Snapshot { "shared": true, }, "enable_diff_snapshots": opts.enable_diff_snapshots, - "resume_vm": true, + "resume_vm": resume_on_load, }) .to_string(), None => serde_json::json!({ @@ -2347,7 +2463,7 @@ impl Snapshot { "backend_type": "File", }, "enable_diff_snapshots": opts.enable_diff_snapshots, - "resume_vm": true, + "resume_vm": resume_on_load, }) .to_string(), }; @@ -2355,7 +2471,12 @@ impl Snapshot { i, pid, thread::spawn(move || -> Result<()> { - api_call(&sock, "PUT", "/snapshot/load", &body) + match backing { + Some(backing) => { + load_paused_rebind_rootfs_and_resume(&sock, &body, &backing) + } + None => api_call(&sock, "PUT", "/snapshot/load", &body), + } }), )); } diff --git a/docs/VENDORED-FIRECRACKER.md b/docs/VENDORED-FIRECRACKER.md index 0bcaa57..e49b97e 100644 --- a/docs/VENDORED-FIRECRACKER.md +++ b/docs/VENDORED-FIRECRACKER.md @@ -10,7 +10,11 @@ The fix is a ~44-line patch adding an opt-in `shared: bool` field to `MemBackend ## Where the fork lives -[**`deeplethe/firecracker`**](https://github.com/deeplethe/firecracker), branch [`forkd-v0.4-mem-backend-shared-v1.12`](https://github.com/deeplethe/firecracker/tree/forkd-v0.4-mem-backend-shared-v1.12) (matches the FC version forkd produces snapshots with). +[**`deeplethe/firecracker`**](https://github.com/deeplethe/firecracker), branch [`forkd-v0.4-mem-backend-shared-v1.12`](https://github.com/deeplethe/firecracker/tree/forkd-v0.4-mem-backend-shared-v1.12) (matches the version that branch was forked from). + +**The fork is needed only for `--live`.** It exists to carry the opt-in `shared: bool` on `MemBackendConfig`, and `forkd-vmm` sends `shared` in exactly one place — the memfd path live-fork uses. Every other restore path, and the whole bake/restore/branch/exec surface, runs on **stock Firecracker**. The tested baseline is **v1.17.0**, and the minimum for per-child rootfs backings (which rely on `PATCH /drives` on a restored VM) is **v1.15**. So if you do not use live-fork, run upstream Firecracker and ignore this document. + +Two things to know before upgrading the version: a vmstate is version-pinned, so a new build refuses to load snapshots written by an older one — an upgrade means re-baking every tag; and the API accepts `PATCH /drives` on builds older than 1.15 but does not move the device's storage, which is why the minimum is hard rather than advisory. - Forked from upstream `v1.12.0` tag. - Five commits on the branch (combined ~140 lines, all forkd-specific feature work):