Confine the Firecracker VMM with the jailer - #960
Draft
epompeii wants to merge 91 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 I've read through the full diff. Here's my review. Review: Jail the Firecracker VMMScope: ~8,400 lines across Overall this is careful, high-quality work. The confinement design is sound: chroot + unprivileged uid + empty netns + cgroup placement in Findings below, most significant first. I found no security defects and no correctness bug I can point at concretely. 1.
|
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.64 µs(-0.89%)Baseline: 4.68 µs | 5.00 µs (92.77%) |
| Adapter::Magic (JSON) | 📈 view plot 🚷 view threshold | 4.48 µs(-1.38%)Baseline: 4.54 µs | 4.81 µs (93.20%) |
| Adapter::Magic (Rust) | 📈 view plot 🚷 view threshold | 25.48 µs(-0.99%)Baseline: 25.74 µs | 26.98 µs (94.46%) |
| Adapter::Rust | 📈 view plot 🚷 view threshold | 3.53 µs(+0.08%)Baseline: 3.53 µs | 3.93 µs (89.91%) |
| Adapter::RustBench | 📈 view plot 🚷 view threshold | 3.53 µs(-0.00%)Baseline: 3.53 µs | 3.93 µs (89.75%) |
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.
The check lived in the argument parser, so it protected the one caller that went through the CLI and nothing else. `bencher_runner` is a library, and the reason the rule exists holds for every caller: the path reaches the jailer as `--chroot-base-dir`, which the jailer resolves against its own working directory rather than the runner's, so a relative one has every path the state directory hands out naming a different file for the jailer than for the runner. `StateDir` carries the invariant now, so a handle to a relative root cannot be constructed. The parser keeps its check and calls the same function rather than restating the rule, so there is one implementation and the operator still hears about it at the command line instead of when the first sandboxed Job builds its jail.
The guard that keeps the runner from chmodding a directory it does not own returned the moment it saw an entry called `jail` or `.lock`, before looking at anything else. `/var/lib` on a host running this runner has a directory called `jail`, and so does anything anyone happened to name that way, so a populated system directory passed the guard and was tightened to 0700. That is the `--state-dir /var/lib` hazard the guard was written for, defeated by the shallowest possible check. Ownership is proven by `jail/firecracker` now, a path only this runner builds, and it builds the whole tree in one step so a shallower half never stands for the whole. The lock file name goes back to being private to the lock, since proving ownership by a name is exactly what stopped being something to do.
The rule the audit produced covered reads that produce a value. A read that gates an action breaks it just as badly and looks nothing like a measurement: `exists` returns false for an error as well as for absence, so a stat that failed authorizes whatever the false branch does. A grep for the idiom found four in this file, three of which defeat a guard written for the exact outcome they allow. `remove_stale_cgroup` read a stat error as "already gone" and returned `Ok`, on which the sweep deletes the chroot that names the cgroup. That strands the cgroup permanently and destroys the only handle any later sweep had for finding it, which is precisely what the sweep's ordering exists to prevent. The retry loop had the same stat with the same meaning, so only a stat that succeeded and said absent counts now. `CgroupManager::new` read a stat error as "not there" and set the created flag, which has `Drop` remove a cgroup somebody else owns. `cleanup` read one as "gone already" and skipped both the removal and the signal, so nothing was armed and nothing would have come back for it. The fourth was harmless and is gone anyway: an `exists` gating an idempotent `create_dir_all` gated nothing the create does not gate itself. The unused cgroup v2 detector goes with it rather than sit there as an example of the pattern, and the cpuset write path no longer treats a missing file as an undelegated controller, which is what `verify_cpuset` concludes one step later about the same question.
The readiness loop polls the child between sleeps but never checked after the loop, so a process that exited during the final fifty milliseconds was reported as a socket that never became ready. That points an operator at Firecracker taking too long when the truth is that it is gone, which is the confusion this error was added to remove. One more poll before giving up. Renamed with it. The jailer `exec`s Firecracker in place, so the pid the runner holds is the jailer up to that moment and the VMM afterwards, and from outside there is no telling which of the two died. Naming the jailer was right about half the time; the error says a jailed process exited and names both possibilities.
An unparseable status line became a 500, which attributes a server error to Firecracker that Firecracker never sent and sends whoever reads the log looking at the VMM. The response is malformed, the error for that already exists, and two tests asserted the invented status until the rule was written down.
Host preparation runs again whenever a sweep is owed, and it carried this warning with it. A host that keeps failing to reclaim a jail would repeat the same advisory on every job, which is how an operator learns to skip warnings. The account it names cannot change under a running runner, so it is worth saying once.
The rule covered reads that produce a value, which is why a stat before a destructive step slipped past it three separate times: it does not look like a measurement at all, and its failure authorizes destruction rather than fabricating a number. The header says so now, and the rows the syntactic sweep turned up are in the tables. The row for teardown killing a cgroup's survivors is gone. Nothing on the jailed path calls `kill_all`: the VMM is killed with a grace period before its cgroup comes down, and whatever survives is caught by the `rmdir`, which arms the retry. The non-sandboxed path calls it explicitly on timeout. Describing a step that does not exist is worse than describing none, because this table is read as a specification, and adding the call to justify the row would be changing behavior to fit a document.
Three copies of the same search differed only in a name and a hint, which is three places for it to drift. One helper takes both. A candidate that cannot be stat'ed is still passed over rather than reported, which is the whole of the failure handling this needs: the list is guesses, and the one thing it can conclude, that nothing was found, is reported by name with what to do about it.
`syscall` is variadic, so each argument is passed at the width it is written at rather than at any width the signature enforces. The integer literals and the descriptor were `i32` where the kernel takes a `long`. It works on the ABI this targets, which is exactly why it is worth writing down: the cast says the width is deliberate rather than inherited from a literal's default type.
The guest root filesystem moved under `--state-dir` when the sandbox became a jail, and the documentation never said so. An operator sizing that directory had no way to learn it holds a full guest rootfs and a copy of the microVM binary for every Job running at once, which is the difference between a few megabytes and several gigabytes. It also names the mitigation that was already made to work. A dedicated filesystem or a `tmpfs` keeps that write traffic off the system disk, which is the answer to what the jail costs a measurement, and a freshly created filesystem is accepted despite the `lost+found` it arrives with, specifically so that mounting one is a supported thing to do. Nine locales, both subcommands.
The classification stands: this gating stat can only withhold a reading, never invent one. Every field is an option that is absent unless it was read, the cgroup block is omitted entirely when there is nothing to read, and the zero that used to stand in for an unparsed field is gone. There is no path here to a number that was not measured, which is what the rule is about. The reason is in the function now, so the next reader does not have to derive it from the type signatures. One thing was worth tightening, for a different reason than the rule. A stat that failed suppressed the reads entirely, throwing away metrics the files might still have given. Only a stat that succeeded and said absent means there is nothing to read now; anything else goes on to ask the files, which report what they can and nothing more. A test pins the property that makes the stat harmless.
The flag was set after preparation returned, so a preparation that failed at any step past the warning left it unset and the advice came back on the next job. On a host whose sweep keeps failing that is every job, which is how an operator learns to skip warnings. It is recorded where it is printed. The table also says it is unenforced on purpose. Nothing checks that the code matches it, a checker would be worth having, and building one here is not the trade to make: the table works by being read, and a reader adding a step has to pick a column. Saying so keeps the gap from looking like an oversight.
"would stranded" to "would strand".
The wait polled between sleeps and then returned a bare `false`, so a VMM that exited during the last interval of its five second budget was never looked at again. The sweep turns that into `JailError::JailStillRunning` and fails the job, confidently naming a pid that is already gone, on a host that is clean. Narrow, and the next sweep heals it, but a spurious failure with a definite error message is worse than most of what this branch has fixed. The same shape as the readiness wait in the VMM process, where the cost was a misleading message rather than a failed job. The deadline and the liveness check become parameters so the interesting case can be tested at all: reproducing a process that exits in the final interval against a real process means racing a sleep, while a deadline already spent proves the check after the loop is the whole verdict, with no sleeps and nothing timing-dependent.
Third instance of one loop shape, and this one is sound, so the reasoning is written down rather than left to be re-derived by whoever greps for the pattern next. A child that exits during the final sleep falls through to the kill, and nothing was reaped, so the pid is still reserved by the child and cannot have been recycled: the signal is one a zombie ignores, and the kill then reaps it and joins the reader. That is exactly what the loop would have done, so there is no verdict here to get wrong.
`if let Reaped::StillRunning` sent both other outcomes down the same path, so a third one arriving later would fall through into another pass of the loop rather than being reported. `reap_one` cannot return `Unexaminable` today, so this is not a live defect; it is the construct that would let one become invisible. In the one module whose entire argument is that a state nobody could examine must not read as a cleared one, that belongs to the compiler.
`TEST.md` told the reader twice to run `cargo test-runner scenarios`, which has always failed since the scenarios started requiring root: the sandbox is built by dropping privilege, so they refuse to start without it. The working two-step form existed only in the CI workflow and in the bail message. A contributor following the documented instructions was guaranteed a failure, which is the cost the rule about keeping these current exists to prevent. Both invocations are the two-step form now, with the reason the build stays unprivileged: it keeps `cargo` from leaving root-owned artifacts in the target directory. The failure-patterns section replaces the note about expected tuning permission errors, which cannot happen now that the scenarios pass `--no-tuning`, with the two things a reader will actually hit: the root refusal, and no tuning output at all. The root `CLAUDE.md` entry says root alongside Linux and KVM.
Elevating the scenarios turned real tuning on for all twenty-five of them, and `--no-tuning` went in to stop the suite detuning the machine running it. That bought safety by giving up every scrap of coverage for eleven knobs and the cpuset partition, and the half that matters most had never executed anywhere: `TuningGuard` restores on `Drop`, and nothing in CI had ever watched it do so. One scenario gets tuning back, and it asserts the pair. While the Job runs, each setting the host will let the runner change must show its tuned value; once the runner exits, every one must be back to what it was. Applying is what the runner is for, restoring is what keeps a benchmark host from drifting a knob at a time across every Job it ever runs. What is exercised is decided by asking the host rather than by assuming. A setting that is absent, already at the target, or present but not writable is recorded and skipped, and writability is established by writing back the value already there, which changes nothing: on a kernel with no hardware watchdog `/proc/sys/kernel/nmi_watchdog` exists, reads `0`, and refuses writes, and waiting for it to change would fail for the host's reasons rather than the runner's. A host that offers nothing to change fails the scenario rather than passing it, because a tuning test that tunes nothing is the vacuous pass this suite has spent the most effort removing. The blast radius is bounded four ways. It is one scenario, so only it can contaminate anything. It runs last, so what it leaves cannot reach the others. The harness snapshots every setting first and restores them from its own copy through a guard that runs on panic and on early return, so the scenario is safe even when the mechanism it tests is broken, which is the property a test of a restore path needs. And two knobs are excluded by argument: `--smt` keeps hyper-threading on, since offlining a sibling changes the core count for everything after it and no harness can put a CPU back, and `--no-irq-steering` because an unmovable IRQ rejects the restoring write with EIO, so the harness could not promise to undo it.
What `host_tuning` covers, what it deliberately does not, and the two failures a reader will actually hit: a host with nothing to change, which is refused rather than passed, and a machine left tuned, which can only mean the guard under test failed since the harness restores from its own snapshot regardless.
CI found the runner leaving `/sys/fs/cgroup/bencher` with `cpuset.cpus=1-3` and `cpuset.mems=0` after a run that reported restoring the partition. Neither obvious explanation was right, and both were checked against a real cgroupfs rather than reasoned about. The restore is not broken: replaying the exact sequence as root on a real cgroup v2 host, including a delegated `cpuset` subtree and a child cgroup, clears both files back to empty and prints all three restore lines. The newline write the unit test asserts works on the real filesystem too. Only one restore line looked present in CI because the other two carry a newline as their value, so they print as a line ending in a space followed by a blank one. What does defeat it is a task still in a descendant cgroup: the kernel refuses to clear a parent's cpuset with `EIO` while anything below it would be left without CPUs, which reproduces the CI symptom exactly, values and all. The partition mode restores, the two cpuset files do not, and the runner reports both outcomes honestly. So the residue is real and the runner should not leave it. The `bencher` cgroup is the runner's own, nothing else reads it, and the next job recreates it on demand, so the guard now removes it once the settings are restored. `rmdir` is self-guarding: it succeeds only when the cgroup is empty, so a concurrent runner's job or a task this one could not reclaim leaves it standing. That also breaks a quieter cycle, since a stale `cpuset.cpus` becomes the value the next tuned run saves as the original and faithfully restores forever. Verified on a real cgroupfs: the cgroup is gone after the guard drops, where before it stood with this run's cpuset in it. The fake tree the unit tests use cannot show this, because ordinary files in a directory make `rmdir` fail where a kernel cgroup's own files do not.
The first CI run of this scenario reported which files were not restored and nothing about why, and the answer, that clearing a parent cpuset is refused while a task remains in a descendant, cost an afternoon of experiments on a real cgroupfs to establish. The bail now says whether the cgroup is still there, what tasks it holds, and which children hold what, so the next failure of this kind arrives with its cause attached rather than costing another round.
Pid 7580 was a live process, and the evidence says whose. A killed and reaped process leaves no rmdir race at all, zero failures in forty trials, and a zombie is not even listed in `cgroup.procs`, so a pid listed there is running. The runner is never in a job cgroup: the placement writes `0` from inside the forked child, which is the jailer that becomes the VMM. So a live VMM outlived its job. How it got there is the harness. One scenario cancels the runner with SIGTERM, and `runner run` installs no handler for it, unlike `runner up`: the process dies without unwinding, leaving its VMM alive in its cgroup and its chroot on disk. That is precisely the case the sweep exists for, and the sweep identifies the VMM by the chroot, comparing device and inode against `/proc/<pid>/root`. Then the harness wipes the state directory before the next scenario and destroys that handle, so every later sweep reads a clean host and the orphan runs on through the rest of the suite. It reached the tuning scenario last of all, where a live task in a descendant made the kernel refuse the parent's cpuset clear with EIO, which is the failure CI reported. The product refuses to remove a chroot whose VMM is alive for exactly this reason. The harness has been doing it once per scenario. It now reaps what the wipe would strand first, killing the VMM and removing the cgroup that shares its name, which is what the sweep would have done had the handle survived. Killing here is the harness's own business; the constraint against it applies to the runner's teardown, which is untouched. The partition diagnosis names each stranded process now rather than printing a bare pid, because what the process is decides whose bug it is, and that cost a round to establish this time.
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.