Confine the Firecracker VMM with the jailer - #960
Draft
epompeii wants to merge 145 commits into
Draft
Conversation
The jailer ships in the same release archive as Firecracker, so extracting both entries from a single download keeps the existing SHA256 pin as the only trust anchor and adds no new URL. Bundling them together also guarantees the VMM and its jailer cannot drift apart across a runner self-update, since both are re-extracted from the same binary. Adds a BENCHER_JAILER_PATH build-time override to match the existing BENCHER_FIRECRACKER_PATH, so a debug build that supplies its own Firecracker is not left without a jailer.
The jail needs somewhere to live that outlives a single job: every per-job directory today is a tempfile::TempDir, and the chroot base, the sweep, and the network namespace handle all need a persistent location. --state-dir defaults to /var/lib/bencher-runner and is created at mode 0700 owned by root, since it holds every job's chroot and therefore the guest rootfs. Both entry points that can reach the VM executor call one idempotent prepare_host(): the daemon has a startup hook and the one-shot CLI does not, so the work lives in the shared function rather than in daemon startup. The sweep reclaims chroots left by a runner that exited without unwinding. Jobs run serially, so anything found is stale by construction, and the runner disappears without unwinding in several ordinary ways: SIGKILL, a crash, and the exec in a self-update. Drop runs in none of them, and each leftover chroot holds a copy of the VMM binary and a full rootfs image. The network namespace is for the VMM process, not the guest. A compromised VMM with host network access can exfiltrate; an empty namespace removes that reach. vsock is unaffected, since its host side is filesystem-scoped Unix domain sockets. The namespace is unshared on a dedicated thread rather than in the runner: namespaces are per-task, so only that thread moves, and the bind mount pins the namespace once the thread exits. /proc/thread-self is required there, because /proc/self resolves through the thread group leader and would pin the host network instead.
The runner spawns the jailer instead of Firecracker. The jailer builds a chroot, creates /dev/kvm, drops to a dedicated unprivileged uid and gid, joins the empty network namespace, and execs Firecracker in place. Managed runners execute arbitrary code submitted by anyone, and until now a VMM escape landed as root on the runner host holding the runner key, every prior job's work directory, and write access to the self-updating runner binary. The VM id is minted before the job's artifacts, because the jail root is a function of it and rootfs.ext4 and vmlinux are now built directly inside the chroot rather than copied in. That is legal because the jailer uses create_dir_all for the chroot and does nothing if the path already exists. The artifacts leaving the workspace temp directory forfeits its RAII cleanup, so the jail guard takes over that responsibility and removes the tree on completion, timeout, cancellation, and every error return. Paths handed to Firecracker now resolve inside the chroot while the runner reaches the same files from outside, so the two views are separate types. Passing a host path where the API expects a chroot path is a compile error rather than a boot that hangs on a socket that never appears. Cgroup placement moves before exec. Membership is inherited across fork and survives execve, so a pre-opened cgroup.procs written from pre_exec places the child before it execs the jailer, and Firecracker inherits it through the jailer's own exec. This also fixes a second defect: the cpuset used to be applied after the VMM was already running, so Firecracker booted its API and touched memory on the wrong cores before being moved. No cgroup flags are passed to the jailer. The runner has to create, verify, read metrics from, and remove the cgroup, and the cpuset partition needs read-back verification because the kernel accepts the write and reports rejection inline. The jailer's write-once interface cannot provide that. Neither --daemonize nor --new-pid-ns is passed either: both make the jailer fork, which would break the pid identity the process management relies on. Confinement failures are fatal, so untrusted code never runs with silently degraded confinement. The cgroup keeps its existing degrade behavior at the edges, since a host that cannot isolate is a declared limitation, but when the cgroup does exist placement and verification are hard requirements: a cgroup that does not contain the VMM is a silent lie about where the benchmark ran.
Unit coverage for the pieces that can be exercised without KVM: both path views and their round trip, the chroot layout against the jailer's documented template, the sweep removing stale jails while leaving unrelated entries alone, and that placement and verification are skipped rather than failed when no cgroup exists. The negative cases are covered too: an unbuildable chroot is an error rather than a warning, and a cgroup that does not contain the VMM aborts. The integration scenarios extend the existing KVM-gated runner harness rather than adding a second one. Two invariants only exist while the VMM is alive and cannot be recovered from the runner's output afterwards, so scenarios gain an optional host-side probe: it finds the VMM by its root directory, which the jailer chroots before exec, then checks that it dropped root to the user its jail was handed to and that it is already in its cgroup. Placement happens before the exec, so membership holds the first time the process is observable. Teardown is checked after completion and after cancellation, since the jailer cleans up nothing and each leftover chroot holds a VMM binary and a full rootfs image. Scenarios now run against their own state directory, so jail assertions are scoped to the scenario and never touch a real runner's state.
The jailer unshares a mount namespace and pivot_roots onto a bind mount of the chroot before exec, so the confined process's root path reads back as `/` from the host and cannot identify it. The bind mount preserves the device and inode of the chroot directory, so comparing those through /proc/<pid>/root picks out exactly the VMM confined to a given jail. Also corrects two comments against the jailer's actual behavior: it does set the mode of the chroot root even when the directory already exists, and its hard link check is on the destination inside the chroot rather than on the source that is copied in.
build_config_from_job never passed the runner's state directory through, so runner up --state-dir prepared and swept one directory while every job built its chroot under the default. The sweep guarded a location that never held a jail, so a SIGKILL, a crash, or a self-update exec leaked a full guest rootfs permanently, and the tree that did hold jails was created by create_dir_all at 0755 rather than the documented 0700 owned by root. The up config is now destructured rather than read field by field. The bug was not that the default was wrong, it was that a builder omission was invisible: Config::new supplies the documented default, so forgetting a with_* call reads as working code. Destructuring makes the omission a build error, which is the same reason the codebase prefers it elsewhere. Left the serde attribute alone deliberately: Config is never deserialized on either job path, so the serde default was not what hid this.
Bind mounting over a file does not report EBUSY, so mounts stack. Against a handle carrying two of them the single detach removed only the top one, the unlink then failed with EBUSY and the error was discarded, and File::create on the surviving nsfs mount failed with EPERM even as root. ensure() then failed permanently: runner up refused to start and every sandboxed runner run failed, until an operator looped umount by hand. Verified on a real kernel. With two mounts stacked, one detach leaves one mount, the unlink reports 'Device or resource busy' and the create reports 'Operation not permitted'; unwinding in a loop leaves none, and both the unlink and the create then succeed. The loop is bounded, since a path that reports a successful unmount forever is a kernel fault and the unlink that follows reports the real state either way. The unlink error is no longer discarded: a handle that cannot be cleared is a confinement failure, not something to paper over with a create that fails more confusingly. Note on the namespace creation this guards: the plan called for forking a child that unshares, and this uses a dedicated thread instead. Namespaces are per task, so unsharing on a thread moves only that thread and leaves the runner on the host network, while avoiding fork in a process that has threads, where only async-signal-safe work is permitted before exec. That is why /proc/thread-self is required rather than /proc/self, which resolves through the thread group leader and would pin the host namespace.
The sweep removes every chroot it finds, on the reasoning that jobs are serial so anything left is stale. That reasoning was an assumption, not a constraint: a one-shot runner run started while the daemon had a job in flight would remove_dir_all the live chroot out from under a running VMM. Both paths now resolve to the same state directory, so nothing kept them apart. An advisory flock on <state_dir>/.lock is held across prepare_host and for the life of a job. It is declared before the jail guard so it outlives the teardown it protects, and the kernel releases it if the holder dies, so a crashed runner cannot wedge future runs. The same lock closes the race where two processes clearing and rebinding the network namespace handle at once stack mounts on it. Unlike the host tuning lock, which degrades to skipping tuning when contended, this one waits: a runner that proceeded without it would destroy another runner's work, so declining to hold it is not an option. It tries once without blocking first so that waiting is announced rather than looking like a hang. The lock file sits beside the chroot base rather than inside it, so the sweep can never reach it.
The command line was built inline in the spawn and nothing covered it, so only a live KVM boot would catch a regression. Forwarding --id after the separator is the sharpest case: the jailer already passes it to Firecracker, which rejects the duplicate and fails every job at startup, with an error that points at Firecracker rather than at the command line that caused it. Extracted so it can be asserted anywhere: --id present exactly once, the --api-sock value carrying the chroot view with no host jail path anywhere in the vector, the uid, gid, chroot base and netns flags all present, the separator, nothing after it but --api-sock and --level, and none of the cgroup or forking flags the design deliberately omits. The spawn destructures the remaining fields rather than reading them, so adding one without deciding what it does on the command line is a build error.
Self-hosted runners land on customer hardware whose id allocation Bencher does not control. A local process owning the jail uid can signal the VMM and, depending on the ptrace scope, trace it, so an operator whose host already allocates in this range needs a way out. --jail-uid and --jail-gid go through both entry points the way --state-dir does. The default is 61016, Bencher's historic default self-hosted API server port, retired in favor of the IANA-registered 6610. It reads as a project convention rather than an arbitrary pick, and still lands in the unallocated gap between the ids systemd-homed claims (60001-60513) and the DynamicUser range (61184-65519). prepare_host warns when the configured id resolves to a named account. The jailer needs no passwd entry, so a name resolving there is the cheap signal that the host allocates in this range. It reads /etc/passwd and /etc/group directly rather than calling getpwuid: the runner ships as a self-contained binary and a local account is exactly what matters. A warning rather than a refusal, since an operator who deliberately created the account is a legitimate setup and only they can tell the two apart.
The scenario job ran cargo test-runner scenarios without sudo. Verified on a real kernel that this cannot work now that the sandbox is jailed: run unprivileged, the jailer fails at ChangeFileOwner with EPERM and exits 1, leaving a half-built chroot, and prepare_host does not even get that far since creating the network namespace directory is denied and unshare is not permitted. Run as root the same command builds the chroot at 0700, populates its device nodes, and leaves Firecracker running as the jail uid. The udev rule that makes /dev/kvm world accessible is enough to use KVM unprivileged but not to build the jail around it: the design drops privilege rather than starting without it. The scenario run is elevated on its own rather than making the whole job root. A --build-only mode builds the binaries as the CI user, and the elevated step runs the built harness directly with BENCHER_RUNNER_BIN, so cargo never runs as root and neither the target directory nor cargo's cache is left root-owned. Verified: zero root-owned files under the target directory after the elevated run. The harness now refuses to run unprivileged with a message naming both steps, rather than failing partway through the first scenario at a mknod. It also honors CARGO_TARGET_DIR, which it previously ignored, so a redirected build does not report success and then a missing binary.
Contributor
🤖 Claude Code ReviewPR: #960 Review: Firecracker jailer confinement for the Bare Metal runnerScope: ~12k lines across I read the production code in full and sampled the harness. I could not run OverallThis is unusually careful work. The failure-policy table in Findings1.
if let Err(e) = download_and_extract_tgz(&url, &wanted, Some(expected_hash)) {
eprintln!("WARNING: Failed to download/extract the Firecracker release: {e}");
}
let resolved = |overridden, name| overridden.or_else(|| {
let dest = cached_binary(out_dir, name, arch, expected_hash);
dest.exists().then_some(dest) // <- picks up the truncated file
});A truncated 2. Losing the cpuset now also loses cgroup metrics and swap control ( Previously a failed 3. The doc comment is explicit that this is deliberate and a reasonable follow-up, and the test that claims the released fd number is a good guard. I agree with the tradeoff, but flag it as the largest residual design risk in the PR: a future reordering that drops 4. Known races, all documented, worth restating for the record
5. Doc-comment density Against the repo's "Less is more. KISS." axiom, several modules run 3:1 or worse prose to code, and a lot of it is history rather than specification: "which is what once let a populated system directory pass the guard", "the two used to disagree", "Reasoning missed instances three times running". That belongs in commit messages (where it already is, across 130 commits). The failure-policy table and the security rationale on Smaller notes
Standards complianceChecked against Not verified, please confirm locally: Model: claude-opus-5 |
Contributor
|
Bencher Report| Project | Bencher |
| Branch | runner-jailer |
| Testbed | intel-v1 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result microseconds (µs) (Result Δ%) | Upper Boundary microseconds (µs) (Limit %) |
|---|---|---|---|
| Adapter::Json | 📈 view plot 🚷 view threshold | 4.66 µs(-0.46%)Baseline: 4.68 µs | 5.00 µs (93.17%) |
| Adapter::Magic (JSON) | 📈 view plot 🚷 view threshold | 4.47 µs(-1.53%)Baseline: 4.54 µs | 4.81 µs (93.05%) |
| Adapter::Magic (Rust) | 📈 view plot 🚷 view threshold | 25.51 µs(-0.88%)Baseline: 25.74 µs | 26.98 µs (94.56%) |
| Adapter::Rust | 📈 view plot 🚷 view threshold | 3.51 µs(-0.66%)Baseline: 3.53 µs | 3.93 µs (89.24%) |
| Adapter::RustBench | 📈 view plot 🚷 view threshold | 3.51 µs(-0.57%)Baseline: 3.53 µs | 3.93 µs (89.23%) |
Every scenario but the two new ones failed in CI with a five second timeout that read as Firecracker's fault. It was the path. sockaddr_un.sun_path is 108 bytes and the limit applies to the string handed to bind and connect, before any resolution, so the host view of a jail under a deep state directory blows it at 144 bytes while the chroot view Firecracker uses stays short. The default state directory happens to fit at 91, which is 17 bytes of headroom on a limit nobody had declared. Socket paths are now a third view, built from a descriptor the runner holds open on the chroot: /proc/self/fd/<n>/api.sock is about thirty bytes and names the same inode however deep the jail is. The descriptor is O_PATH, held for the life of the job, and its lifetime is enforced by ownership rather than by discipline, because a closed and reused number would silently address a different directory. SocketPath checks every value against the limit at construction and names the limit, the length, and the offending path, so this can never again surface as a mystery timeout. The timeout was also swallowing the real error. An over-long path is rejected by the standard library before any syscall, and the readiness loop retried that for the full five seconds. It now retries only what a not-yet-listening VMM actually produces and fails immediately on anything describing the address itself.
Both new scenarios reported PASSED in CI while every other sandboxed scenario failed to boot a VM at all. The confinement probe checks the VMM's uid, its cgroup, and its root inode, all of which hold whether or not the guest ever runs, so the scenario stayed green through a broken product. That is the worst failure mode a confinement test has. Both now assert the job succeeded, exit code and guest output, before asserting anything about confinement. jail_teardown_on_cancel is replaced by jail_sweep_reclaims_orphan, which tests the mechanism that actually covers exits that never unwind. SIGTERM to the one-shot runner takes the default disposition, since signal handlers are installed only by the daemon, so Drop never ran and the scenario was asserting teardown that could not have happened; it passed only because the run failed fast. The replacement kills the runner once its VMM is up, proves the chroot survived, and then proves the next job swept it. It reaps the orphaned VMM itself: the sweep reclaims the chroot but nothing reaps an orphaned VMM or its cgroup, so a stray Firecracker would otherwise burn benchmark cores for the rest of the suite. The confinement probe no longer treats a VMM caught mid-flight as a violation. The jailer pivot_roots before it drops privilege, so there is a window where the process root already matches the jail while the process is still root; that is now not-ready-yet, and the timeout is what catches a VMM that never drops. A cgroup that cannot be read says so out loud rather than passing silently. The scenarios also pass --no-tuning. Elevating them turned real host tuning on for all twenty-five: unprivileged every knob failed with EPERM and warned, but as root they apply, and offlining SMT siblings on a two-vCPU hosted runner would change the core count mid-suite.
runner up required root just to start. prepare_host ran at startup, and unprivileged it cannot create the state directory, cannot create /run/netns, and cannot unshare, so the daemon died after preflight and never reached polling. That broke the API smoke tests and contradicts documented behavior: a Runner serving only non-sandboxed Specs is a supported configuration, and the daemon cannot know its Specs at startup because it learns them from the server. Preparation now happens immediately before the first job that builds a jail, on both entry points, which also retires the special case the one-shot path carried for the same reason. It stays fatal, since a sandboxed job that cannot be confined must not run, and it is not remembered on failure, so a transient permission problem is retried by the next job rather than needing a restart. The sweep still runs before any jail exists in the process, which is what its purpose requires. Verified unprivileged on Linux: the daemon reaches Connecting to channel, and the state directory is not created. Ordering matters and is deliberate: preparation takes the jail lock and releases it before the job takes it. flock is per open file description, so nesting the two would block on itself; that is now spelled out on the lock. The sweep reports what it reclaimed. Each leftover held a VMM binary and a full guest rootfs image, and an operator never heard about any of it.
--jail-uid 0 was accepted and silently defeated the whole jail: the sandbox is built by dropping privilege, so a jail user of root is not a weaker jail but no jail at all, and untrusted code would run against a root VMM. It is a plausible typo and an even more plausible fix for an operator hitting a permission error. JailUser now validates and carries private fields, so 0 cannot reach the jailer through the flag, the environment variable, or the library. The flags carry a range parser as well. The network namespace is rebuilt rather than reused. Proving a handle is a namespace and is not the runner's own does not prove it is empty: a bencher-jail left by an operator experimenting with ip netns could hold interfaces, and the VMM would silently regain the host network reach this exists to remove. Recreating is cheaper and stronger than asserting a namespace holds nothing but a down lo. It is also rebuilt per job rather than once per lifetime, since the handle lives on a tmpfs and is operator visible, and it takes its own lock. The jail lock is scoped to a state directory while the namespace is process global, so two runners started with different --state-dir values held different locks and could still stack mounts on the same handle, which is the race the lock was added to close. StateDir::create refuses a root that already exists, is not empty, and carries nothing the runner put there. It applies 0700 on every call so an older runner's laxer directory is tightened, which pointed at --state-dir /var/lib would have chmodded that directory and taken the host down. The named-account warning no longer implies more than it delivers: it reads the local files, so it is blind to the LDAP, Active Directory, and SSSD hosts most likely to allocate in this range.
The check written to stop vacuous passes was itself vacuous in the scenario it was written to protect. ScenarioOutput captures the runner's stdout, not the guest's, and the runner prints "Launching jailed Firecracker microVM..." on its way to starting a VM. Matching on "jailed" therefore matched the runner announcing its intent, so the marker check passed while the guest never ran, leaving a non-zero exit code as the only real guard where two were designed. Both scenarios now use tokens the runner's own output cannot contain, following the convention already in this file. "swept" does not collide today but sits one refactor away from the sweep's own reporting.
The sweep reclaimed the chroot and left the more damaging half. A runner that is SIGKILLed, crashes, or execs itself during a self-update does not signal its jailed VMM, so the VMM is reparented and keeps running, holding the exclusive benchmark CPUs through a cgroup nothing removes. The consequence is not leakage, it is wrong numbers that look right. The next job's cpuset write is rejected by the cgroup still owning those CPUs, and that failure was swallowed twice over: apply_cpuset returned Ok on every internal failure and the caller only warned on top of it. Every subsequent run would report success while measuring somewhere other than where it claimed, until someone rebooted. A half-applied fidelity mechanism is a confinement-grade failure, so a cpuset that cannot be applied to a cgroup that exists now aborts the job. Failing to create the cgroup at all still degrades, because a declared absence of isolation is not a lie about it. Killing a process the runner does not own is a new destructive capability, so the target is identified as narrowly as it can be: only a process whose root directory is the chroot being swept, compared by device and inode. Verified on a real kernel against a live jailed VMM: exactly one of 125 processes matched. Not "any process owned by the jail uid", which on a shared host may legitimately own something else. The pid is pinned with a pidfd before the signal. A pid found by scanning /proc can exit and have its number recycled before the signal lands, and this runs as root. Holding the descriptor keeps the number from being reused, which turns the identity check into a guarantee rather than a narrow window. Ordering is forced: reap, then remove the tree, then remove the cgroup. Removing the tree first pulls the rootfs from under a process still running, and rmdir on a cgroup that still holds one fails. A cgroup that survives anyway is reported loudly, because a surviving isolated cpuset is exactly the silent degradation this exists to prevent.
Both cleanups named the socket view, which is a descriptor number. Unlinking has no sun_path limit, so that view bought nothing and cost a dependency on a descriptor still being open. Both run from Drop, where a future reordering could close it first, and where the failure would not be an error: the number is reused immediately, so the identical string resolves to a different directory and the unlink deletes whatever file inherited it. Reserving the socket view for bind and connect makes that unrepresentable, and a test pins it rather than leaving it a convention: drop the paths, claim the released number with another directory, and assert the same string no longer names the jail.
A sandboxed Job is a jailed Job, and the jailer needs root, so the smoke test's Firecracker runner could no longer come up as the unprivileged CI user. Only that one process is elevated. Cargo and everything else stay as the invoking user, and the no-sandbox runner in the same test stays unprivileged, which proves the coupling holds in both directions in a single run. The already-built binary is run under sudo directly rather than through cargo, so nothing root-owned lands in the target directory. Teardown signals the process group rather than the handle. Verified on Linux with sudo 1.9.15p5 that sudo forks rather than execing in place: the handle is sudo (pid N) and the runner is a separate process in the same group. Killing only the handle left a root runner daemon running, which would have held the jail lock for the rest of the test; killing the group leaves nothing. The kill is itself elevated, because the unprivileged test process cannot signal a root daemon. Missing passwordless sudo now fails immediately and says why, instead of surfacing thirty seconds later as a readiness timeout with nothing pointing at the cause.
A Runner executing sandboxed Jobs must run as root where before it did not, so it is called out where an operator actually looks. The self-hosted Runner intro said Firecracker sandboxing requires Linux with KVM enabled, which is now only half the requirement, and the start-the-Runner page showed bare runner up commands that read as unprivileged. Both are corrected in all nine locales. The changelog entry names the capabilities rather than asserting the requirement: mknod for the chroot's device nodes, chown to hand the guest images to the jail user, pivot_root, and setns to join the network namespace. A world-readable /dev/kvm is enough to use KVM unprivileged but not to build the jail around it, which is exactly the assumption an operator will have. It points anyone who cannot run as root at --danger-allow-no-sandbox while being explicit that this trades away the microVM itself and not just the jail.
Making a failed cpuset.cpus write fatal was too broad and would have broken hosts that worked before. enable_controllers falls back as far as +cpu +memory +pids, and only those three are required, so a host that does not delegate cpuset (a containerized runner, or a cgroup namespace without it in subtree_control) creates its cgroup successfully and then has no cpuset.cpus to write at all. Every sandboxed job on such a host would have started failing where it previously ran with a warning, and the doc comment claimed such hosts were handled earlier by not creating a cgroup, which is not what the code does. The line the spec actually draws is between an absent mechanism and a half-applied one. A controller that is not delegated is a declared absence of isolation: the cgroup is dropped and the job runs without one, exactly as on a host where the cgroup could not be created. A controller that is present and rejects the write is a cgroup claiming an isolation it does not have, which stays fatal. The local execution path takes the same distinction but keeps both branches best effort, since a non-sandboxed run makes no confinement claim to falsify.
A stale cgroup that could not be removed was only warned about, while the message itself said the leftover still holds the benchmark CPUs and the next run's isolation will be rejected. With that rejection now fatal, and with preparation latching on success, one unremovable cgroup meant every job in that daemon failed forever with no retry: recovery needed a restart and a manual rmdir. The module doc promised a failure is not remembered, and that promise was false. Removal now retries on a deadline, because rmdir fails while the cgroup still holds a process and the reap that precedes it may need a moment to land. If it still fails the sweep reports it, so preparation does not latch and the next job sweeps again rather than inheriting a host that can never isolate. A chroot that will not go away costs disk and stays a warning. The reap no longer fails silently. pidfd_open collapsed ENOSYS on kernels before 5.3, EPERM, and already-exited into one empty answer, so an orphan could be left holding the benchmark CPUs with nothing said. Already-exited is now distinguished from everything else, and everything else is reported. wait_for_exit also treats a zombie as exited. The orphan reparents to PID 1, and where the runner is itself PID 1 with no init to reap it, /proc/<pid> persists forever and every sweep would stall its full timeout and warn about a process that was already dead.
The latch was a static AtomicBool, which the repo rules prohibit outright. It also read and wrote as two separate operations, harmless only because JailLock happened to serialize every caller, and it made the laziness test depend on what else had run in the process: it passed under nextest, which gives each test its own process, and was flaky under plain cargo test. It is now an owned token created by the daemon loop and by the one-shot CLI, threaded to the executor. The latch belongs to one runner process, nothing else can observe or reset it, and each test gets its own. Verified in-process and single-threaded on Linux, which is the case a global would have broken. The VM identity gets a newtype. The same string is the jailer's --id, the chroot directory name, and the cgroup name, and remove_stale_cgroup took a bare &str read straight off a directory entry, which is exactly the confusion worth making impossible. Recovering an identity from a chroot name is now a named operation rather than an implicit conversion. Also renames copy_into_jail, which was called to stage the Firecracker binary outside the chroot and printed 'Copied ... into the jail at /tmp/...', which was simply false. The jail wording moves to the kernel call sites, which are the ones that actually copy into the chroot.
`rmdir` refuses while a cgroup still holds a process, and the SIGKILL the teardown sends lands before the reap does. A single un-retried attempt loses that race, and losing it strands the cgroup forever: the jail directory that names it is removed on the very next line, so nothing looks for it again. The same bounded five second wait the runner's own removal uses, and what outlasts it is named out loud, since no other teardown step reports it.
The two /proc readings bracket the runner's lifetime, so their difference sees a VMM still alive when the runner exits and nothing else. A VMM that launched and exited inside the run leaves it empty, and the marker assertion is what catches that, so say so rather than claiming no VMM launched at all.
…atus read The reap now polls a pidfd instead of reading /proc/<pid>/status, and an unanswerable poll fails the job rather than reading as a dead process, so the old row described a step that no longer exists with the opposite policy from the code that replaced it. Add rows for the two cgroup.procs reads the reap gained, and for the refused +cpuset enable write, which is the second way an absence of cpuset is now declared. This table is read as a specification.
The state directory guard tolerates the lock file's name in a root that holds nothing else, so the two modules must agree on that name. Reuse the constant the lock module owns rather than respelling the literal, and correct the lock module's comment, which still claimed the name was private, unique to it, and never counted by the guard: all three became false when the guard began tolerating it.
Three public items still linked private ones by intra-doc link, the pattern already delinked elsewhere in this branch. Plain backticks name them without promising a reader a link that resolves to nothing.
Two same-named make_private helpers take opposite stances on symlinks. The chroot's path form is safe because it runs under a state tree the guard has already proven free of symlinked components; a sentence saying so keeps the pair from reading as a contradiction.
An operator who points --state-dir at a dedicated filesystem through a symlink had the guard refuse it, so the recommended layout failed on upgrade. Follow the link on three showings instead: the directory holding it is writable by nobody but root, so only the operator could have aimed it; the link is a single hop to an already-canonical target, so nothing else chose what it resolves to; and the target's whole ancestry is writable by nobody but root, so nobody can rearrange it afterwards. Each showing answers an attack the others let through. Without the first, a link planted in a world-writable directory aims root at a victim. Without the second, a hop planted in a loose intermediate redirects a link the operator did make. Without the third, a single hop to a real directory under a writable parent hands that parent's owner the tree. Interior components stay refused outright: nothing below the root has an operator's reason to be a link. Refusing is the default for anything unproven, including a dangling or unreadable link, so what is not understood is never followed.
…m it The tree was chmodded 0700 but never chowned, so a state directory handed to the runner by an unprivileged owner stayed that owner's: 0700 against everyone except the one account that could still write it, and everything the runner built inside it. Take ownership as well as the mode. EPERM alone is ignored, which refuses exactly a process that never builds a jail, since a sandboxed Job checks for root by name long before this runs. The chroot tightened by path, which resolves a link, on the strength of a comment asserting nobody but root could have swapped one in. That was untrue for precisely the directory this fixes. It now tightens the same way the state tree does, through a descriptor opened O_NOFOLLOW, so the claim is a second fence rather than something the function has to trust. Re-prove the tree at job time as well. Host preparation runs once per process, and the per-Job path resolved an existing component, so a jail directory swapped for a link after preparation aimed every later chroot, guest rootfs, and chown at a directory somebody else chose. The sweep also removed each jail while its own listing was still open. Directory offsets are not stable across removal, so an entry could be skipped, and a skipped jail was worse than a missed one: the pass still reported nothing left behind, which spent the signal that would have earned the next Job another sweep. Nothing came back for it until a restart, in a design whose whole claim is that nothing latches. Take the listing first, then reclaim from it.
An interrupted poll was read as a VMM still running. Inside the retry loop that cost nothing, but the last look before the verdict turned it into a job failed on a clean host, naming a pid that had already exited, which is the misdiagnosis that look exists to prevent. A signal reports an arrival in this process and says nothing about the one being watched. Every other unreadable answer still counts as still running.
These paths formatted the cause into a string and dropped it, which loses the kind a caller could match on and the source a reader follows. Name the stream and the port the socket file carries instead, and keep the original error.
An unparseable component was dropped, so a set the runner could not read became a partial or empty one, and two unreadable renderings compared equal and passed verification. The read that decides whether confinement was applied is the last place to answer a question nobody asked. An empty set stays a real reading: it is a set narrowed to nothing, which fails for saying so.
Jobs sharing a state directory are serialized by the jail lock, so the capacity it needs is one jail, not one per concurrent Job. The symbolic link form is supported and was undocumented, including the precondition that decides whether it works: the target has to exist already, so a link to a filesystem not yet mounted is refused rather than created through.
Between the spawn and the guard's construction there was one fallible step, and `Child::drop` neither kills nor waits, so a failure there left the jailer, or the VMM it had become, running with nothing armed to reap it while the jail teardown removed the chroot out from under it. That is the outcome the reap exists to prevent, reached by the one path the reap never hears about. Construct the guard first and take stderr from it, so every fallible step is covered by a `Drop` that kills and reaps.
The announcer ran until the wait set its predicate, which happened only on the normal return. A panic in the wait unwound past that line and left the scope joining a thread whose predicate would never flip, so the process hung in place of propagating the panic. Flip it from a drop guard, which covers both ways out.
The accept branch is the one place this code follows a symlink as root, and it could only be exercised by a test running as root, which the unit tests do not. The showings stay exactly as they were, in the same order and refusing the same way; only who answers "is this writable by nobody but root" becomes a parameter, which is how the sweep's reap and the chroot's tightening are already tested. The elevated tests stay as the end to end proof, and now say when they skip. The create doc also claimed more than the mechanism delivers. A link cannot be applied through, but a root whose parent is writable can be swapped for a real directory the attacker already owns, and the chown and chmod do land on it. That is harmless, since it reaches only their own directory and neither is recursive, and it is worth saying rather than implying the window is empty.
The loop tracked what was still outstanding and then the message was built from which destinations were missing on disk. A destination left by an earlier run in the same output directory made those two disagree, and the error named nothing at all: entries not found, followed by an empty list.
The round trip left the skipped fields at their defaults, so it proved nothing about them and would not notice a new one. Set them away from default and destructure exhaustively, which makes adding a field a build error here rather than a value that quietly stops surviving.
The two steps that set the staged binary's mode reported through the bare io error, so a failure reached the operator as a permission denied with nothing to act on, next to a copy one line above that names the file it is about.
Reclaiming a jail waits out a VMM that will not exit and then a cgroup that will not go away, and host preparation holds the jail lock throughout. A state directory holding several of those stalls the runner for minutes with nothing printed, which reads exactly like a hang. The lock already solved this for its own wait, so the sweep repeats on the same schedule through the same mechanism. Nothing about the reaping changes: the bounds, the waits, and every verdict are untouched, and a jail reclaimed inside one interval still prints nothing. Also say why the elevated tests build their base directly under the filesystem root. The accept path they exercise requires an ancestry writable by nobody but root the whole way to it, which no directory under a temporary one can offer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Confine the Firecracker VMM with the Firecracker jailer. The runner spawns
jailer, which builds a chroot, creates the guest device nodes, drops to an unprivileged uid and gid, joins an empty network namespace, and execs Firecracker in place.jailerfrom the same pinned release archive as Firecracker, so the VMM and its jailer cannot drift apart across a runner self-update--state-dir(default/var/lib/bencher-runner), the persistent home for each Job's chroot, with an advisory lock and a sweep that reclaims jails left behind by a runner that exited without unwinding--jail-uid/--jail-gid(default61016), validated to reject0from flag, environment, and library alike, with a warning when the id resolves to a named local accountcgroup.procswritten frompre_execplaces the child before it execs the jailer, and Firecracker inherits itWhy
Managed bare metal Runners execute arbitrary code submitted by anyone. Until now the VMM was a plain child of the runner, so it inherited the runner's root, the full host filesystem, and the host network namespace. The runner itself has to be root to reach
/proc/irq/*/smp_affinity_list,/dev/cpu_dma_latency, andcgroup.subtree_controlfor measurement fidelity, which is precisely why the VMM must stop inheriting it. An escape landed as root on the host, holding the Runner key, every prior Job's work directory, and write access to the self-updating runner binary.Moving placement before the exec also fixes a second defect. The cpuset used to be applied after the VMM was already running, so Firecracker booted its API and touched memory on the wrong cores before being moved onto the benchmark cores.
Reaping orphans matters for the same reason. A runner killed mid-Job used to leave a jailed Firecracker running on the benchmark cores indefinitely, contending with every subsequent run on that host until someone noticed. Nothing downstream detects that contention, so it surfaces as wrong numbers that look right, which is the worst available failure mode for this product. It also leaves untrusted guest code executing with no timeout, because the process that enforced the timeout is gone.
No cgroup flags are passed to the jailer. The runner has to create, verify, read metrics from, and remove the cgroup, and both the cpuset partition and the per-VM cpuset need read-back verification because the kernel accepts the write and reports rejection inline. The jailer's write-once interface cannot provide that. Neither
--daemonizenor--new-pid-nsis passed either: both make the jailer fork, which would break the pid identity the process management relies on.Confinement failures are fatal, so untrusted code never runs with silently degraded confinement. Fidelity mechanisms keep their degrade behavior where isolation is genuinely unavailable, since a declared absence of isolation is not a lie about it, but a half-applied one is an error.
Breaking change
A Runner that executes sandboxed Jobs must now run as root. The jailer needs
mknodfor the chroot's device nodes,chownto hand the guest images to the jail user,pivot_root, andsetns. A world-readable/dev/kvmlets a process use KVM but not build the jail around it. Sandboxed Jobs previously ran fine unprivileged.A Sandbox implies a jail. Operators who cannot run as root can use
--danger-allow-no-sandbox, which trades away the microVM, not merely the jail. There is deliberately no sandbox-without-jail mode.How
plus/bencher_runner/src/jail/:state.rs,netns.rs,chroot.rs,paths.rs,reap.rs, and the state directory lockplus/bencher_runner/src/firecracker/process.rs: the jailer argv and thepre_execplacementplus/bencher_runner/src/firecracker/mod.rs: placement and verification, both conditional on the cgroup existingplus/bencher_runner/build.rs,src/jailer_bin.rs: bundle the jailer out of the archive already pinned by SHA256, adding no new download or trust anchortasks/test_runner: scenarios covering confinement and orphan reclamation, with a host-side probe for invariants that only exist while the VMM is alivetasks/test_api: the sandboxed smoke test runner is elevated; the no-sandbox runner beside it stays unprivileged, so one run proves the coupling in both directions.github/workflows/runner.yml: scenario binaries build unprivileged and run elevatedAlso fixes
ensure_runner_binhardcodingworkspace_root/targetand ignoringCARGO_TARGET_DIR, which blocked running the suite against a separate target directory.Verification
Confinement is proven end to end in CI: the jail scenarios boot a real guest under the jailer, confirm it runs as the jail uid inside the chroot and is already in its cgroup before the guest starts, and confirm the sweep reclaims an orphaned jail, its VMM, and its cgroup.
Unix socket paths are the one non-obvious constraint. The 108 byte
sun_pathlimit applies to the string before resolution, so the host view of a deep jail is unusable. Sockets are addressed through anO_PATHdescriptor on the chroot, and the limit is checked at construction with an error naming the path rather than presenting as a Firecracker timeout.Not yet run: a before and after benchmark variance comparison. The guest rootfs moves from a
/tmptemp directory to the state directory, so on hosts where/tmpis tmpfs the guest block device changes from RAM backed to disk backed. That is the most plausible source of a variance regression, and--state-dircan be pointed at a tmpfs if it proves to be one.Accepted risks
This moves a security boundary, so the risks that were weighed and kept are named here rather than only in the code.
The socket view is not lifetime bound to the descriptor it names. A
SocketPathis a/proc/self/fd/Nstring, and nothing in the type system ties it to theJailPathsholding descriptorN. Onlybindandconnecttake that view, and a test pins the hazard by deliberately reclaiming the descriptor number, but what makes it safe today is declaration order invm_executeon both the success and error paths. That ordering is load bearing and unenforced. Binding the lifetime is the follow-up.The network namespace handle is global to the host.
ensureholds its lock across the rebuild but not across the jailer's open, so two runners with different--state-dirvalues can make one another's jailer seeENOENT. That fails the job loudly, which is the point: no ordering here leaves a VMM on the host network, and a handle per state directory would buy isolation this deployment does not need.The jail uid is shared across runners on one host. Two VMMs running as the same unprivileged uid can signal, and possibly
ptrace, one another. This matters only where a single host serves runners with separate state directories, and--jail-uidgives each of them a distinct id.