From 450dc94fab37837fc2c1b4ffdfb7f1fdfef6205f Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Tue, 20 Oct 2026 12:31:21 -0700 Subject: [PATCH 1/4] nvx-rs: add caller-owned guest cgroup and child-status APIs Add two guest libraries on which any guest init or agent builds, without NVX's control protocol, PID 1, or an async runtime. `nvx::guest::cgroup` takes the agent's cgroup and the workload cgroup from the caller, as a `Layout`, instead of the fixed `nvx-exec`, and refuses a workload cgroup that holds the agent's. An `ExecutionCgroup` is the cgroup of one execution: the workload cgroup itself, or a child cgroup of it, which `prepare` creates exclusively, with the resource values that the caller chooses. Through it, the caller places a process, at spawn or afterwards, checks occupancy through `pids.current`, kills with `cgroup.kill`, and settles, over the execution's cgroup or the whole workload subtree, as the caller's `Scope` chooses. Settling calls the caller's status collector between its checks and collects no status itself, `remove` refuses an occupied cgroup, and a `prepare` that fails removes the execution cgroup that it created. `nvx::guest::process` gives each child one status collector: the caller. A `Child` waits only for the child that it names, with `waitid` on its pidfd or on its process ID, so every other child's status stays for its collector. A caller that collects every status itself, such as a PID 1 that adopts orphans, hands each one to its `Child` with `observe`. Statuses are `std::process::ExitStatus`, real-time signals included. Only an exit or a terminating signal ends a child: a stop doesn't, not even one that a wait reports to the child's tracer. The managed agent becomes a caller. The direct agent keeps its own `waitpid(-1)` loop, as PID 1, and hands the workload's status to the workload's `Child`; it prepares, kills, and settles `nvx-exec` through an `ExecutionCgroup`, over the whole subtree, as before. The sandbox agent places each launcher in `container` through the same handles. Admission, mode-specific cleanup, diagnostics, and the agent's test fixtures are unchanged, except that a direct workload that makes the agent its tracer and stops no longer counts as ended. A test on a real cgroup v2 hierarchy, which runs as root, shows that a descendant that leaves the workload's session and holds its output pipe is found, killed, and settled while a sibling execution keeps running, and another shows that the cgroup operations leave an unrelated child's status to its collector. The managed lifecycle test gains a workload whose detached descendant holds its output: it ends at once, and the next workload runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f1cb1a1-0d06-46b2-a35a-54af8eb9583c --- doc/project-structure.md | 2 +- nvx-rs/README.md | 15 +- nvx-rs/src/guest/cgroup.rs | 1037 ++++++++++++++++++++++ nvx-rs/src/guest/managed_agent/cgroup.rs | 171 ++-- nvx-rs/src/guest/managed_agent/exec.rs | 74 +- nvx-rs/src/guest/managed_agent/launch.rs | 5 +- nvx-rs/src/guest/managed_agent/mod.rs | 9 + nvx-rs/src/guest/managed_agent/seam.rs | 5 +- nvx-rs/src/guest/mod.rs | 14 +- nvx-rs/src/guest/process.rs | 420 +++++++++ nvx-rs/src/guest/spawn.rs | 7 +- nvx-rs/src/guest/sys/mod.rs | 28 +- nvx-rs/src/guest/sys/process.rs | 139 ++- nvx-rs/src/lib.rs | 8 +- nvx-rs/tests/managed_lifecycle.rs | 33 +- 15 files changed, 1792 insertions(+), 175 deletions(-) create mode 100644 nvx-rs/src/guest/cgroup.rs create mode 100644 nvx-rs/src/guest/process.rs diff --git a/doc/project-structure.md b/doc/project-structure.md index ac5a64d47..c61c0ab61 100644 --- a/doc/project-structure.md +++ b/doc/project-structure.md @@ -104,7 +104,7 @@ nvx/ | | |-- net/ CIDR parsing and subtraction, and hostname and guest-network forms | | |-- host/ Artifacts, launch, network and egress-policy lowering, sessions, VM instances, sandbox stores, the sandbox launch profile, and the `nvx-rs` CLI (feature `cli`) | | |-- testing/ Fake OpenVMM (feature `testing`) -| | |-- guest/ Programs that run in Linux guests, the guest's NVXS connection, and the library that starts a program for any guest init or agent (feature `guest`) +| | |-- guest/ Programs that run in Linux guests, the guest's NVXS connection, and the libraries through which any guest init or agent starts a program, collects its children's statuses, and settles its executions' cgroups (feature `guest`) | | `-- bin/ nvx-rs CLI, nvx-fake-openvmm test double, and the guest binaries | |-- tests/ Fake-OpenVMM instance lifecycle, sandbox store, CLI, NVXS session, guest-binary, and hypervisor managed-lifecycle tests, the store's record fixtures, and the golden fixtures that Python recorded | |-- build.rs Stages the bundled artifacts (feature `bundled`) diff --git a/nvx-rs/README.md b/nvx-rs/README.md index 824db9f77..b8d4768f8 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -36,8 +36,9 @@ nvx-rs/ │ │ with the Python semantics they keep; with feature `cli`, the `nvx-rs` CLI │ ├── testing/ feature `testing`: the fake OpenVMM │ ├── guest/ feature `guest`: the programs that run in Linux guests, the guest's NVXS -│ │ connection, and the library through which a guest init or agent starts a -│ │ program +│ │ connection, and the libraries through which a guest init or agent starts +│ │ a program, collects its children's statuses, and settles its executions' +│ │ cgroups │ └── bin/ nvx-rs, nvx-fake-openvmm, and the guest binaries ├── tests/ fake-OpenVMM, CLI, NVXS session, process-supervision, store, and guest-binary │ tests, the store's record fixtures, and the golden fixtures that Python @@ -69,6 +70,8 @@ nvx-rs/ | `nvx::guest` | The programs that run in Linux guests; see [Guest binaries](#guest-binaries) | | `nvx::guest::nvxs` | `GuestConnection`: the guest's role of the NVXS session over a byte stream, such as the control console, on which NVX's managed agent and other guest agents run their sessions. It never reads past the record in progress, so a caller that polls the stream between records sees every record that it has not returned | | `nvx::guest::spawn` | `Command`: starts a program in a new process for any guest init or agent, with its arguments, environment, root and working directories, standard streams, inherited descriptors, cgroup, and process group, without NVX's control protocol, managed agent, or workload identity. Its `LaunchReport` tells whether the program started, or which `LaunchStage` failed and why. NVX's managed agent is one caller | +| `nvx::guest::process` | `Child`: the status of one child that the caller names, by its pidfd or its process ID. It waits for that child alone, so another child's status stays for the caller; a caller that collects every status itself, such as a PID 1 that adopts orphans, hands each one to its `Child` with `observe`. Statuses are `std::process::ExitStatus`, real-time signals included | +| `nvx::guest::cgroup` | `Layout`: the agent's cgroup and the workload cgroup, which the caller names, and which may not hold the agent's. `ExecutionCgroup`: the cgroup of one execution, the workload cgroup itself or a child of it, which it prepares with the caller's resource values, places processes in, checks for occupancy through `pids.current`, kills with `cgroup.kill`, and settles, over the execution's cgroup or the whole workload subtree, as the caller's `Scope` chooses. Settling calls the caller's status collector between its checks, and collects no status itself | Lowering functions take the names by which messages refer to their inputs, such as `EgressRules::name` and `PathList::name`, so callers' messages name their own @@ -490,9 +493,11 @@ as `nvx/`. The `guest` feature adds the programs that run in NVX's Linux guests: the `nvx::guest` module and one binary per program. The Alpine, Ubuntu, and Azure Linux images ship all nine at their established `/sbin/nvx-*` paths. The -module's `spawn` library, which starts a program in a new process, is not a -program: `nvx-managed-agent` launches its workloads through it, and any other -guest init or agent can too. +module's libraries are not programs: `spawn` starts a program in a new +process, `process` collects the status of a child that the caller names, and +`cgroup` places, kills, and settles the processes of an execution. +`nvx-managed-agent` launches, collects, and settles its workloads through them, +and any other guest init or agent can too. | Binary | Program | | --- | --- | diff --git a/nvx-rs/src/guest/cgroup.rs b/nvx-rs/src/guest/cgroup.rs new file mode 100644 index 000000000..c3497debe --- /dev/null +++ b/nvx-rs/src/guest/cgroup.rs @@ -0,0 +1,1037 @@ +//! Cgroup v2 handles through which a guest init or agent places, accounts, kills, and settles the +//! processes of its executions. +//! +//! A [`Layout`] names the two cgroups that a caller uses: the agent's, which holds the caller +//! itself, and the workload cgroup, in which or below which its executions run. The library fixes +//! neither, and refuses a workload cgroup that holds the agent's, so that ending a workload's +//! processes never ends the agent. NVX's managed agent, for example, runs as PID 1 in the +//! hierarchy's root, and runs its workloads in the root's `nvx-exec`. +//! +//! An [`ExecutionCgroup`] is the handle of one execution's cgroup: the workload cgroup itself, +//! for a caller that runs one execution at a time, or a child cgroup of it, for one that runs +//! several, whose workload cgroup then holds no process of its own. Through it, the caller: +//! +//! - [prepares](ExecutionCgroup::prepare) the cgroup, with the resource values that it chooses; +//! - [places](ExecutionCgroup::placement) the execution's process in it, which its descendants +//! then inherit, however they detach from its process group or session; +//! - checks whether any process [occupies](ExecutionCgroup::is_occupied) it; +//! - [kills](ExecutionCgroup::kill) every process in it with `cgroup.kill`; +//! - and [settles](ExecutionCgroup::settle) it: waits until no process is left. +//! +//! Each of the last three covers a [`Scope`] that the caller chooses: the execution's cgroup +//! alone, or the workload cgroup's whole subtree. +//! +//! Occupancy is the cgroup's `pids.current`, which counts every process that the cgroup charges, +//! a zombie that no process has collected included. The library collects no status: settling +//! calls the caller's own status collector between its checks, as [`process`](super::process) +//! describes. Each operation opens the cgroup's interface files anew, so a handle stays valid +//! whatever else changes the cgroup. +//! +//! ```no_run +//! use std::time::Duration; +//! +//! use nvx::guest::cgroup::{ExecutionCgroup, Layout, Scope}; +//! use nvx::guest::process::Child; +//! use nvx::guest::spawn::Command; +//! +//! # fn main() -> std::io::Result<()> { +//! let layout = Layout::new("/sys/fs/cgroup/agent", "/sys/fs/cgroup/workloads")?; +//! let cgroup = ExecutionCgroup::prepare(&layout, Some("exec-1"), &[("pids.max", "64")])?; +//! let spawned = Command::new(c"/bin/true".to_owned(), vec![c"true".to_owned()]) +//! .cgroup(cgroup.placement()?) +//! .spawn()?; +//! let mut child = Child::new(spawned.id())?; +//! child.wait()?; +//! // End whatever the execution left behind, without touching the other executions. +//! cgroup.kill(Scope::Execution)?; +//! cgroup.settle(Scope::Execution, Duration::from_secs(2), || Ok(()))?; +//! cgroup.remove()?; +//! # Ok(()) +//! # } +//! ``` + +use std::fs; +use std::io; +use std::path::{Component, Path, PathBuf}; +use std::time::{Duration, Instant}; + +use super::spawn::Cgroup; +use super::sys::{self, Errno, Mode, OpenFlags, ProcessId}; + +/// A cgroup's membership file. +const PROCS: &str = "cgroup.procs"; +/// A cgroup's file that kills every process in it and below it. +const KILL: &str = "cgroup.kill"; +/// A cgroup's file that enables controllers for its children. +const SUBTREE_CONTROL: &str = "cgroup.subtree_control"; +/// The `pids` controller's count of the processes in a cgroup and below it. +const PIDS_CURRENT: &str = "pids.current"; +/// The longest canonical `pids.current` that a check reads, with its newline. +const COUNT_CAPACITY: usize = 32; +/// The interval between two checks of a settling cgroup. +const SETTLE_INTERVAL: Duration = Duration::from_millis(10); + +/// The cgroups that a guest init or agent uses: its own, and the workload cgroup in which or +/// below which its executions run. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Layout { + /// The cgroup that holds the agent itself. + agent: PathBuf, + /// The cgroup in which or below which the executions run. + workload: PathBuf, +} + +impl Layout { + /// Names the agent's cgroup, `agent`, and the workload cgroup, `workload`, as the absolute + /// paths of their directories in a mounted cgroup v2 hierarchy. + /// + /// The paths are compared as written, so the caller names both without symbolic links or + /// other aliases: the check keeps a mistaken layout from ending the agent, not a deliberate + /// one. A relative path, one with a `..` component, a workload cgroup without a parent, and + /// one that is the agent's or holds it are `EINVAL`. The workload cgroup is not the + /// hierarchy's root either, which has none of the interface files that the handles use. + pub fn new(agent: impl Into, workload: impl Into) -> io::Result { + let agent = agent.into(); + let workload = workload.into(); + if !well_formed(&agent) + || !well_formed(&workload) + || workload.parent().is_none() + || agent.starts_with(&workload) + { + return Err(Errno::EINVAL.into()); + } + Ok(Self { agent, workload }) + } + + /// Returns the directory of the agent's cgroup. + pub fn agent(&self) -> &Path { + &self.agent + } + + /// Returns the directory of the workload cgroup. + pub fn workload(&self) -> &Path { + &self.workload + } +} + +/// The processes that an operation of an [`ExecutionCgroup`] covers. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +pub enum Scope { + /// The execution's cgroup, and the cgroups below it. For an execution with a cgroup of its + /// own, the workload's other executions keep running; one that runs in the workload cgroup + /// itself covers the whole workload subtree. + Execution, + /// The workload cgroup, and the whole subtree below it, every execution's cgroup included. + Workload, +} + +/// The handle of one execution's cgroup: the workload cgroup itself, or a child cgroup of it. +/// +/// It names the cgroup by its path, and dropping it changes nothing. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ExecutionCgroup { + /// The execution's cgroup. + path: PathBuf, + /// The workload cgroup. + workload: PathBuf, + /// Whether the execution has a cgroup of its own below the workload cgroup. + own: bool, +} + +impl ExecutionCgroup { + /// Names the cgroup of an execution in `layout`, without changing the hierarchy: the + /// workload cgroup itself, or, with `name`, its child cgroup `name`. + /// + /// A name that is not one path component, such as an empty one, `.`, `..`, or one with a + /// `/` or a NUL, is `EINVAL`. + pub fn open(layout: &Layout, name: Option<&str>) -> io::Result { + let path = match name { + None => layout.workload.clone(), + Some(name) => { + let mut components = Path::new(name).components(); + if name.contains('\0') + || !matches!(components.next(), Some(Component::Normal(_))) + || components.next().is_some() + || name.contains('/') + { + return Err(Errno::EINVAL.into()); + } + layout.workload.join(name) + } + }; + Ok(Self { + path, + workload: layout.workload.clone(), + own: name.is_some(), + }) + } + + /// Prepares the cgroup of an execution in `layout`, which [`open`](Self::open) names, and + /// returns its handle. + /// + /// The workload cgroup is created unless it exists, and an execution's own cgroup is created + /// below it, which fails with `EEXIST` if it exists, so that no two executions share one. + /// Each of them gets the `pids` controller, which counts its processes, and the controller of + /// each resource: the controllers whose interface files it lacks are enabled in its parent's + /// `cgroup.subtree_control`, in one write. The workload cgroup's parent must have them, as + /// the hierarchy's root does. Then each resource's value is written to its file in the + /// execution's cgroup, in order: `("pids.max", "64")`, for example, limits the execution to + /// 64 processes. An empty value is written as a newline, as `echo` writes it, so that the + /// kernel applies or refuses it too. A workload cgroup that gives its executions' cgroups a + /// domain controller, such as `memory`, holds no process of its own, as cgroup v2 requires. + /// + /// A resource file that is not a controller's interface file, `CONTROLLER.NAME`, is `EINVAL`, + /// before anything changes. A failure once the execution's own cgroup exists, such as a value + /// that the kernel refuses, removes that cgroup again, so that its name can be prepared anew; + /// the workload cgroup stays, with the controllers that it enabled, as the executions share + /// it. + pub fn prepare( + layout: &Layout, + name: Option<&str>, + resources: &[(&str, &str)], + ) -> io::Result { + for (file, _) in resources { + controller(file)?; + } + let execution = Self::open(layout, name)?; + let above_workload = layout + .workload + .parent() + .expect("a layout's workload cgroup has a parent"); + let files = || std::iter::once(PIDS_CURRENT).chain(resources.iter().map(|(file, _)| *file)); + create(&layout.workload, false)?; + provide(&layout.workload, above_workload, files())?; + if execution.own { + create(&execution.path, true)?; + } + if let Err(error) = execution.configure(files(), resources) { + if execution.own { + // The new cgroup holds no process yet. + let _ = fs::remove_dir(&execution.path); + } + return Err(error); + } + Ok(execution) + } + + /// Gives the execution's own cgroup, if it has one, the interface files `files` through the + /// workload cgroup, then writes each resource's value to its file, in order. + fn configure<'a>( + &self, + files: impl IntoIterator, + resources: &[(&str, &str)], + ) -> io::Result<()> { + if self.own { + provide(&self.path, &self.workload, files)?; + } + for (file, value) in resources { + // Writing nothing would reach no controller. + let value = if value.is_empty() { "\n" } else { value }; + write(&self.path.join(file), value.as_bytes())?; + } + Ok(()) + } + + /// Returns the directory of the execution's cgroup. + pub fn path(&self) -> &Path { + &self.path + } + + /// Returns the placement that makes a [spawned](super::spawn::Command::cgroup) child join + /// the execution's cgroup before its program runs, so that the program and its descendants + /// start in it. + pub fn placement(&self) -> io::Result { + Cgroup::open(&self.path) + } + + /// Moves the process `id`, with all of its threads, into the execution's cgroup. An ID of 0, + /// or one above `i32::MAX`, is `EINVAL`. + pub fn place(&self, id: u32) -> io::Result<()> { + let process = ProcessId::from_positive(i64::from(id)) + .ok_or_else(|| io::Error::from(Errno::EINVAL))?; + write(&self.path.join(PROCS), &process.line()) + } + + /// Returns whether `scope` holds a process, a zombie that no process has collected included. + /// + /// It reads `pids.current`, which must be one canonical decimal and a newline: another form + /// is `EPROTO`, and one of more than 31 bytes `EOVERFLOW`. + pub fn is_occupied(&self, scope: Scope) -> io::Result { + let path = sys::path_c_string(&self.scope(scope).join(PIDS_CURRENT))?; + let descriptor = sys::open( + &path, + OpenFlags::O_RDONLY | OpenFlags::O_CLOEXEC, + Mode::empty(), + )?; + let mut buffer = [0u8; COUNT_CAPACITY]; + let mut length = 0; + let result = loop { + match descriptor.read(&mut buffer[length..]) { + Ok(count) if count > 0 => { + length += count; + if length < buffer.len() { + continue; + } + break Err(Errno::EOVERFLOW); + } + Ok(_) => break parse_count(&buffer[..length]), + Err(Errno::EINTR) => {} + Err(error) => break Err(error), + } + }; + let occupied = result?; + descriptor.close()?; + Ok(occupied) + } + + /// Kills every process in `scope` with `SIGKILL`, through `cgroup.kill`, which also kills a + /// process that one of them forks meanwhile. It does not wait for them to end: see + /// [`settle`](Self::settle). + pub fn kill(&self, scope: Scope) -> io::Result<()> { + write(&self.scope(scope).join(KILL), b"1") + } + + /// Waits at most `timeout` until `scope` holds no process, and fails with `ETIMEDOUT` if one + /// is left then. It ends no process itself: a caller [kills](Self::kill) the scope first, or + /// waits for its processes to end on their own. + /// + /// Before each check, it calls `reap`, the caller's status collector, which collects the + /// statuses of the caller's children that have ended, and whose failure ends the wait: a + /// zombie stays in `pids.current` until its parent collects it. A caller whose executions' + /// orphaned descendants another process collects, such as its PID 1, still collects the + /// status of each child of its own. A caller that drives its own poll loop checks + /// [`is_occupied`](Self::is_occupied) there instead. + pub fn settle( + &self, + scope: Scope, + timeout: Duration, + mut reap: impl FnMut() -> io::Result<()>, + ) -> io::Result<()> { + let deadline = Instant::now().checked_add(timeout); + loop { + reap()?; + if !self.is_occupied(scope)? { + return Ok(()); + } + let remaining = match deadline { + Some(deadline) => deadline.saturating_duration_since(Instant::now()), + None => SETTLE_INTERVAL, + }; + if remaining.is_zero() { + return Err(Errno::ETIMEDOUT.into()); + } + std::thread::sleep(remaining.min(SETTLE_INTERVAL)); + } + } + + /// Removes the execution's own cgroup unless it is gone already, and fails with `EBUSY` + /// while a process, a zombie included, or a cgroup is left in it, so that a removal never + /// hides a status that the caller has yet to collect. An execution that runs in the workload + /// cgroup itself leaves it in place. + pub fn remove(&self) -> io::Result<()> { + if !self.own { + return Ok(()); + } + match self.is_occupied(Scope::Execution) { + Ok(false) => {} + Ok(true) => return Err(Errno::EBUSY.into()), + Err(error) if error.kind() == io::ErrorKind::NotFound && !self.path.exists() => { + return Ok(()); + } + Err(error) => return Err(error), + } + match fs::remove_dir(&self.path) { + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(()), + result => result, + } + } + + /// Returns the directory of the cgroup that `scope` covers. + fn scope(&self, scope: Scope) -> &Path { + match scope { + Scope::Execution => &self.path, + Scope::Workload => &self.workload, + } + } +} + +/// Returns whether `path` is absolute and has no `..` component, so that its components name +/// the cgroups that it reaches. +fn well_formed(path: &Path) -> bool { + path.is_absolute() + && !path + .components() + .any(|component| component == Component::ParentDir) +} + +/// Returns the controller of the interface file `file`, `CONTROLLER.NAME`, or `EINVAL` for a +/// file that is not a controller's, such as one of the core `cgroup.*` files. +fn controller(file: &str) -> io::Result<&str> { + let name = |part: &str| { + !part.is_empty() + && part + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_') + }; + match file.split_once('.') { + Some((controller, rest)) + if controller != "cgroup" + && name(controller) + && controller.bytes().all(|byte| !byte.is_ascii_uppercase()) + && rest.split('.').all(name) => + { + Ok(controller) + } + _ => Err(Errno::EINVAL.into()), + } +} + +/// Creates the cgroup `path`, which fails with `EEXIST` if it exists and `exclusive` is set. +/// Otherwise an existing cgroup is accepted, and a file in its place is `ENOTDIR`. +fn create(path: &Path, exclusive: bool) -> io::Result<()> { + match sys::create_directory(&sys::path_c_string(path)?, Mode::from_bits_retain(0o755)) { + Ok(()) => Ok(()), + Err(Errno::EEXIST) if !exclusive && fs::metadata(path)?.is_dir() => Ok(()), + Err(Errno::EEXIST) if !exclusive => Err(Errno::ENOTDIR.into()), + Err(error) => Err(error.into()), + } +} + +/// Makes sure that the cgroup `cgroup` has each of the interface files `files`: the controllers +/// of those that it lacks are enabled together in `parent`'s `cgroup.subtree_control`, which +/// gives them to every child of `parent`. +fn provide<'a>( + cgroup: &Path, + parent: &Path, + files: impl IntoIterator, +) -> io::Result<()> { + let mut missing = Vec::new(); + for file in files { + match fs::metadata(cgroup.join(file)) { + Ok(_) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => { + let controller = controller(file)?; + if !missing.contains(&controller) { + missing.push(controller); + } + } + Err(error) => return Err(error), + } + } + if missing.is_empty() { + return Ok(()); + } + let enable = missing + .iter() + .map(|controller| format!("+{controller}")) + .collect::>() + .join(" "); + write(&parent.join(SUBTREE_CONTROL), enable.as_bytes()) +} + +/// Parses the canonical decimal form of `pids.current`, and returns whether it is nonzero. +fn parse_count(bytes: &[u8]) -> Result { + if bytes.len() < 2 + || bytes.last() != Some(&b'\n') + || (bytes.first() == Some(&b'0') && bytes.len() != 2) + || !bytes[..bytes.len() - 1].iter().all(u8::is_ascii_digit) + { + return Err(Errno::EPROTO); + } + Ok(bytes.first() != Some(&b'0')) +} + +/// Writes a whole control value to the existing file `path`, and reports a failure to close it, +/// with which the kernel may refuse the value. +fn write(path: &Path, bytes: &[u8]) -> io::Result<()> { + let path = sys::path_c_string(path)?; + let descriptor = sys::open( + &path, + OpenFlags::O_WRONLY | OpenFlags::O_CLOEXEC, + Mode::empty(), + )?; + descriptor.write_all(bytes)?; + Ok(descriptor.close()?) +} + +#[cfg(test)] +mod tests { + use std::cell::Cell; + use std::ffi::CString; + use std::io::Read; + use std::os::fd::AsFd; + use std::os::unix::process::ExitStatusExt; + + use nix::poll::{PollFd, PollFlags, PollTimeout}; + + use super::*; + use crate::guest::process::Child; + use crate::guest::spawn::Command; + + /// Returns a command that runs `/bin/sh -c script`. + fn shell(script: &str) -> Command { + Command::new( + c"/bin/sh".to_owned(), + vec![ + c"sh".to_owned(), + c"-c".to_owned(), + CString::new(script).unwrap(), + ], + ) + } + + /// Returns the error number of `result`'s error. + fn error(result: io::Result) -> Option { + result.unwrap_err().raw_os_error() + } + + /// Returns a fake hierarchy of regular files in a new temporary directory, which the caller + /// keeps: the root, which holds the agent, with an empty `cgroup.subtree_control`, and the + /// workload cgroup `workload`, without processes, as the layout of an agent that runs in the + /// root. + fn hierarchy() -> (tempfile::TempDir, Layout) { + let temporary = tempfile::tempdir().unwrap(); + let root = temporary.path().join("cgroup"); + let workload = root.join("workload"); + fs::create_dir_all(&workload).unwrap(); + fs::write(root.join(SUBTREE_CONTROL), b"").unwrap(); + for (file, contents) in [ + (PIDS_CURRENT, b"0\n".as_slice()), + (KILL, b""), + (PROCS, b""), + (SUBTREE_CONTROL, b""), + ] { + fs::write(workload.join(file), contents).unwrap(); + } + let layout = Layout::new(&root, &workload).unwrap(); + (temporary, layout) + } + + /// Creates the fake cgroup `name` of an execution below `layout`'s workload cgroup, with + /// the processes that `pids.current` counts, and returns its handle. + fn fake_execution(layout: &Layout, name: &str, pids: &[u8]) -> ExecutionCgroup { + let path = layout.workload().join(name); + fs::create_dir(&path).unwrap(); + for (file, contents) in [(PIDS_CURRENT, pids), (KILL, b""), (PROCS, b"")] { + fs::write(path.join(file), contents).unwrap(); + } + ExecutionCgroup::open(layout, Some(name)).unwrap() + } + + #[test] + fn layouts_keep_the_agent_out_of_the_workload_subtree() { + for (agent, workload) in [ + ("/sys/fs/cgroup", "/sys/fs/cgroup/nvx-exec"), + ("/sys/fs/cgroup/agent", "/sys/fs/cgroup/container"), + ("/sys/fs/cgroup/agent", "/sys/fs/cgroup/agent-workloads"), + ( + "/sys/fs/cgroup/workloads/agent", + "/sys/fs/cgroup/workloads/executions", + ), + ] { + let layout = Layout::new(agent, workload).unwrap(); + assert_eq!( + (layout.agent(), layout.workload()), + (Path::new(agent), Path::new(workload)) + ); + } + for (agent, workload) in [ + ("sys/fs/cgroup", "/sys/fs/cgroup/nvx-exec"), + ("/sys/fs/cgroup", "sys/fs/cgroup/nvx-exec"), + ("/sys/fs/cgroup/agent", "/"), + ("/sys/fs/cgroup/workload", "/sys/fs/cgroup/workload"), + ("/sys/fs/cgroup/workload/", "/sys/fs/cgroup/workload"), + ("/sys/fs/cgroup/workload/agent", "/sys/fs/cgroup/workload"), + ("/sys/fs/cgroup", "/sys/fs/cgroup/nvx-exec/.."), + ( + "/sys/fs/cgroup/workload/../agent", + "/sys/fs/cgroup/workload", + ), + ] { + assert_eq!( + error(Layout::new(agent, workload)), + Some(Errno::EINVAL as i32), + "{agent} {workload}" + ); + } + } + + #[test] + fn executions_run_in_the_workload_cgroup_or_in_one_child_of_it() { + let layout = Layout::new("/sys/fs/cgroup", "/sys/fs/cgroup/workloads").unwrap(); + let workload = ExecutionCgroup::open(&layout, None).unwrap(); + assert_eq!(workload.path(), layout.workload()); + for name in ["exec-1", "exec.1", "..."] { + let execution = ExecutionCgroup::open(&layout, Some(name)).unwrap(); + assert_eq!(execution.path(), layout.workload().join(name)); + } + for name in ["", ".", "..", "a/b", "/a", "a/", "./a", "a\0b"] { + assert_eq!( + error(ExecutionCgroup::open(&layout, Some(name))), + Some(Errno::EINVAL as i32), + "{name:?}" + ); + assert_eq!( + error(ExecutionCgroup::prepare(&layout, Some(name), &[])), + Some(Errno::EINVAL as i32), + "{name:?}" + ); + } + } + + #[test] + fn preparation_creates_the_cgroup_and_enables_its_controllers_together() { + let (temporary, _) = hierarchy(); + let root = temporary.path().join("cgroup"); + let layout = Layout::new(&root, root.join("fresh")).unwrap(); + let control = root.join(SUBTREE_CONTROL); + // A new cgroup lacks the files of both controllers, which one write enables. + let failure = + ExecutionCgroup::prepare(&layout, None, &[("memory.max", "max")]).unwrap_err(); + assert!(layout.workload().is_dir()); + assert_eq!(fs::read(&control).unwrap(), b"+pids +memory"); + // A fake hierarchy gains no file when a controller is enabled. + assert_eq!(failure.raw_os_error(), Some(Errno::ENOENT as i32)); + + // With every file in place, nothing is enabled, and the values are written in order. + fs::write(&control, b"").unwrap(); + for file in [PIDS_CURRENT, "memory.max", "memory.high"] { + fs::write(layout.workload().join(file), b"").unwrap(); + } + let execution = ExecutionCgroup::prepare( + &layout, + None, + &[("memory.max", "1024"), ("memory.high", "512")], + ) + .unwrap(); + assert_eq!(execution.path(), layout.workload()); + assert_eq!(fs::read(&control).unwrap(), b""); + assert_eq!( + fs::read(layout.workload().join("memory.max")).unwrap(), + b"1024" + ); + assert_eq!( + fs::read(layout.workload().join("memory.high")).unwrap(), + b"512" + ); + + // An existing workload cgroup is accepted, a file in its place is not. + fs::remove_dir_all(layout.workload()).unwrap(); + fs::write(layout.workload(), b"").unwrap(); + assert_eq!( + error(ExecutionCgroup::prepare(&layout, None, &[])), + Some(Errno::ENOTDIR as i32) + ); + } + + #[test] + fn invalid_resources_change_nothing() { + let (temporary, _) = hierarchy(); + let root = temporary.path().join("cgroup"); + let layout = Layout::new(&root, root.join("fresh")).unwrap(); + for file in [ + "cgroup.procs", + "cgroup.subtree_control", + "pids", + ".max", + "pids.", + "pids..max", + "Pids.max", + "pids/max", + "../pids.max", + "pids.max/x", + "pids.max\n", + ] { + assert_eq!( + error(ExecutionCgroup::prepare(&layout, None, &[(file, "1")])), + Some(Errno::EINVAL as i32), + "{file:?}" + ); + } + assert!(!layout.workload().exists()); + assert_eq!(fs::read(root.join(SUBTREE_CONTROL)).unwrap(), b""); + for file in [ + "hugetlb.2MB.max", + "memory.swap.max", + "cpuset.cpus.partition", + "io.max", + ] { + assert_eq!(controller(file).unwrap(), file.split('.').next().unwrap()); + } + } + + #[test] + fn an_execution_s_own_cgroup_is_new_and_takes_its_controllers_from_the_workload() { + let (_temporary, layout) = hierarchy(); + let root = layout.agent().to_owned(); + let execution = ExecutionCgroup::prepare(&layout, Some("exec-1"), &[]).unwrap(); + assert!(execution.path().is_dir()); + // The workload cgroup counts its processes already, so only it enables the controller. + assert_eq!(fs::read(root.join(SUBTREE_CONTROL)).unwrap(), b""); + assert_eq!( + fs::read(layout.workload().join(SUBTREE_CONTROL)).unwrap(), + b"+pids" + ); + // A second execution of the same name would share the first one's cgroup. + let duplicate = ExecutionCgroup::prepare(&layout, Some("exec-1"), &[]).unwrap_err(); + assert_eq!(duplicate.kind(), io::ErrorKind::AlreadyExists); + // A workload cgroup that lacks the controller gets it from its parent first. + fs::remove_file(layout.workload().join(PIDS_CURRENT)).unwrap(); + ExecutionCgroup::prepare(&layout, Some("exec-2"), &[]).unwrap(); + assert_eq!(fs::read(root.join(SUBTREE_CONTROL)).unwrap(), b"+pids"); + } + + #[test] + fn a_failed_preparation_leaves_no_cgroup_of_the_execution_s_own() { + let (_temporary, layout) = hierarchy(); + // No controller creates `pids.max` in a fake cgroup, so its write fails once the + // execution's cgroup exists. + let failed = ExecutionCgroup::prepare(&layout, Some("exec-1"), &[("pids.max", "16")]); + assert_eq!(failed.unwrap_err().kind(), io::ErrorKind::NotFound); + assert!(!layout.workload().join("exec-1").exists()); + // The name can be prepared anew. + let execution = ExecutionCgroup::prepare(&layout, Some("exec-1"), &[]).unwrap(); + assert!(execution.path().is_dir()); + // An execution in the workload cgroup itself leaves the shared cgroup in place. + let failed = ExecutionCgroup::prepare(&layout, None, &[("pids.max", "16")]); + assert_eq!(failed.unwrap_err().kind(), io::ErrorKind::NotFound); + assert!(layout.workload().join(PIDS_CURRENT).is_file()); + } + + #[test] + fn an_empty_resource_value_reaches_its_file_as_a_newline() { + let (_temporary, layout) = hierarchy(); + let limit = layout.workload().join("pids.max"); + fs::write(&limit, b"").unwrap(); + ExecutionCgroup::prepare(&layout, None, &[("pids.max", "")]).unwrap(); + // The kernel gets a value to apply or refuse, as from `echo`, not a write of nothing. + assert_eq!(fs::read(&limit).unwrap(), b"\n"); + } + + #[test] + fn occupancy_is_the_scope_s_canonical_process_count() { + let (_temporary, layout) = hierarchy(); + let execution = fake_execution(&layout, "exec-1", b"1\n"); + assert!(execution.is_occupied(Scope::Execution).unwrap()); + assert!(!execution.is_occupied(Scope::Workload).unwrap()); + let count = layout.workload().join(PIDS_CURRENT); + for (contents, expected) in [ + (b"0\n".as_slice(), Ok(false)), + (b"10\n", Ok(true)), + (b"4194304\n", Ok(true)), + (b"", Err(Errno::EPROTO)), + (b"0", Err(Errno::EPROTO)), + (b"00\n", Err(Errno::EPROTO)), + (b"-1\n", Err(Errno::EPROTO)), + (b" 1\n", Err(Errno::EPROTO)), + (b"max\n", Err(Errno::EPROTO)), + (&[b'1'; 40], Err(Errno::EOVERFLOW)), + ] { + fs::write(&count, contents).unwrap(); + assert_eq!( + execution + .is_occupied(Scope::Workload) + .map_err(|error| Errno::from_raw(error.raw_os_error().unwrap())), + expected, + "{contents:?}" + ); + } + fs::remove_file(&count).unwrap(); + assert_eq!( + error(execution.is_occupied(Scope::Workload)), + Some(Errno::ENOENT as i32) + ); + } + + #[test] + fn a_kill_reaches_the_scope_s_cgroup_alone() { + let (_temporary, layout) = hierarchy(); + let execution = fake_execution(&layout, "exec-1", b"0\n"); + execution.kill(Scope::Execution).unwrap(); + assert_eq!(fs::read(execution.path().join(KILL)).unwrap(), b"1"); + assert_eq!(fs::read(layout.workload().join(KILL)).unwrap(), b""); + execution.kill(Scope::Workload).unwrap(); + assert_eq!(fs::read(layout.workload().join(KILL)).unwrap(), b"1"); + fs::remove_file(execution.path().join(KILL)).unwrap(); + assert_eq!( + error(execution.kill(Scope::Execution)), + Some(Errno::ENOENT as i32) + ); + } + + #[test] + fn settlement_collects_statuses_between_its_checks_until_the_deadline() { + let (_temporary, layout) = hierarchy(); + let execution = fake_execution(&layout, "exec-1", b"2\n"); + let count = execution.path().join(PIDS_CURRENT); + // Each collection lets one process go, as collecting a zombie does. + let collections = Cell::new(0); + execution + .settle(Scope::Execution, Duration::from_secs(10), || { + collections.set(collections.get() + 1); + let left = 3 - collections.get().min(3); + fs::write(&count, format!("{left}\n")) + }) + .unwrap(); + assert_eq!(collections.get(), 3); + + fs::write(&count, b"1\n").unwrap(); + let failed = execution.settle(Scope::Execution, Duration::from_secs(10), || { + Err(Errno::ECHILD.into()) + }); + assert_eq!(error(failed), Some(Errno::ECHILD as i32)); + collections.set(0); + let timed_out = execution.settle(Scope::Execution, Duration::from_millis(50), || { + collections.set(collections.get() + 1); + Ok(()) + }); + assert_eq!(error(timed_out), Some(Errno::ETIMEDOUT as i32)); + assert!(collections.get() > 1); + fs::write(&count, b"bad\n").unwrap(); + assert_eq!( + error(execution.settle(Scope::Execution, Duration::ZERO, || Ok(()))), + Some(Errno::EPROTO as i32) + ); + // The workload cgroup is empty, whatever its execution's count. + execution + .settle(Scope::Workload, Duration::ZERO, || Ok(())) + .unwrap(); + } + + #[test] + fn processes_are_placed_in_the_execution_s_cgroup() { + let (_temporary, layout) = hierarchy(); + let execution = fake_execution(&layout, "exec-1", b"0\n"); + let procs = execution.path().join(PROCS); + let spawned = shell("exit 0") + .cgroup(execution.placement().unwrap()) + .spawn() + .unwrap(); + let mut child = Child::new(spawned.id()).unwrap(); + assert!(child.wait().unwrap().success()); + assert_eq!( + fs::read(&procs).unwrap(), + format!("{}\n", child.id()).into_bytes() + ); + execution.place(4_194_304).unwrap(); + assert_eq!(fs::read(&procs).unwrap(), b"4194304\n"); + for id in [0, u32::MAX] { + assert_eq!(error(execution.place(id)), Some(Errno::EINVAL as i32)); + } + assert!( + ExecutionCgroup::open(&layout, Some("missing")) + .unwrap() + .placement() + .is_err() + ); + } + + #[test] + fn only_an_empty_cgroup_of_the_execution_s_own_is_removed() { + let (_temporary, layout) = hierarchy(); + ExecutionCgroup::open(&layout, None) + .unwrap() + .remove() + .unwrap(); + assert!(layout.workload().is_dir()); + let execution = fake_execution(&layout, "exec-1", b"1\n"); + assert_eq!(error(execution.remove()), Some(Errno::EBUSY as i32)); + fs::remove_file(execution.path().join(PIDS_CURRENT)).unwrap(); + assert_eq!(error(execution.remove()), Some(Errno::ENOENT as i32)); + fs::remove_dir_all(execution.path()).unwrap(); + execution.remove().unwrap(); + } + + #[test] + fn cgroup_operations_leave_an_unrelated_child_s_status() { + let mut unrelated = std::process::Command::new("/bin/true").spawn().unwrap(); + let process = ProcessId::from_child(&unrelated); + while !sys::has_exited(process).unwrap() { + std::thread::yield_now(); + } + let (_temporary, layout) = hierarchy(); + let workload = ExecutionCgroup::prepare(&layout, None, &[]).unwrap(); + let execution = fake_execution(&layout, "exec-1", b"0\n"); + for cgroup in [&workload, &execution] { + let spawned = shell("exit 0") + .cgroup(cgroup.placement().unwrap()) + .spawn() + .unwrap(); + let mut child = Child::new(spawned.id()).unwrap(); + for scope in [Scope::Execution, Scope::Workload] { + assert!(!cgroup.is_occupied(scope).unwrap()); + cgroup.kill(scope).unwrap(); + cgroup + .settle(scope, Duration::from_secs(10), || { + child.try_wait().map(drop) + }) + .unwrap(); + } + assert!(child.wait().unwrap().success()); + } + // Nothing collected a status that it did not name. + assert!(unrelated.try_wait().unwrap().unwrap().success()); + } + + /// A workload cgroup in the real hierarchy, which the test removes, with everything in it, + /// when it ends. + struct RealWorkload(Layout); + + impl RealWorkload { + /// Creates a new workload cgroup below the root of the hierarchy at `/sys/fs/cgroup`, + /// with the test's own cgroup as the agent's. It returns `None` where the test cannot + /// change the hierarchy: unless the test runs as root in a cgroup v2 hierarchy with the + /// `pids` controller, in which it can create a cgroup, as a read-only mount, or one + /// that a user namespace's root does not own, does not let it. + fn new(name: &str) -> Option { + let root = Path::new("/sys/fs/cgroup"); + let controllers = fs::read_to_string(root.join("cgroup.controllers")).ok()?; + if !nix::unistd::geteuid().is_root() + || !controllers + .split_whitespace() + .any(|controller| controller == "pids") + { + return None; + } + let own = fs::read_to_string("/proc/self/cgroup").ok()?; + let own = own.lines().find_map(|line| line.strip_prefix("0::"))?; + let agent = root.join(own.trim_start_matches('/')); + let workload = root.join(format!("nvx-test-{}-{name}", std::process::id())); + // The test's cgroup lies outside the mount's view in another cgroup namespace, + // where it reads as a path with `..` components, which no layout accepts. + let layout = Layout::new(agent, workload).ok()?; + match fs::create_dir(layout.workload()) { + Ok(()) => Some(Self(layout)), + Err(error) + if matches!( + error.kind(), + io::ErrorKind::ReadOnlyFilesystem | io::ErrorKind::PermissionDenied + ) => + { + None + } + Err(error) => panic!( + "cannot create the test's cgroup {}: {error}", + layout.workload().display() + ), + } + } + } + + impl Drop for RealWorkload { + /// Kills whatever is left in the workload cgroup, then removes its cgroups. + fn drop(&mut self) { + let workload = self.0.workload(); + let _ = write(&workload.join(KILL), b"1"); + for _ in 0..1000 { + if fs::read(workload.join(PIDS_CURRENT)).is_ok_and(|count| count == b"0\n") { + break; + } + std::thread::sleep(SETTLE_INTERVAL); + } + if let Ok(entries) = fs::read_dir(workload) { + for entry in entries.flatten() { + if entry.file_type().is_ok_and(|kind| kind.is_dir()) { + let _ = fs::remove_dir(entry.path()); + } + } + } + let _ = fs::remove_dir(workload); + } + } + + /// Returns whether `reader` polls readable, with data or end of file, within `timeout`. + fn readable(reader: &std::io::PipeReader, timeout: Duration) -> bool { + let mut descriptors = [PollFd::new(reader.as_fd(), PollFlags::POLLIN)]; + nix::poll::poll(&mut descriptors, PollTimeout::try_from(timeout).unwrap()).unwrap() == 1 + } + + #[test] + fn descendants_that_hold_pipes_are_found_and_settled() { + // The test changes the real hierarchy, which needs root; elsewhere it does nothing. + let Some(workload) = RealWorkload::new("settle") else { + return; + }; + let layout = &workload.0; + let first = ExecutionCgroup::prepare(layout, Some("first"), &[("pids.max", "16")]).unwrap(); + let second = ExecutionCgroup::prepare(layout, Some("second"), &[]).unwrap(); + assert_eq!( + fs::read_to_string(first.path().join("pids.max")).unwrap(), + "16\n" + ); + assert_eq!( + ExecutionCgroup::prepare(layout, Some("first"), &[]) + .unwrap_err() + .kind(), + io::ErrorKind::AlreadyExists + ); + // A value that the kernel refuses, an empty one included, leaves no cgroup behind, so the + // name can be prepared anew. + for value in ["-1", ""] { + assert_eq!( + error(ExecutionCgroup::prepare( + layout, + Some("third"), + &[("pids.max", value)] + )), + Some(Errno::EINVAL as i32), + "{value:?}" + ); + assert!(!layout.workload().join("third").exists()); + } + ExecutionCgroup::prepare(layout, Some("third"), &[("pids.max", "8")]) + .unwrap() + .remove() + .unwrap(); + + // The first execution's program ends at once, but leaves a descendant in a session of its + // own, outside the program's process group, which holds the program's output open. + let (mut output, writer) = std::io::pipe().unwrap(); + let spawned = shell("setsid sleep 60 & echo started") + .stdout(writer) + .cgroup(first.placement().unwrap()) + .spawn() + .unwrap(); + let mut child = Child::with_pidfd(spawned.id()).unwrap(); + // A child joins its cgroup before its program runs, which its launch report waits for. + assert!(spawned.into_report().wait().unwrap().is_none()); + assert!(child.wait().unwrap().success()); + let mut started = [0u8; 8]; + output.read_exact(&mut started).unwrap(); + assert_eq!(&started, b"started\n"); + assert!(!readable(&output, Duration::from_millis(200))); + assert!(first.is_occupied(Scope::Execution).unwrap()); + + // The second execution keeps running. + let spawned = shell("exec sleep 60") + .cgroup(second.placement().unwrap()) + .spawn() + .unwrap(); + let mut sibling = Child::new(spawned.id()).unwrap(); + assert!(spawned.into_report().wait().unwrap().is_none()); + + // The first execution's cgroup finds the descendant: once it is settled, the output ends. + first.kill(Scope::Execution).unwrap(); + first + .settle(Scope::Execution, Duration::from_secs(10), || Ok(())) + .unwrap(); + assert!(readable(&output, Duration::ZERO)); + let mut rest = Vec::new(); + output.read_to_end(&mut rest).unwrap(); + assert!(rest.is_empty()); + first.remove().unwrap(); + assert!(!first.path().exists()); + assert_eq!(sibling.try_wait().unwrap(), None); + assert!(second.is_occupied(Scope::Execution).unwrap()); + + // Settling the whole workload subtree ends the sibling too. + second.kill(Scope::Workload).unwrap(); + second + .settle(Scope::Workload, Duration::from_secs(10), || { + sibling.try_wait().map(drop) + }) + .unwrap(); + assert_eq!(sibling.status().unwrap().signal(), Some(nix::libc::SIGKILL)); + second.remove().unwrap(); + } +} diff --git a/nvx-rs/src/guest/managed_agent/cgroup.rs b/nvx-rs/src/guest/managed_agent/cgroup.rs index e66cdafa5..9ed5f93f9 100644 --- a/nvx-rs/src/guest/managed_agent/cgroup.rs +++ b/nvx-rs/src/guest/managed_agent/cgroup.rs @@ -1,21 +1,46 @@ +use std::io; +use std::os::unix::process::ExitStatusExt; use std::path::Path; +use std::process::ExitStatus; use std::time::Duration; -use crate::guest::sys::{self, ChildStatus, Errno, Mode, MountFlags, OpenFlags, ProcessId}; +use crate::guest::cgroup::{ExecutionCgroup, Layout, Scope}; +use crate::guest::process::Child; +use crate::guest::spawn::Cgroup; +use crate::guest::sys::{self, Errno, Mode, MountFlags, ProcessId}; -use super::seam::{System, path_c_string}; +use super::seam::{Paths, System, path_c_string}; -/// Number of ten-millisecond retries after killing a cgroup. -const SETTLE_ATTEMPTS: usize = 200; -/// Delay between cgroup-emptiness checks. -const SETTLE_DELAY: Duration = Duration::from_millis(10); +/// How long settlement waits for the execution cgroup to empty: as long as the C agent's 200 +/// retries, ten milliseconds apart. +const SETTLE_TIMEOUT: Duration = Duration::from_secs(2); + +/// Returns the agent's cgroups in its mode. The direct agent runs in the hierarchy's root, and its +/// workloads in `nvx-exec`, which it prepares and settles whole. The sandbox agent runs in +/// `agent`, and its workloads in `container`, which the init agent prepares. +fn layout(paths: &Paths, direct: bool) -> Result { + let root = &paths.cgroup_root; + let layout = if direct { + Layout::new(root, root.join("nvx-exec")) + } else { + Layout::new(root.join("agent"), root.join("container")) + }; + layout.map_err(|error| sys::errno_of(&error)) +} + +/// Returns the handle of the direct agent's execution cgroup, `nvx-exec`, in which each workload +/// runs, one at a time. +fn execution(paths: &Paths) -> Result { + ExecutionCgroup::open(&layout(paths, true)?, None).map_err(|error| sys::errno_of(&error)) +} /// Mounts cgroup v2 when needed and prepares the per-execution cgroup. pub(crate) fn prepare(system: &mut impl System) -> Result<(), Errno> { - let root = system.paths().cgroup_root.clone(); + let layout = layout(system.paths(), true)?; + let root = layout.agent(); if std::fs::metadata(root.join("cgroup.procs")).is_err() { - mkdir(&root)?; - let target = path_c_string(&root)?; + mkdir(root)?; + let target = path_c_string(root)?; system.mount( Some(c"cgroup2"), &target, @@ -23,105 +48,69 @@ pub(crate) fn prepare(system: &mut impl System) -> Result<(), Errno> { MountFlags::MS_NOSUID | MountFlags::MS_NODEV | MountFlags::MS_NOEXEC, )?; } - let execution = root.join("nvx-exec"); - mkdir(&execution)?; - match std::fs::metadata(execution.join("pids.current")) { - Ok(_) => Ok(()), - Err(error) => match sys::errno_of(&error) { - Errno::ENOENT => write_path(&root.join("cgroup.subtree_control"), b"+pids"), - error => Err(error), - }, - } + ExecutionCgroup::prepare(&layout, None, &[]) + .map(drop) + .map_err(|error| sys::errno_of(&error)) } -/// Returns whether the execution cgroup reports one or more tasks. -pub(crate) fn has_tasks(system: &impl System) -> Result { - let path = system - .paths() - .cgroup_root - .join("nvx-exec") - .join("pids.current"); - let path = path_c_string(&path)?; - let descriptor = sys::open( - &path, - OpenFlags::O_RDONLY | OpenFlags::O_CLOEXEC, - Mode::empty(), - )?; - let mut buffer = [0u8; 32]; - let mut length = 0; - let result = loop { - match descriptor.read(&mut buffer[length..]) { - Ok(count) if count > 0 => { - length += count; - if length < buffer.len() { - continue; - } - break Err(Errno::EOVERFLOW); - } - Ok(_) => break parse_count(&buffer[..length]), - Err(Errno::EINTR) => {} - Err(error) => break Err(error), - } - }; - match result { - Ok(value) => descriptor.close().map(|()| value), - Err(error) => Err(error), - } +/// Returns the cgroup that each direct workload joins before its program runs. +pub(crate) fn placement(paths: &Paths) -> Result { + execution(paths)? + .placement() + .map_err(|error| sys::errno_of(&error)) } -/// Parses the canonical decimal form of `pids.current`. -fn parse_count(bytes: &[u8]) -> Result { - if bytes.len() < 2 - || bytes.last() != Some(&b'\n') - || (bytes.first() == Some(&b'0') && bytes.len() != 2) - || !bytes[..bytes.len() - 1].iter().all(u8::is_ascii_digit) - { - return Err(Errno::EPROTO); - } - Ok(bytes.first() != Some(&b'0')) +/// Returns whether the execution cgroup reports one or more tasks. +pub(crate) fn has_tasks(system: &impl System) -> Result { + execution(system.paths())? + .is_occupied(Scope::Workload) + .map_err(|error| sys::errno_of(&error)) } /// Requests atomic killing of every process in the execution cgroup. pub(crate) fn kill(system: &mut impl System) -> Result<(), Errno> { - let path = system - .paths() - .cgroup_root - .join("nvx-exec") - .join("cgroup.kill"); - let result = write_path(&path, b"1"); + let result = execution(system.paths()).and_then(|execution| { + execution + .kill(Scope::Workload) + .map_err(|error| sys::errno_of(&error)) + }); if let Err(error) = result { super::diagnostics::error(system, "exec-cgroup-kill", error as i32); } result } -/// Reaps every currently available child and records the workload's first terminal state. -pub(crate) fn reap(child: ProcessId, status: &mut Option) { - while let Some(observed) = sys::reap_any_nonblocking() { - if observed.process == child && status.is_none() { - *status = Some(observed.status); - } +/// Collects the status of every child that has ended, as the agent is its process's one status +/// collector, and hands the workload's status to `child`. The direct agent is PID 1, so its +/// children include every orphaned descendant of its workloads. +pub(crate) fn reap(child: &mut Child) { + while let Some(reaped) = sys::reap_any_nonblocking() { + child.observe( + reaped.process.raw().unsigned_abs(), + ExitStatus::from_raw(reaped.wait_status), + ); } } +/// Collects the status of every child that has ended once the workload's status is known, when +/// every child left is an orphaned descendant. +fn reap_orphans() -> io::Result<()> { + while sys::reap_any_nonblocking().is_some() {} + Ok(()) +} + /// Reaps orphaned descendants and returns the cgroup's current occupancy. pub(crate) fn verify_empty(system: &impl System) -> Result { - while sys::reap_any_nonblocking().is_some() {} + let _ = reap_orphans(); has_tasks(system) } /// Kills and reaps the execution cgroup until it is empty or the deadline expires. pub(crate) fn settle(system: &mut impl System) -> Result<(), Errno> { kill(system)?; - for attempt in 0..=SETTLE_ATTEMPTS { - if !verify_empty(system)? { - return Ok(()); - } - if attempt != SETTLE_ATTEMPTS { - std::thread::sleep(SETTLE_DELAY); - } - } - Err(Errno::ETIMEDOUT) + execution(system.paths())? + .settle(Scope::Workload, SETTLE_TIMEOUT, reap_orphans) + .map_err(|error| sys::errno_of(&error)) } /// Kills the workload process group and, in direct mode, its cgroup. @@ -134,7 +123,9 @@ pub(crate) fn terminate(system: &mut impl System, direct: bool, child: ProcessId /// Moves a sandbox launcher into the container cgroup. pub(crate) fn write_container_pid(system: &impl System, child: ProcessId) -> Result<(), Errno> { - write_path(&system.paths().container_cgroup_procs, &child.line()) + ExecutionCgroup::open(&layout(system.paths(), false)?, None) + .and_then(|container| container.place(child.raw().unsigned_abs())) + .map_err(|error| sys::errno_of(&error)) } /// Creates one cgroup directory with mode 0755, accepting an existing path. @@ -146,18 +137,6 @@ fn mkdir(path: &Path) -> Result<(), Errno> { } } -/// Writes a complete control value and reports explicit close failure. -fn write_path(path: &Path, bytes: &[u8]) -> Result<(), Errno> { - let path = path_c_string(path)?; - let descriptor = sys::open( - &path, - OpenFlags::O_WRONLY | OpenFlags::O_CLOEXEC, - Mode::empty(), - )?; - descriptor.write_all(bytes)?; - descriptor.close() -} - #[cfg(test)] mod tests { use std::process::Command; diff --git a/nvx-rs/src/guest/managed_agent/exec.rs b/nvx-rs/src/guest/managed_agent/exec.rs index 414a56373..71646aa80 100644 --- a/nvx-rs/src/guest/managed_agent/exec.rs +++ b/nvx-rs/src/guest/managed_agent/exec.rs @@ -1,9 +1,9 @@ +use std::os::unix::process::ExitStatusExt; use std::time::{Duration, Instant}; +use crate::guest::process::Child; use crate::guest::spawn::{LaunchReport, LaunchStage}; -use crate::guest::sys::{ - self, ChildStatus, Errno, FileDescriptor, Mode, OpenFlags, ProcessId, c_string, -}; +use crate::guest::sys::{self, Errno, FileDescriptor, Mode, OpenFlags, ProcessId, c_string}; use crate::protocol::{ self, APP_CANCEL, APP_EXIT, APP_STDERR, APP_STDOUT, BUSY, CWD_FAILED, GuestExecPayload, LAUNCH_FAILED, OUTER_DATA, @@ -68,7 +68,7 @@ pub(crate) fn run( } let Ok(Launched { - child, + mut child, stdout, stderr, report, @@ -80,12 +80,12 @@ pub(crate) fn run( let mut launch_failed = tty::make_nonblocking(&stdout).is_err() || tty::make_nonblocking(&stderr).is_err(); if !launch_failed && !config.direct { - launch_failed = cgroup::write_container_pid(system, child).is_err() - || release_barrier(system, child).is_err(); + launch_failed = cgroup::write_container_pid(system, child.process()).is_err() + || release_barrier(system, child.process()).is_err(); } if launch_failed { - cgroup::terminate(system, config.direct, child); - let _ = sys::wait(child); + cgroup::terminate(system, config.direct, child.process()); + let _ = child.wait(); unlink_barrier(system); return session.send_error(request_id, 125, LAUNCH_FAILED); } @@ -100,9 +100,8 @@ pub(crate) fn run( let mut cancelled = false; let mut session_lost = false; let mut stragglers_killed = false; - let mut wait_status = None; - while wait_status.is_none() || stdout_pipe.is_some() || stderr_pipe.is_some() { + while child.status().is_none() || stdout_pipe.is_some() || stderr_pipe.is_some() { let ready = match sys::readable_or_hung_up( [ stdout_pipe.as_ref(), @@ -114,7 +113,7 @@ pub(crate) fn run( Ok(ready) => ready, Err(Errno::EINTR) => [false; 3], Err(_) => { - cgroup::terminate(system, config.direct, child); + cgroup::terminate(system, config.direct, child.process()); [false; 3] } }; @@ -122,21 +121,21 @@ pub(crate) fn run( let control = match handle_control(system, session, request_id) { Ok(control) => control, Err(error) => { - cgroup::terminate(system, config.direct, child); - let _ = sys::wait(child); + cgroup::terminate(system, config.direct, child.process()); + let _ = child.wait(); return Err(error); } }; match control { ExecControl::None => {} - ExecControl::Cancel if wait_status.is_none() && !cancelled => { + ExecControl::Cancel if child.status().is_none() && !cancelled => { cancelled = true; - cgroup::terminate(system, config.direct, child); + cgroup::terminate(system, config.direct, child.process()); } ExecControl::Cancel => {} ExecControl::SessionLost => { session_lost = true; - cgroup::terminate(system, config.direct, child); + cgroup::terminate(system, config.direct, child.process()); } } } @@ -156,10 +155,10 @@ pub(crate) fn run( if stdout_result.is_failure() || stderr_result.is_failure() { output_limited = stdout_result == StreamResult::Limited || stderr_result == StreamResult::Limited; - cgroup::terminate(system, config.direct, child); + cgroup::terminate(system, config.direct, child.process()); } - cgroup::reap(child, &mut wait_status); - if wait_status.is_some() + cgroup::reap(&mut child); + if child.status().is_some() && config.direct && !stragglers_killed && (stdout_pipe.is_some() || stderr_pipe.is_some()) @@ -167,18 +166,15 @@ pub(crate) fn run( stragglers_killed = true; let _ = cgroup::kill(system); } - if wait_status.is_none() + if child.status().is_none() && execution.timeout_ms != 0 && started.elapsed() >= Duration::from_millis(u64::from(execution.timeout_ms)) { timed_out = true; - cgroup::terminate(system, config.direct, child); + cgroup::terminate(system, config.direct, child.process()); } - if (timed_out || output_limited || cancelled || session_lost) - && wait_status.is_none() - && let Some(status) = sys::wait(child) - { - wait_status = Some(status); + if (timed_out || output_limited || cancelled || session_lost) && child.status().is_none() { + let _ = child.wait(); } } @@ -228,24 +224,24 @@ pub(crate) fn run( } else if output_limited { session.send_app(APP_EXIT, request_id, 125, b"output-limit") } else { - match wait_status { - Some(ChildStatus::Exited(status)) => { - session.send_app(APP_EXIT, request_id, status, b"exit") - } - Some(ChildStatus::Signaled(signal)) => { - session.send_app(APP_EXIT, request_id, 128 + signal, b"signal") - } - Some(ChildStatus::Other) | None => { - session.send_app(APP_EXIT, request_id, 125, b"failed") - } + match child.status() { + Some(status) => match (status.code(), status.signal()) { + (Some(code), _) => session.send_app(APP_EXIT, request_id, code, b"exit"), + (None, Some(signal)) => { + session.send_app(APP_EXIT, request_id, 128 + signal, b"signal") + } + (None, None) => session.send_app(APP_EXIT, request_id, 125, b"failed"), + }, + None => session.send_app(APP_EXIT, request_id, 125, b"failed"), } } } /// A spawned workload and the parent's ends of its pipes. struct Launched { - /// The workload process, which leads its own process group. - child: ProcessId, + /// The workload process, which leads its own process group, and whose status the agent + /// collects. + child: Child, /// Reader of the workload's standard output. stdout: FileDescriptor, /// Reader of the workload's standard error. @@ -278,7 +274,7 @@ fn launch( )?; let spawned = system.spawn(command)?; Ok(Launched { - child: spawned.process(), + child: Child::from_process(spawned.process()), stdout, stderr, report: spawned.into_report(), diff --git a/nvx-rs/src/guest/managed_agent/launch.rs b/nvx-rs/src/guest/managed_agent/launch.rs index ad17faa87..70ee980b0 100644 --- a/nvx-rs/src/guest/managed_agent/launch.rs +++ b/nvx-rs/src/guest/managed_agent/launch.rs @@ -1,7 +1,7 @@ use std::os::unix::ffi::OsStrExt; use crate::guest::parse::parse_decimal; -use crate::guest::spawn::{Cgroup, Command}; +use crate::guest::spawn::Command; use crate::guest::sys::{self, Errno, FileDescriptor, Mode, OpenFlags, c_string}; use crate::protocol::GuestExecPayload; @@ -97,8 +97,7 @@ pub(crate) fn command(paths: &Paths, request: LaunchRequest<'_>) -> Result Self { Self { cgroup_root: "/sys/fs/cgroup".into(), - container_cgroup_procs: "/sys/fs/cgroup/container/cgroup.procs".into(), hostfs_dir: "/run/nvx/hostfs".into(), proc_cmdline: "/proc/cmdline".into(), diagnostic_port: "/dev/port".into(), diff --git a/nvx-rs/src/guest/mod.rs b/nvx-rs/src/guest/mod.rs index be9a6d16c..2d587ee9d 100644 --- a/nvx-rs/src/guest/mod.rs +++ b/nvx-rs/src/guest/mod.rs @@ -1,10 +1,12 @@ -//! The programs that run in NVX's Linux guests, which ship as `/sbin/nvx-*`, and two libraries: -//! [`nvxs`], the guest's I/O driver of the NVXS session, which guest agents call, and [`spawn`], -//! through which a guest init or agent starts a program. +//! The programs that run in NVX's Linux guests, which ship as `/sbin/nvx-*`, and the libraries +//! on which any guest init or agent builds: [`nvxs`], the guest's I/O driver of the NVXS session; +//! [`spawn`], through which it starts a program; [`process`], through which it collects the +//! status of each child that it names, and of no other; and [`cgroup`], through which it places, +//! accounts, kills, and settles the processes of its executions. //! //! Each other module holds one program, and the crate's binary of the same name runs its `main`. -//! [`spawn`] needs none of NVX's control protocol, managed agent, or workload identity, and NVX's -//! managed agent starts its workloads through it. +//! The libraries need none of NVX's control protocol, managed agent, or workload identity, and +//! NVX's managed agent starts, collects, and settles its workloads through them. //! //! The programs keep the command lines, exit statuses, and output of the C programs that they //! replace, except that numeric arguments are plain decimal, with a `-` sign where `nvx-time`'s @@ -62,6 +64,7 @@ mod sys; #[cfg(target_arch = "x86_64")] mod kernel_text; +pub mod cgroup; pub mod console_pending; #[cfg(target_arch = "x86_64")] pub mod device_io; @@ -72,6 +75,7 @@ pub mod managed_agent; #[cfg(target_arch = "x86_64")] pub mod mmio_write; pub mod nvxs; +pub mod process; pub mod reseed; pub mod spawn; #[cfg(target_arch = "x86_64")] diff --git a/nvx-rs/src/guest/process.rs b/nvx-rs/src/guest/process.rs new file mode 100644 index 000000000..162454837 --- /dev/null +++ b/nvx-rs/src/guest/process.rs @@ -0,0 +1,420 @@ +//! Collecting the statuses of the processes that a guest init or agent starts. +//! +//! A process has one status collector: the caller. Nothing in the guest libraries waits for an +//! arbitrary child, which could take the status of a child that another part of the caller +//! started. A [`Child`] waits only for the child that it names, with `waitid` on the child's +//! pidfd or on its process ID, and leaves the status of every other child to the caller. +//! +//! A caller that must collect the status of every child, such as a PID 1 that adopts orphaned +//! processes, keeps its own `waitpid(-1)` loop instead, and hands each status that it collects to +//! the [`Child`] that it belongs to with [`Child::observe`]: +//! +//! ```no_run +//! use std::process::ExitStatus; +//! +//! use nvx::guest::process::Child; +//! use nvx::guest::spawn::Command; +//! +//! /// The caller's own `waitpid(-1, WNOHANG)` loop, which returns the ID and the status of one +//! /// child that has ended. +//! fn reap_any() -> Option<(u32, ExitStatus)> { +//! # unimplemented!() +//! } +//! +//! # fn main() -> std::io::Result<()> { +//! let spawned = Command::new(c"/bin/true".to_owned(), vec![c"true".to_owned()]).spawn()?; +//! let mut child = Child::new(spawned.id())?; +//! while child.status().is_none() { +//! if let Some((id, status)) = reap_any() { +//! child.observe(id, status); +//! } +//! } +//! # Ok(()) +//! # } +//! ``` +//! +//! Any other caller lets each [`Child`] collect its own status, for example once its +//! [pidfd](Child::pidfd) polls readable: +//! +//! ```no_run +//! use nvx::guest::process::Child; +//! use nvx::guest::spawn::Command; +//! +//! # fn main() -> std::io::Result<()> { +//! let spawned = Command::new(c"/bin/true".to_owned(), vec![c"true".to_owned()]).spawn()?; +//! let mut child = Child::with_pidfd(spawned.id())?; +//! // Poll `child.pidfd()` with the caller's other descriptors until it is readable, then: +//! let status = child.wait()?; +//! assert!(status.success()); +//! # Ok(()) +//! # } +//! ``` + +use std::io; +use std::os::fd::{AsFd, BorrowedFd, OwnedFd}; +use std::os::unix::process::ExitStatusExt; +use std::process::ExitStatus; + +use super::sys::{self, Errno, ProcessId, WaitTarget}; + +/// A child process that the caller started and whose status it collects. +/// +/// Unlike [`std::process::Child`], it neither starts the process nor holds its standard streams: +/// it names a child of the caller, for example one that [`spawn`](super::spawn) started, and +/// keeps its status once it has one. Dropping it neither waits for the child nor stops it. +/// +/// Its status is the one with which it ended: an exit, or a terminating signal. A stop does not +/// end it, not even one that a wait reports to the caller as the child's tracer. +#[derive(Debug)] +pub struct Child { + /// The child's process ID. + process: ProcessId, + /// A pidfd that refers to the child, through which waits name it. + pidfd: Option, + /// The status with which the child ended, once a wait or an observation collected it. + status: Option, +} + +impl Child { + /// Names the caller's child `id`, whose status nothing has collected yet. Its waits use + /// `waitid` on the process ID, which names the child for as long as its status stays + /// uncollected: a caller with a collector of its own hands the child every status that + /// the collector takes for `id`, with [`observe`](Self::observe), before the child waits, as + /// the process ID may name another process afterwards. A pidfd keeps naming the child + /// whatever collects its status; see [`with_pidfd`](Self::with_pidfd). + /// + /// An ID of 0, or one above `i32::MAX`, which no process can have, is `EINVAL`. + pub fn new(id: u32) -> io::Result { + Ok(Self { + process: process_id(id)?, + pidfd: None, + status: None, + }) + } + + /// Names the caller's child `id`, whose status nothing has collected yet, by a new pidfd, + /// which [`pidfd`](Self::pidfd) lends for polling: it polls readable once the child ends. + /// Its waits use `waitid` on the pidfd, which keeps naming the child even after another + /// collector takes its status. The pidfd is opened at once, while the process ID still names + /// the child. + /// + /// An ID of 0, or one above `i32::MAX`, is `EINVAL`, and one that names no process `ESRCH`. + pub fn with_pidfd(id: u32) -> io::Result { + let process = process_id(id)?; + let pidfd = sys::open_pidfd(process)?; + Ok(Self { + process, + pidfd: Some(pidfd.into()), + status: None, + }) + } + + /// Names the crate's own child, for the callers that hold its [`ProcessId`]. + pub(crate) fn from_process(process: ProcessId) -> Self { + Self { + process, + pidfd: None, + status: None, + } + } + + /// Returns the child's process ID. + pub fn id(&self) -> u32 { + self.process.raw().unsigned_abs() + } + + /// Returns the child's process ID for the crate's own callers. + pub(crate) fn process(&self) -> ProcessId { + self.process + } + + /// Lends the child's pidfd, if [`with_pidfd`](Self::with_pidfd) opened one, which polls + /// readable once the child ends. + pub fn pidfd(&self) -> Option> { + self.pidfd.as_ref().map(AsFd::as_fd) + } + + /// Returns the status with which the child ended, once a wait or + /// [`observe`](Self::observe) collected it. + pub fn status(&self) -> Option { + self.status + } + + /// Hands the child the status that the caller's own collector, such as its `waitpid(-1)` + /// loop, collected for the process `id`, and returns whether it was this child's final + /// status, which the child then keeps. + /// + /// Only a status with which a process ended counts: an exit, or a terminating signal. A stop + /// or a continuation, which a collector that waits with `WUNTRACED` or `WCONTINUED` reports + /// while the process lives, is not recorded, and this returns `false`. Only the first final + /// status counts too: once the child has one, a later status for its ID belongs to another + /// process that the ID was reused for, and this returns `false`. + pub fn observe(&mut self, id: u32, status: ExitStatus) -> bool { + let ended = status.code().is_some() || status.signal().is_some(); + if !ended || self.status.is_some() || id != self.id() { + return false; + } + self.status = Some(status); + true + } + + /// Collects the child's status if it has ended, without blocking, and returns it, or `None` + /// while the child runs. Once the child has a status, this returns it again. + /// + /// It collects no other child's status. A child that a pidfd names, and whose status another + /// collector took without handing it over with [`observe`](Self::observe), is `ECHILD`. + pub fn try_wait(&mut self) -> io::Result> { + self.collect(false) + } + + /// Waits until the child ends, collects its status, and returns it. Once the child has a + /// status, this returns it again at once. + /// + /// It collects no other child's status. A child that a pidfd names, and whose status another + /// collector took without handing it over with [`observe`](Self::observe), is `ECHILD`. + pub fn wait(&mut self) -> io::Result { + self.collect(true)? + .ok_or_else(|| io::Error::from(Errno::ECHILD)) + } + + /// Returns the child's status, which a wait for this child alone collects if the child has + /// none yet, blocking until it ends when `block` is set. + fn collect(&mut self, block: bool) -> io::Result> { + if self.status.is_none() { + let target = match &self.pidfd { + Some(pidfd) => WaitTarget::Pidfd(pidfd.as_fd()), + None => WaitTarget::Process(self.process), + }; + self.status = sys::wait_named(target, block)?.map(ExitStatus::from_raw); + } + Ok(self.status) + } +} + +/// Returns the process ID `id`, or `EINVAL` for one that no process can have. +fn process_id(id: u32) -> io::Result { + ProcessId::from_positive(i64::from(id)).ok_or_else(|| io::Error::from(Errno::EINVAL)) +} + +#[cfg(test)] +mod tests { + use std::ffi::CString; + use std::time::Duration; + + use nix::poll::{PollFd, PollFlags, PollTimeout}; + + use super::*; + use crate::guest::spawn::Command; + + /// Spawns `/bin/sh -c script` and returns its process ID. + fn spawn_shell(script: &str) -> u32 { + Command::new( + c"/bin/sh".to_owned(), + vec![ + c"sh".to_owned(), + c"-c".to_owned(), + CString::new(script).unwrap(), + ], + ) + .spawn() + .unwrap() + .id() + } + + /// Returns whether `pidfd` polls readable within `timeout`. + fn readable(pidfd: BorrowedFd<'_>, timeout: Duration) -> bool { + let mut descriptors = [PollFd::new(pidfd, PollFlags::POLLIN)]; + let timeout = PollTimeout::try_from(timeout).unwrap(); + nix::poll::poll(&mut descriptors, timeout).unwrap() == 1 + } + + /// Spawns a child that ends at once, and waits until it has ended without collecting its + /// status. + fn unrelated_zombie() -> std::process::Child { + let child = std::process::Command::new("/bin/true").spawn().unwrap(); + let process = ProcessId::from_child(&child); + while !sys::has_exited(process).unwrap() { + std::thread::yield_now(); + } + child + } + + #[test] + fn a_named_child_s_status_is_collected_and_no_other() { + let mut unrelated = unrelated_zombie(); + for (script, code, signal) in [ + ("exit 3", Some(3), None), + ("kill -TERM $$", None, Some(nix::libc::SIGTERM)), + // A real-time signal, which `nix`'s wait decoding cannot represent. + ("kill -40 $$", None, Some(40)), + ] { + for with_pidfd in [false, true] { + let id = spawn_shell(script); + let mut child = if with_pidfd { + Child::with_pidfd(id).unwrap() + } else { + Child::new(id).unwrap() + }; + assert_eq!(child.id(), id); + assert_eq!(child.pidfd().is_some(), with_pidfd); + let status = child.wait().unwrap(); + assert_eq!((status.code(), status.signal()), (code, signal), "{script}"); + // The status stays, and no further wait takes place. + assert_eq!(child.try_wait().unwrap(), Some(status)); + assert_eq!(child.wait().unwrap(), status); + assert_eq!(child.status(), Some(status)); + } + } + // Every wait named its child, so the unrelated child's status is still there. + assert!(unrelated.try_wait().unwrap().unwrap().success()); + } + + #[test] + fn a_pidfd_polls_readable_once_its_child_ends() { + let (reader, mut writer) = std::io::pipe().unwrap(); + let spawned = Command::new( + c"/bin/sh".to_owned(), + vec![c"sh".to_owned(), c"-c".to_owned(), c"read line".to_owned()], + ) + .stdin(reader) + .spawn() + .unwrap(); + let mut child = Child::with_pidfd(spawned.id()).unwrap(); + let pidfd = child.pidfd().unwrap().try_clone_to_owned().unwrap(); + assert!(!readable(pidfd.as_fd(), Duration::ZERO)); + assert_eq!(child.try_wait().unwrap(), None); + std::io::Write::write_all(&mut writer, b"go\n").unwrap(); + assert!(readable(pidfd.as_fd(), Duration::from_secs(10))); + assert!(child.try_wait().unwrap().unwrap().success()); + } + + #[test] + fn an_observed_status_belongs_to_its_child_alone() { + // The test collects the child's status itself, as a PID 1's own loop would. + let mut process = std::process::Command::new("/bin/sh") + .args(["-c", "exit 7"]) + .spawn() + .unwrap(); + let id = process.id(); + let collected = process.wait().unwrap(); + let mut child = Child::new(id).unwrap(); + assert!(!child.observe(id + 1, ExitStatus::from_raw(0))); + assert_eq!(child.status(), None); + assert!(child.observe(id, collected)); + // A later status for the ID belongs to another process. + assert!(!child.observe(id, ExitStatus::from_raw(0))); + assert_eq!(child.status().unwrap().code(), Some(7)); + // The observed status answers the waits, which would otherwise find no child. + assert_eq!(child.try_wait().unwrap(), Some(collected)); + assert_eq!(child.wait().unwrap(), collected); + } + + #[test] + fn stops_and_continuations_do_not_end_an_observed_child() { + // The statuses that `waitpid` reports with `WUNTRACED` and `WCONTINUED` for a child that + // lives on. + let stopped = ExitStatus::from_raw((nix::libc::SIGSTOP << 8) | 0x7f); + let continued = ExitStatus::from_raw(0xffff); + assert_eq!(stopped.stopped_signal(), Some(nix::libc::SIGSTOP)); + assert!(continued.continued()); + let mut child = Child::new(4242).unwrap(); + for status in [stopped, continued] { + assert!(!child.observe(4242, status), "{status:?}"); + assert_eq!(child.status(), None); + } + // The final status still counts. + let killed = ExitStatus::from_raw(nix::libc::SIGKILL); + assert!(child.observe(4242, killed)); + assert_eq!(child.status().unwrap().signal(), Some(nix::libc::SIGKILL)); + assert!(!child.observe(4242, stopped)); + assert_eq!(child.status(), Some(killed)); + } + + /// Returns whether the process `id` reaches a tracing stop within ten seconds, or `false` once + /// it has ended instead. + fn stops_for_its_tracer(id: u32) -> bool { + let deadline = std::time::Instant::now() + Duration::from_secs(10); + loop { + let stat = std::fs::read_to_string(format!("/proc/{id}/stat")).unwrap(); + // The state follows the command name, in parentheses, which may hold any character. + match stat + .rsplit_once(") ") + .and_then(|(_, rest)| rest.chars().next()) + { + Some('t') => return true, + Some('Z') => return false, + _ => {} + } + assert!( + std::time::Instant::now() < deadline, + "{id} neither stopped nor ended" + ); + std::thread::sleep(Duration::from_millis(5)); + } + } + + #[test] + fn a_traced_child_s_stop_does_not_end_it() { + for with_pidfd in [false, true] { + let id = sys::spawn_traced_child(7).raw().unsigned_abs(); + let mut child = if with_pidfd { + Child::with_pidfd(id) + } else { + Child::new(id) + } + .unwrap(); + if !stops_for_its_tracer(id) { + // The system refuses tracing, which leaves nothing to check. + assert_eq!(child.wait().unwrap().code(), Some(99)); + return; + } + // A wait reports the stop to the child's tracer whatever its options, but the child + // still runs. + assert_eq!(child.try_wait().unwrap(), None); + assert_eq!(child.status(), None); + sys::continue_traced(child.process()); + assert_eq!(child.wait().unwrap().code(), Some(7)); + } + // A blocking wait goes on past the stop until the child ends, here by `SIGKILL`. + let id = sys::spawn_traced_child(7).raw().unsigned_abs(); + let mut child = Child::new(id).unwrap(); + assert!(stops_for_its_tracer(id)); + let killer = std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(200)); + let process = nix::unistd::Pid::from_raw(i32::try_from(id).unwrap()); + nix::sys::signal::kill(process, nix::sys::signal::Signal::SIGKILL).unwrap(); + }); + assert_eq!(child.wait().unwrap().signal(), Some(nix::libc::SIGKILL)); + killer.join().unwrap(); + } + + #[test] + fn a_child_whose_status_another_collector_took_is_echild() { + let mut process = std::process::Command::new("/bin/true").spawn().unwrap(); + let mut child = Child::with_pidfd(process.id()).unwrap(); + // Another collector takes the status, and hands it over to no one. + process.wait().unwrap(); + assert_eq!( + child.try_wait().unwrap_err().raw_os_error(), + Some(Errno::ECHILD as i32) + ); + assert_eq!( + child.wait().unwrap_err().raw_os_error(), + Some(Errno::ECHILD as i32) + ); + assert_eq!(child.status(), None); + } + + #[test] + fn ids_that_no_process_can_have_are_refused() { + for id in [0, i32::MAX.unsigned_abs() + 1, u32::MAX] { + for result in [Child::new(id), Child::with_pidfd(id)] { + assert_eq!( + result.unwrap_err().raw_os_error(), + Some(Errno::EINVAL as i32), + "{id}" + ); + } + } + } +} diff --git a/nvx-rs/src/guest/spawn.rs b/nvx-rs/src/guest/spawn.rs index e4803e426..3955dfd31 100644 --- a/nvx-rs/src/guest/spawn.rs +++ b/nvx-rs/src/guest/spawn.rs @@ -21,9 +21,10 @@ //! the caller chooses the program, which applies its own credentials, and the identity, if any, //! as which the child checks its working directory. NVX's managed agent is one caller. //! -//! The caller owns the child. [`Spawned::id`] names it, and the caller collects its status: this -//! module never waits for a process. [`LaunchReport::wait`] tells whether the program started, or -//! which stage failed and why. +//! The caller owns the child. [`Spawned::id`] names it, and the caller collects its status, for +//! example through a [`process::Child`](super::process::Child): this module never waits for a +//! process. [`LaunchReport::wait`] tells whether the program started, or which stage failed and +//! why. //! //! ```no_run //! use std::fs::File; diff --git a/nvx-rs/src/guest/sys/mod.rs b/nvx-rs/src/guest/sys/mod.rs index 1071b094d..7a5ad0cac 100644 --- a/nvx-rs/src/guest/sys/mod.rs +++ b/nvx-rs/src/guest/sys/mod.rs @@ -22,13 +22,16 @@ //! The spawn boundary and the managed agent need a few operations that `nix` cannot provide, //! which reach libc through `nix`'s re-export: a spawned child, which may neither allocate nor //! drop an owner, fills its preallocated `PWD` entry with the `getcwd` system call, and executes -//! and exits through libc; the agent's wait-status decoding keeps the real-time signals that -//! `nix`'s `Signal` cannot represent; and its execution helper duplicates the descriptor that it -//! inherits by number, which it cannot borrow before it knows that the descriptor is open. So do -//! the time tools, for the calls that `nix` does not wrap: `ioperm`, `adjtimex`, `klogctl`, -//! `getauxval`, `sigqueue`, and `_exit`; `sched_setscheduler`, which musl does not implement for -//! one thread; and the `clock_gettime` and `getpid` system calls, which the latency probe -//! measures without the vDSO's or libc's shortcuts. Each such call says why. +//! and exits through libc; the agent's wait-status decoding, and the waits for one named child, +//! keep the real-time signals that `nix`'s `Signal` cannot represent; a pidfd is opened with the +//! `pidfd_open` system call, which neither `nix` nor musl wraps; and the agent's execution helper +//! duplicates the descriptor that it inherits by number, which it cannot borrow before it knows +//! that the descriptor is open. So do the time tools, for the calls that `nix` does not wrap: +//! `ioperm`, `adjtimex`, `klogctl`, `getauxval`, `sigqueue`, and `_exit`; `sched_setscheduler`, +//! which musl does not implement for one thread; and the `clock_gettime` and `getpid` system +//! calls, which the latency probe measures without the vDSO's or libc's shortcuts. The tests +//! trace a child with libc's `ptrace`, as the crate does not enable `nix`'s. Each such call says +//! why. //! //! The x86 instructions of the time tools are `core::arch` intrinsics, except the port //! instructions, which only `port` executes, as inline assembly. The constants of the kernel's @@ -91,11 +94,14 @@ pub(crate) use process::sigpipe_disposition; #[cfg(target_arch = "x86_64")] pub(crate) use process::syscall_process_id; pub(crate) use process::{ - ChildStatus, DirectStdout, ForkResult, ProcessId, Stdout, current_process, detach, exec, - exit_immediately, fork, has_exited, ignore_interrupt_and_hangup, kill_process_group, - process_exists, process_name, reap_any_nonblocking, restore_sigpipe, take_inherited_descriptor, - try_wait, wait, with_sigpipe_ignored, write_port_diagnostic, write_standard_error, + ChildStatus, DirectStdout, ForkResult, ProcessId, Stdout, WaitTarget, current_process, detach, + exec, exit_immediately, fork, has_exited, ignore_interrupt_and_hangup, kill_process_group, + open_pidfd, process_exists, process_name, reap_any_nonblocking, restore_sigpipe, + take_inherited_descriptor, try_wait, wait, wait_named, with_sigpipe_ignored, + write_port_diagnostic, write_standard_error, }; +#[cfg(test)] +pub(crate) use process::{continue_traced, spawn_traced_child}; pub(crate) use scheduler::{ AffinityGuard, IdlePriority, SchedulingPolicy, current_affinity, current_cpu, current_thread, pin_current_thread, promote_all_threads, scheduling_policy, set_scheduling_policy, diff --git a/nvx-rs/src/guest/sys/process.rs b/nvx-rs/src/guest/sys/process.rs index 77e8bff5a..c80879073 100644 --- a/nvx-rs/src/guest/sys/process.rs +++ b/nvx-rs/src/guest/sys/process.rs @@ -2,7 +2,7 @@ use std::ffi::{CStr, CString}; use std::io::{self, Read, Write}; -use std::os::fd::RawFd; +use std::os::fd::{AsRawFd, BorrowedFd, RawFd}; use std::sync::atomic::{AtomicBool, Ordering}; use nix::sys::signal::{SaFlags, SigAction, SigHandler, SigSet, Signal, kill}; @@ -222,11 +222,13 @@ pub(crate) struct ReapedChild { pub(crate) process: ProcessId, /// Terminal state reported by the kernel. pub(crate) status: ChildStatus, + /// The raw wait status that `waitpid` reported, from which `status` is decoded. + pub(crate) wait_status: i32, } /// Decodes Linux's raw wait status without narrowing real-time signal numbers. fn child_status(process: nix::libc::pid_t, status: nix::libc::c_int) -> ReapedChild { - let status = if nix::libc::WIFEXITED(status) { + let decoded = if nix::libc::WIFEXITED(status) { ChildStatus::Exited(nix::libc::WEXITSTATUS(status)) } else if nix::libc::WIFSIGNALED(status) { ChildStatus::Signaled(nix::libc::WTERMSIG(status)) @@ -235,12 +237,18 @@ fn child_status(process: nix::libc::pid_t, status: nix::libc::c_int) -> ReapedCh }; ReapedChild { process: ProcessId::from_pid(Pid::from_raw(process)), - status, + status: decoded, + wait_status: status, } } /// Reaps one available child without blocking. /// +/// Only a program that is its process's one status collector calls this: the managed agent, which +/// as the direct agent is PID 1 and also collects its workloads' orphaned descendants, and the +/// time daemon. The guest libraries wait only for the child that the caller names, with +/// [`wait_named`]. +/// /// libc is used instead of `nix` because `nix` converts a terminating signal to its finite /// `Signal` enum and rejects Linux real-time signal numbers, after the child is reaped. /// Interruptions are retried. `ECHILD`, a running child, and other errors all mean that no @@ -300,6 +308,131 @@ pub(crate) fn kill_process_group(process: ProcessId) { let _ = kill(Pid::from_raw(-process.0.as_raw()), Signal::SIGKILL); } +/// One child that a wait names, which is all that it may collect. +#[derive(Clone, Copy, Debug)] +pub(crate) enum WaitTarget<'a> { + /// The child with this process ID. + Process(ProcessId), + /// The child to which this pidfd refers. + Pidfd(BorrowedFd<'a>), +} + +/// Collects the status of the child that `target` names once it has ended, and returns its raw +/// wait status, in the form that `waitpid` reports. Without `block`, it returns `None` while the +/// child runs. +/// +/// Only an exit or a terminating signal ends the child. `waitid` also reports each stop of a +/// child that this process traces, whatever its options, which leaves the child running: a +/// blocking wait goes on past such a stop, and one without `block` returns `None`. +/// +/// Only the named child's status is collected, so every other child's stays for its collector. +/// Interruptions are retried. A target that is not a child of this process, or whose status was +/// collected already, is `ECHILD`. +/// +/// libc is used because `nix`'s `waitid` decodes the state into a `WaitStatus`, which fails for a +/// real-time signal after the child is reaped. The raw status is rebuilt from the child event's +/// code and status, which keeps the signal number and the core dump. +pub(crate) fn wait_named(target: WaitTarget<'_>, block: bool) -> Result, Errno> { + let (kind, id) = match target { + WaitTarget::Process(process) => (nix::libc::P_PID, process.0.as_raw() as nix::libc::id_t), + WaitTarget::Pidfd(descriptor) => ( + nix::libc::P_PIDFD, + descriptor.as_raw_fd() as nix::libc::id_t, + ), + }; + let options = if block { + nix::libc::WEXITED + } else { + nix::libc::WEXITED | nix::libc::WNOHANG + }; + loop { + // SAFETY: zero is a valid initial representation for `siginfo_t`, and leaves `si_pid` zero + // unless a child event fills it. + let mut information = unsafe { std::mem::zeroed::() }; + // SAFETY: `information` is writable, and the call collects at most the named child. + let result = unsafe { nix::libc::waitid(kind, id, &mut information, options) }; + if result < 0 { + match Errno::last() { + Errno::EINTR => continue, + error => return Err(error), + } + } + // SAFETY: a successful `waitid` either initialized the Linux child-event fields, or, for a + // child that still runs, left the zeroed `si_pid`. + let (process, status) = unsafe { (information.si_pid(), information.si_status()) }; + if process == 0 { + return Ok(None); + } + return Ok(Some(match information.si_code { + nix::libc::CLD_EXITED => (status & 0xff) << 8, + nix::libc::CLD_KILLED => status & 0x7f, + nix::libc::CLD_DUMPED => (status & 0x7f) | 0x80, + // A traced child's stop, which does not end it. + _ if block => continue, + _ => return Ok(None), + })); + } +} + +/// Forks a child that makes the calling thread its tracer, stops with `SIGSTOP`, and exits with +/// `status` once its tracer continues it, or with 99 if it cannot be traced. +/// +/// libc's `ptrace` is used because the crate does not enable `nix`'s. +#[cfg(test)] +pub(crate) fn spawn_traced_child(status: u8) -> ProcessId { + // SAFETY: the child makes only the async-signal-safe `ptrace`, `raise`, and `_exit` calls, + // as a child of a multithreaded process may. + match unsafe { nix::unistd::fork() }.expect("fork") { + nix::unistd::ForkResult::Child => { + // SAFETY: `PTRACE_TRACEME` reads none of the other arguments. + let traced = unsafe { + nix::libc::ptrace( + nix::libc::PTRACE_TRACEME, + 0, + std::ptr::null_mut::(), + std::ptr::null_mut::(), + ) + }; + if traced != 0 { + exit_immediately(99); + } + let _ = nix::sys::signal::raise(Signal::SIGSTOP); + exit_immediately(status) + } + nix::unistd::ForkResult::Parent { child } => ProcessId::from_pid(child), + } +} + +/// Continues `process`, which the calling thread traces and which is stopped, without a signal. +#[cfg(test)] +pub(crate) fn continue_traced(process: ProcessId) { + // SAFETY: `PTRACE_CONT` reads the process ID, and a null signal, which delivers none. + let result = unsafe { + nix::libc::ptrace( + nix::libc::PTRACE_CONT, + process.0.as_raw(), + std::ptr::null_mut::(), + std::ptr::null_mut::(), + ) + }; + assert_eq!(result, 0, "PTRACE_CONT: {}", Errno::last()); +} + +/// Opens a close-on-exec pidfd that refers to `process`, which polls readable once the process +/// ends, and keeps naming it after it is reaped. +/// +/// libc's `syscall` is used because neither `nix` nor musl wraps `pidfd_open`. +pub(crate) fn open_pidfd(process: ProcessId) -> Result { + // SAFETY: `pidfd_open` reads only its integer arguments, and returns a new descriptor or -1. + let descriptor = Errno::result(unsafe { + nix::libc::syscall(nix::libc::SYS_pidfd_open, process.0.as_raw(), 0) + })?; + // A descriptor number fits a `RawFd`. + let descriptor = descriptor as RawFd; + // SAFETY: the kernel has just created `descriptor`, so it is open and nothing else owns it. + Ok(unsafe { FileDescriptor::from_raw(descriptor) }) +} + /// Returns whether `process` has reached a waitable terminal state without reaping it. /// /// libc is used because `nix`'s `waitid` decodes the state into a `WaitStatus` too, which fails diff --git a/nvx-rs/src/lib.rs b/nvx-rs/src/lib.rs index 046a55304..91b843476 100644 --- a/nvx-rs/src/lib.rs +++ b/nvx-rs/src/lib.rs @@ -20,9 +20,11 @@ //! - `testing` (feature `testing`): a fake OpenVMM that serves the control console without //! running a VM. //! - `guest` (feature `guest`, Linux): the programs that run in NVX guests, such as -//! `nvx-console-pending`, the guest's NVXS connection, and `guest::spawn`, through which any -//! guest init or agent starts a program in a new process. The crate's `nvx-*` guest binaries -//! run the programs. +//! `nvx-console-pending`, the guest's NVXS connection, and the libraries through which any +//! guest init or agent starts a program in a new process (`guest::spawn`), collects the status +//! of each child that it names (`guest::process`), and places, kills, and settles the processes +//! of its executions in cgroups (`guest::cgroup`). The crate's `nvx-*` guest binaries run the +//! programs. //! //! # Example //! diff --git a/nvx-rs/tests/managed_lifecycle.rs b/nvx-rs/tests/managed_lifecycle.rs index ab4288a0e..abd34aa02 100644 --- a/nvx-rs/tests/managed_lifecycle.rs +++ b/nvx-rs/tests/managed_lifecycle.rs @@ -1,7 +1,8 @@ //! The managed lifecycle of a guest whose init hands the boot to the managed agent, on a real //! hypervisor: authenticated sessions and reconnects, request-scoped working directories and -//! environments, the workload identity, timeouts, refused working directories, and stop, driven -//! through `nvx::host::session` as `nvx::host::instance` drives it. +//! environments, the workload identity, timeouts, descendants that outlive their workload, +//! refused working directories, and stop, driven through `nvx::host::session` as +//! `nvx::host::instance` drives it. //! //! `scripts/nvx.py test-microvm --scenario managed-lifecycle` runs it for the selected guest and //! backend, then checks the outcome report that OpenVMM wrote and the guest console. It sets: @@ -481,6 +482,34 @@ fn a_managed_guest_runs_request_scoped_workloads_until_it_stops() { after_timeout.printed("still-usable"), "the guest was not usable after a timeout: {after_timeout:?}" ); + // A workload that leaves a descendant in a session of its own, outside the workload's process + // group, which keeps the workload's output open, ends at once with its own status: the guest + // finds the descendant in the workload's cgroup and ends it. The next workload runs, which the + // guest allows only once that cgroup is settled. + let started = Instant::now(); + let detached = guest.exec( + &mut session, + &["/bin/sh", "-c", "setsid sleep 30 & printf started"], + 5_000, + None, + WorkloadEnvironment::Default, + ); + let elapsed = started.elapsed(); + assert!( + detached.printed("started") && elapsed < Duration::from_secs(15), + "a workload whose descendant held its output ended with {detached:?} after {elapsed:?}" + ); + let after_detached = guest.exec( + &mut session, + &["/bin/sh", "-c", "printf still-usable"], + 5_000, + None, + WorkloadEnvironment::Default, + ); + assert!( + after_detached.printed("still-usable"), + "the guest did not settle a workload's descendant: {after_detached:?}" + ); // The guest refuses a working directory that the workload cannot enter, with its error // number and a diagnostic that names it, before anything of the workload runs. for (description, cwd, error) in REFUSED_CWDS { From 3396264217f2ef1b0251cf87452472e868a201e8 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Tue, 20 Oct 2026 20:14:19 -0700 Subject: [PATCH 2/4] nvx-rs: hold the directories of guest cgroup handles open An `ExecutionCgroup` now holds the directory of its cgroup open, and the workload cgroup's for an execution with a cgroup of its own, and each operation opens the interface files that it uses in them. A handle that the caller keeps after it removed the cgroup and prepared another at the same path so reaches nothing: its kill, occupancy check and placements fail with `ENOENT`, and its removal finds the cgroup gone, leaving the new one alone. Opening a handle now needs the cgroup to exist. Clones share the directories, and dropping the last one closes them, without killing or removing the cgroup. The managed agent no longer drops a failure to collect the status of a workload that it ended: it reports the `exec-wait` diagnostic, and a request whose workload it ended for a cause ends with that cause instead of waiting for the status forever. Its collection of orphans, which cannot fail, no longer returns a result. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f1cb1a1-0d06-46b2-a35a-54af8eb9583c --- nvx-rs/README.md | 2 +- nvx-rs/src/guest/cgroup.rs | 243 ++++++++++++++++----- nvx-rs/src/guest/managed_agent/cgroup.rs | 14 +- nvx-rs/src/guest/managed_agent/dispatch.rs | 42 ++++ nvx-rs/src/guest/managed_agent/exec.rs | 26 ++- nvx-rs/src/guest/spawn.rs | 12 + nvx-rs/src/guest/sys/fd.rs | 11 + nvx-rs/src/guest/sys/mod.rs | 2 +- 8 files changed, 283 insertions(+), 69 deletions(-) diff --git a/nvx-rs/README.md b/nvx-rs/README.md index b8d4768f8..610104c0b 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -71,7 +71,7 @@ nvx-rs/ | `nvx::guest::nvxs` | `GuestConnection`: the guest's role of the NVXS session over a byte stream, such as the control console, on which NVX's managed agent and other guest agents run their sessions. It never reads past the record in progress, so a caller that polls the stream between records sees every record that it has not returned | | `nvx::guest::spawn` | `Command`: starts a program in a new process for any guest init or agent, with its arguments, environment, root and working directories, standard streams, inherited descriptors, cgroup, and process group, without NVX's control protocol, managed agent, or workload identity. Its `LaunchReport` tells whether the program started, or which `LaunchStage` failed and why. NVX's managed agent is one caller | | `nvx::guest::process` | `Child`: the status of one child that the caller names, by its pidfd or its process ID. It waits for that child alone, so another child's status stays for the caller; a caller that collects every status itself, such as a PID 1 that adopts orphans, hands each one to its `Child` with `observe`. Statuses are `std::process::ExitStatus`, real-time signals included | -| `nvx::guest::cgroup` | `Layout`: the agent's cgroup and the workload cgroup, which the caller names, and which may not hold the agent's. `ExecutionCgroup`: the cgroup of one execution, the workload cgroup itself or a child of it, which it prepares with the caller's resource values, places processes in, checks for occupancy through `pids.current`, kills with `cgroup.kill`, and settles, over the execution's cgroup or the whole workload subtree, as the caller's `Scope` chooses. Settling calls the caller's status collector between its checks, and collects no status itself | +| `nvx::guest::cgroup` | `Layout`: the agent's cgroup and the workload cgroup, which the caller names, and which may not hold the agent's. `ExecutionCgroup`: the cgroup of one execution, the workload cgroup itself or a child of it, which it prepares with the caller's resource values, places processes in, checks for occupancy through `pids.current`, kills with `cgroup.kill`, and settles, over the execution's cgroup or the whole workload subtree, as the caller's `Scope` chooses. It holds its cgroups' directories open, so that it never reaches another cgroup that takes one of their paths. Settling calls the caller's status collector between its checks, and collects no status itself | Lowering functions take the names by which messages refer to their inputs, such as `EgressRules::name` and `PathList::name`, so callers' messages name their own diff --git a/nvx-rs/src/guest/cgroup.rs b/nvx-rs/src/guest/cgroup.rs index c3497debe..3b53e1f31 100644 --- a/nvx-rs/src/guest/cgroup.rs +++ b/nvx-rs/src/guest/cgroup.rs @@ -24,8 +24,12 @@ //! Occupancy is the cgroup's `pids.current`, which counts every process that the cgroup charges, //! a zombie that no process has collected included. The library collects no status: settling //! calls the caller's own status collector between its checks, as [`process`](super::process) -//! describes. Each operation opens the cgroup's interface files anew, so a handle stays valid -//! whatever else changes the cgroup. +//! describes. +//! +//! A handle holds the directories of its cgroups open, from when it was +//! [opened](ExecutionCgroup::open), and each operation opens the interface files that it uses in +//! them anew. So a handle stays valid whatever controllers its cgroup gains, and reaches its own +//! cgroups alone, even once the caller removes one and prepares another at its path. //! //! ```no_run //! use std::time::Duration; @@ -53,10 +57,11 @@ use std::fs; use std::io; use std::path::{Component, Path, PathBuf}; +use std::sync::Arc; use std::time::{Duration, Instant}; use super::spawn::Cgroup; -use super::sys::{self, Errno, Mode, OpenFlags, ProcessId}; +use super::sys::{self, Errno, FileDescriptor, Mode, OpenFlags, ProcessId}; /// A cgroup's membership file. const PROCS: &str = "cgroup.procs"; @@ -127,42 +132,55 @@ pub enum Scope { /// The handle of one execution's cgroup: the workload cgroup itself, or a child cgroup of it. /// -/// It names the cgroup by its path, and dropping it changes nothing. -#[derive(Clone, Debug, PartialEq, Eq)] +/// It holds the directory of the execution's cgroup open, and the workload cgroup's for an +/// execution with a cgroup of its own, as [`open`](Self::open) found them, and each operation +/// opens the interface files that it uses in them. So it reaches those cgroups alone, even once +/// another cgroup takes one of their paths: a cgroup that was removed has no interface file any +/// more, and an operation on it fails with `ENOENT`. Clones share the directories, and dropping +/// the last one closes them, but neither kills nor removes the cgroup: the caller does that with +/// [`kill`](Self::kill), [`settle`](Self::settle), which takes its status collector, and +/// [`remove`](Self::remove), and sees their failures. +#[derive(Clone, Debug)] pub struct ExecutionCgroup { - /// The execution's cgroup. + /// The path of the execution's cgroup. path: PathBuf, - /// The workload cgroup. + /// The path of the workload cgroup. workload: PathBuf, /// Whether the execution has a cgroup of its own below the workload cgroup. own: bool, + /// The directories of the cgroups, which the clones share. + directories: Arc, +} + +/// The open directories of an [`ExecutionCgroup`]'s cgroups. +#[derive(Debug)] +struct Directories { + /// The execution's cgroup. + execution: FileDescriptor, + /// The workload cgroup, for an execution with a cgroup of its own; otherwise `execution` + /// names it. + workload: Option, } impl ExecutionCgroup { - /// Names the cgroup of an execution in `layout`, without changing the hierarchy: the - /// workload cgroup itself, or, with `name`, its child cgroup `name`. + /// Opens the cgroup of an execution in `layout`, without changing the hierarchy: the + /// workload cgroup itself, or, with `name`, its child cgroup `name`. It holds the directory + /// of that cgroup open, and the workload cgroup's for a child cgroup, so both must exist. /// /// A name that is not one path component, such as an empty one, `.`, `..`, or one with a - /// `/` or a NUL, is `EINVAL`. + /// `/` or a NUL, is `EINVAL`, and a cgroup that does not exist `ENOENT`. pub fn open(layout: &Layout, name: Option<&str>) -> io::Result { - let path = match name { - None => layout.workload.clone(), - Some(name) => { - let mut components = Path::new(name).components(); - if name.contains('\0') - || !matches!(components.next(), Some(Component::Normal(_))) - || components.next().is_some() - || name.contains('/') - { - return Err(Errno::EINVAL.into()); - } - layout.workload.join(name) - } - }; + let path = execution_path(layout, name)?; + let workload = name.map(|_| open_directory(&layout.workload)).transpose()?; + let execution = open_directory(&path)?; Ok(Self { path, workload: layout.workload.clone(), own: name.is_some(), + directories: Arc::new(Directories { + execution, + workload, + }), }) } @@ -193,7 +211,7 @@ impl ExecutionCgroup { for (file, _) in resources { controller(file)?; } - let execution = Self::open(layout, name)?; + let path = execution_path(layout, name)?; let above_workload = layout .workload .parent() @@ -201,17 +219,18 @@ impl ExecutionCgroup { let files = || std::iter::once(PIDS_CURRENT).chain(resources.iter().map(|(file, _)| *file)); create(&layout.workload, false)?; provide(&layout.workload, above_workload, files())?; - if execution.own { - create(&execution.path, true)?; + if name.is_some() { + create(&path, true)?; } - if let Err(error) = execution.configure(files(), resources) { - if execution.own { - // The new cgroup holds no process yet. - let _ = fs::remove_dir(&execution.path); - } - return Err(error); + let prepared = Self::open(layout, name).and_then(|execution| { + execution.configure(files(), resources)?; + Ok(execution) + }); + if prepared.is_err() && name.is_some() { + // The new cgroup holds no process yet. + let _ = fs::remove_dir(&path); } - Ok(execution) + prepared } /// Gives the execution's own cgroup, if it has one, the interface files `files` through the @@ -227,7 +246,7 @@ impl ExecutionCgroup { for (file, value) in resources { // Writing nothing would reach no controller. let value = if value.is_empty() { "\n" } else { value }; - write(&self.path.join(file), value.as_bytes())?; + write_in(&self.directories.execution, file, value.as_bytes())?; } Ok(()) } @@ -241,7 +260,7 @@ impl ExecutionCgroup { /// the execution's cgroup before its program runs, so that the program and its descendants /// start in it. pub fn placement(&self) -> io::Result { - Cgroup::open(&self.path) + Cgroup::open_in(&self.directories.execution) } /// Moves the process `id`, with all of its threads, into the execution's cgroup. An ID of 0, @@ -249,7 +268,7 @@ impl ExecutionCgroup { pub fn place(&self, id: u32) -> io::Result<()> { let process = ProcessId::from_positive(i64::from(id)) .ok_or_else(|| io::Error::from(Errno::EINVAL))?; - write(&self.path.join(PROCS), &process.line()) + write_in(&self.directories.execution, PROCS, &process.line()) } /// Returns whether `scope` holds a process, a zombie that no process has collected included. @@ -257,12 +276,7 @@ impl ExecutionCgroup { /// It reads `pids.current`, which must be one canonical decimal and a newline: another form /// is `EPROTO`, and one of more than 31 bytes `EOVERFLOW`. pub fn is_occupied(&self, scope: Scope) -> io::Result { - let path = sys::path_c_string(&self.scope(scope).join(PIDS_CURRENT))?; - let descriptor = sys::open( - &path, - OpenFlags::O_RDONLY | OpenFlags::O_CLOEXEC, - Mode::empty(), - )?; + let descriptor = open_file(self.directory(scope), PIDS_CURRENT, OpenFlags::O_RDONLY)?; let mut buffer = [0u8; COUNT_CAPACITY]; let mut length = 0; let result = loop { @@ -288,7 +302,7 @@ impl ExecutionCgroup { /// process that one of them forks meanwhile. It does not wait for them to end: see /// [`settle`](Self::settle). pub fn kill(&self, scope: Scope) -> io::Result<()> { - write(&self.scope(scope).join(KILL), b"1") + write_in(self.directory(scope), KILL, b"1") } /// Waits at most `timeout` until `scope` holds no process, and fails with `ETIMEDOUT` if one @@ -327,7 +341,8 @@ impl ExecutionCgroup { /// Removes the execution's own cgroup unless it is gone already, and fails with `EBUSY` /// while a process, a zombie included, or a cgroup is left in it, so that a removal never /// hides a status that the caller has yet to collect. An execution that runs in the workload - /// cgroup itself leaves it in place. + /// cgroup itself leaves it in place, and a cgroup that the caller prepared at the path of a + /// removed one stays. pub fn remove(&self) -> io::Result<()> { if !self.own { return Ok(()); @@ -335,7 +350,7 @@ impl ExecutionCgroup { match self.is_occupied(Scope::Execution) { Ok(false) => {} Ok(true) => return Err(Errno::EBUSY.into()), - Err(error) if error.kind() == io::ErrorKind::NotFound && !self.path.exists() => { + Err(error) if error.kind() == io::ErrorKind::NotFound && self.is_removed() => { return Ok(()); } Err(error) => return Err(error), @@ -346,15 +361,73 @@ impl ExecutionCgroup { } } + /// Returns whether the execution's cgroup was removed, which leaves its directory without + /// even the membership file that every cgroup has. + fn is_removed(&self) -> bool { + open_file(&self.directories.execution, PROCS, OpenFlags::O_RDONLY) + .is_err_and(|error| error.kind() == io::ErrorKind::NotFound) + } + /// Returns the directory of the cgroup that `scope` covers. - fn scope(&self, scope: Scope) -> &Path { - match scope { - Scope::Execution => &self.path, - Scope::Workload => &self.workload, + fn directory(&self, scope: Scope) -> &FileDescriptor { + match (scope, &self.directories.workload) { + (Scope::Workload, Some(workload)) => workload, + _ => &self.directories.execution, } } } +/// Returns the path of an execution's cgroup in `layout`: the workload cgroup itself, or its +/// child cgroup `name`, which is one path component, or `EINVAL`. +fn execution_path(layout: &Layout, name: Option<&str>) -> io::Result { + let Some(name) = name else { + return Ok(layout.workload.clone()); + }; + let mut components = Path::new(name).components(); + if name.contains('\0') + || !matches!(components.next(), Some(Component::Normal(_))) + || components.next().is_some() + || name.contains('/') + { + return Err(Errno::EINVAL.into()); + } + Ok(layout.workload.join(name)) +} + +/// Opens the directory of the cgroup at `path`, which names that cgroup from then on, whatever +/// takes its path. +fn open_directory(path: &Path) -> io::Result { + Ok(sys::open( + &sys::path_c_string(path)?, + OpenFlags::O_RDONLY | OpenFlags::O_DIRECTORY | OpenFlags::O_CLOEXEC, + Mode::empty(), + )?) +} + +/// Opens the interface file `file` of the cgroup whose directory `directory` holds, with `flags`. +fn open_file( + directory: &FileDescriptor, + file: &str, + flags: OpenFlags, +) -> io::Result { + let name = sys::c_string(file.as_bytes())?; + Ok(sys::open_at( + directory, + &name, + flags | OpenFlags::O_CLOEXEC, + Mode::empty(), + )?) +} + +/// Writes a whole control value to the interface file `file` of the cgroup whose directory +/// `directory` holds, and reports a failure to close it, with which the kernel may refuse the +/// value. +fn write_in(directory: &FileDescriptor, file: &str, bytes: &[u8]) -> io::Result<()> { + let descriptor = open_file(directory, file, OpenFlags::O_WRONLY)?; + descriptor.write_all(bytes)?; + Ok(descriptor.close()?) +} + /// Returns whether `path` is absolute and has no `..` component, so that its components name /// the cgroups that it reaches. fn well_formed(path: &Path) -> bool { @@ -558,10 +631,11 @@ mod tests { #[test] fn executions_run_in_the_workload_cgroup_or_in_one_child_of_it() { - let layout = Layout::new("/sys/fs/cgroup", "/sys/fs/cgroup/workloads").unwrap(); + let (_temporary, layout) = hierarchy(); let workload = ExecutionCgroup::open(&layout, None).unwrap(); assert_eq!(workload.path(), layout.workload()); for name in ["exec-1", "exec.1", "..."] { + fs::create_dir(layout.workload().join(name)).unwrap(); let execution = ExecutionCgroup::open(&layout, Some(name)).unwrap(); assert_eq!(execution.path(), layout.workload().join(name)); } @@ -577,6 +651,21 @@ mod tests { "{name:?}" ); } + // A handle holds its cgroups' directories, which must exist. + assert_eq!( + error(ExecutionCgroup::open(&layout, Some("missing"))), + Some(Errno::ENOENT as i32) + ); + fs::write(layout.workload().join("file"), b"").unwrap(); + assert_eq!( + error(ExecutionCgroup::open(&layout, Some("file"))), + Some(Errno::ENOTDIR as i32) + ); + let missing = Layout::new(layout.agent(), layout.agent().join("missing")).unwrap(); + assert_eq!( + error(ExecutionCgroup::open(&missing, None)), + Some(Errno::ENOENT as i32) + ); } #[test] @@ -818,12 +907,39 @@ mod tests { for id in [0, u32::MAX] { assert_eq!(error(execution.place(id)), Some(Errno::EINVAL as i32)); } - assert!( - ExecutionCgroup::open(&layout, Some("missing")) - .unwrap() - .placement() - .is_err() - ); + } + + #[test] + fn a_handle_reaches_its_own_cgroups_alone_once_another_takes_a_path() { + let (_temporary, layout) = hierarchy(); + let old = fake_execution(&layout, "exec-1", b"0\n"); + let clone = old.clone(); + // The caller removes the cgroup, as `remove` does in a real hierarchy, and prepares + // another of the same name. + fs::remove_dir_all(old.path()).unwrap(); + let new = fake_execution(&layout, "exec-1", b"1\n"); + for handle in [&old, &clone] { + assert_eq!( + error(handle.kill(Scope::Execution)), + Some(Errno::ENOENT as i32) + ); + assert_eq!( + error(handle.is_occupied(Scope::Execution)), + Some(Errno::ENOENT as i32) + ); + assert_eq!(error(handle.place(1)), Some(Errno::ENOENT as i32)); + assert_eq!(error(handle.placement()), Some(Errno::ENOENT as i32)); + handle.remove().unwrap(); + } + assert!(new.path().is_dir()); + assert_eq!(fs::read(new.path().join(KILL)).unwrap(), b""); + assert_eq!(fs::read(new.path().join(PROCS)).unwrap(), b""); + assert!(new.is_occupied(Scope::Execution).unwrap()); + // The workload cgroup is still the one that the old handle opened. + old.kill(Scope::Workload).unwrap(); + assert_eq!(fs::read(layout.workload().join(KILL)).unwrap(), b"1"); + new.kill(Scope::Execution).unwrap(); + assert_eq!(fs::read(new.path().join(KILL)).unwrap(), b"1"); } #[test] @@ -1021,6 +1137,19 @@ mod tests { assert!(rest.is_empty()); first.remove().unwrap(); assert!(!first.path().exists()); + // A cgroup prepared at the removed one's path is out of the old handle's reach. + let again = ExecutionCgroup::prepare(layout, Some("first"), &[]).unwrap(); + assert_eq!( + error(first.kill(Scope::Execution)), + Some(Errno::ENOENT as i32) + ); + assert_eq!( + error(first.is_occupied(Scope::Execution)), + Some(Errno::ENOENT as i32) + ); + first.remove().unwrap(); + assert!(again.path().is_dir()); + again.remove().unwrap(); assert_eq!(sibling.try_wait().unwrap(), None); assert!(second.is_occupied(Scope::Execution).unwrap()); diff --git a/nvx-rs/src/guest/managed_agent/cgroup.rs b/nvx-rs/src/guest/managed_agent/cgroup.rs index 9ed5f93f9..a59556449 100644 --- a/nvx-rs/src/guest/managed_agent/cgroup.rs +++ b/nvx-rs/src/guest/managed_agent/cgroup.rs @@ -1,4 +1,3 @@ -use std::io; use std::os::unix::process::ExitStatusExt; use std::path::Path; use std::process::ExitStatus; @@ -93,15 +92,15 @@ pub(crate) fn reap(child: &mut Child) { } /// Collects the status of every child that has ended once the workload's status is known, when -/// every child left is an orphaned descendant. -fn reap_orphans() -> io::Result<()> { +/// every child left is an orphaned descendant. Like the C agent's loop, it stops once no ended +/// child is left to collect, and has no failure to report. +fn reap_orphans() { while sys::reap_any_nonblocking().is_some() {} - Ok(()) } /// Reaps orphaned descendants and returns the cgroup's current occupancy. pub(crate) fn verify_empty(system: &impl System) -> Result { - let _ = reap_orphans(); + reap_orphans(); has_tasks(system) } @@ -109,7 +108,10 @@ pub(crate) fn verify_empty(system: &impl System) -> Result { pub(crate) fn settle(system: &mut impl System) -> Result<(), Errno> { kill(system)?; execution(system.paths())? - .settle(Scope::Workload, SETTLE_TIMEOUT, reap_orphans) + .settle(Scope::Workload, SETTLE_TIMEOUT, || { + reap_orphans(); + Ok(()) + }) .map_err(|error| sys::errno_of(&error)) } diff --git a/nvx-rs/src/guest/managed_agent/dispatch.rs b/nvx-rs/src/guest/managed_agent/dispatch.rs index 60f090177..36ec3810f 100644 --- a/nvx-rs/src/guest/managed_agent/dispatch.rs +++ b/nvx-rs/src/guest/managed_agent/dispatch.rs @@ -953,6 +953,48 @@ mod tests { Ok(()) } + #[test] + fn a_workload_status_that_the_agent_cannot_collect_still_ends_its_request() -> TestResult { + // While `SIGCHLD` is ignored, which applies to the whole process, the kernel collects + // every child's status itself, so the test runs in a process of its own. + if super::super::isolate_process_test( + "NVX_DISPATCH_LOST_STATUS_TEST_CHILD", + "guest::managed_agent::dispatch::tests::a_workload_status_that_the_agent_cannot_collect_still_ends_its_request", + ) { + return Ok(()); + } + let _process = super::super::PROCESS_TEST_LOCK.lock()?; + let (temporary, mut system) = TestSystem::with_cgroup(); + system.paths.setpriv = super::super::seam::setpriv_shim(temporary.path(), ":"); + system.spawner = Some(super::super::seam::spawn_as_privileged); + crate::guest::sys::ignore_signals(&[crate::guest::sys::Signal::SIGCHLD]); + let payload = encode_exec_payload(&Workload { + argv: vec!["/bin/sleep".to_owned(), "30".to_owned()], + timeout_ms: 100, + cwd: None, + environment: WorkloadEnvironment::Default, + })?; + let mut harness = Harness::with_system(true, system, 0); + // An agent that waited for the lost status would never answer. + harness + .host + .set_read_timeout(Some(std::time::Duration::from_secs(20)))?; + assert_eq!( + harness.request_all(42, APP_EXEC, &payload), + vec![response(APP_EXIT, 42, 124, b"timeout")] + ); + let system = harness.stop(); + assert_eq!( + system.diagnostics, + format!( + "NVX-MANAGED-ERROR: stage=exec-wait status={}\n", + Errno::ECHILD as i32 + ) + .as_bytes() + ); + Ok(()) + } + #[test] fn direct_launch_starts_the_workload_in_the_directory_that_it_checked() -> TestResult { if super::super::isolate_process_test( diff --git a/nvx-rs/src/guest/managed_agent/exec.rs b/nvx-rs/src/guest/managed_agent/exec.rs index 71646aa80..35f1cbabb 100644 --- a/nvx-rs/src/guest/managed_agent/exec.rs +++ b/nvx-rs/src/guest/managed_agent/exec.rs @@ -85,7 +85,7 @@ pub(crate) fn run( } if launch_failed { cgroup::terminate(system, config.direct, child.process()); - let _ = child.wait(); + wait_ended(system, &mut child); unlink_barrier(system); return session.send_error(request_id, 125, LAUNCH_FAILED); } @@ -122,7 +122,7 @@ pub(crate) fn run( Ok(control) => control, Err(error) => { cgroup::terminate(system, config.direct, child.process()); - let _ = child.wait(); + wait_ended(system, &mut child); return Err(error); } }; @@ -173,8 +173,13 @@ pub(crate) fn run( timed_out = true; cgroup::terminate(system, config.direct, child.process()); } - if (timed_out || output_limited || cancelled || session_lost) && child.status().is_none() { - let _ = child.wait(); + // The outcome is the cause for which the agent ended the workload, so a status that it + // cannot collect only ends the loop, which would otherwise wait for it forever. + if (timed_out || output_limited || cancelled || session_lost) + && child.status().is_none() + && !wait_ended(system, &mut child) + { + break; } } @@ -237,6 +242,19 @@ pub(crate) fn run( } } +/// Waits until the workload that the agent ended has ended, and returns whether its status is +/// known. A failure to collect it, which leaves the agent without the status, is the +/// `exec-wait` diagnostic. +fn wait_ended(system: &mut impl System, child: &mut Child) -> bool { + match child.wait() { + Ok(_) => true, + Err(error) => { + super::diagnostics::error(system, "exec-wait", sys::errno_of(&error) as i32); + false + } + } +} + /// A spawned workload and the parent's ends of its pipes. struct Launched { /// The workload process, which leads its own process group, and whose status the agent diff --git a/nvx-rs/src/guest/spawn.rs b/nvx-rs/src/guest/spawn.rs index 3955dfd31..a559bf630 100644 --- a/nvx-rs/src/guest/spawn.rs +++ b/nvx-rs/src/guest/spawn.rs @@ -184,6 +184,18 @@ impl Cgroup { )?; Ok(Self { procs }) } + + /// Opens the membership file of the cgroup v2 whose directory `directory` holds open, so + /// that a child joins that cgroup even once another cgroup takes its path. + pub(crate) fn open_in(directory: &FileDescriptor) -> io::Result { + let procs = sys::open_at( + directory, + c"cgroup.procs", + OpenFlags::O_WRONLY | OpenFlags::O_CLOEXEC, + Mode::empty(), + )?; + Ok(Self { procs }) + } } /// A program, and how a new process starts it. diff --git a/nvx-rs/src/guest/sys/fd.rs b/nvx-rs/src/guest/sys/fd.rs index fca62ab41..979ae17ef 100644 --- a/nvx-rs/src/guest/sys/fd.rs +++ b/nvx-rs/src/guest/sys/fd.rs @@ -208,6 +208,17 @@ pub(crate) fn open(path: &CStr, flags: OpenFlags, mode: Mode) -> Result Result { + nix::fcntl::openat(&directory.0, name, flags, mode).map(FileDescriptor) +} + /// Creates an anonymous close-on-exec file that permits seals. pub(crate) fn create_memfd(name: &CStr) -> Result { memfd_create(name, MFdFlags::MFD_CLOEXEC | MFdFlags::MFD_ALLOW_SEALING).map(FileDescriptor) diff --git a/nvx-rs/src/guest/sys/mod.rs b/nvx-rs/src/guest/sys/mod.rs index 7a5ad0cac..9a35e1ab4 100644 --- a/nvx-rs/src/guest/sys/mod.rs +++ b/nvx-rs/src/guest/sys/mod.rs @@ -74,7 +74,7 @@ pub(crate) use clock::{monotonic_ns, syscall_monotonic_nanoseconds}; pub(crate) use discipline::{ClockAdjustment, ClockDiscipline, adjust_clock}; pub(crate) use fd::{ CreatedFile, FileDescriptor, FileLock, Mode, OpenFlags, create_directory, create_fifo, - create_memfd, create_pipe, lock_file, open, page_size, unlink, + create_memfd, create_pipe, lock_file, open, open_at, page_size, unlink, }; pub(crate) use filesystem::{ MountFlags, ScratchDirectory, TmpfsMount, change_directory, check_exists, chmod, chown, From e3f1d7a7d87baf9f500b9426c9f6b58358207ef5 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Tue, 20 Oct 2026 20:49:40 -0700 Subject: [PATCH 3/4] nvx-rs: let guest cgroup removals take turns ExecutionCgroup::remove checked its held cgroup, then removed the cgroup at its path. Between the two, a clone of the handle could remove the cgroup, and the caller could prepare another at the same path, which the delayed removal then deleted. The process's removals of execution cgroups now take turns, through one process-wide lock, with its preparations of executions' own cgroups. In its turn, a removal checks that its cgroup is still there before it removes the cgroup at its path, which is that cgroup, as cgroup v2 never renames one. A preparation holds its turn from the creation of its cgroup until it returns, so that its handle holds the cgroup that it created, and a failure removes that cgroup alone. An execution that runs in the workload cgroup itself, as the direct agent's does, takes no turn. Two tests on a real cgroup v2 hierarchy pause a removal after its check, and a preparation once it created its cgroup, while another handle removes the cgroup and prepares another at its path, which stays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f1cb1a1-0d06-46b2-a35a-54af8eb9583c --- nvx-rs/README.md | 2 +- nvx-rs/src/guest/cgroup.rs | 144 +++++++++++++++++++++++++++++++++++-- 2 files changed, 140 insertions(+), 6 deletions(-) diff --git a/nvx-rs/README.md b/nvx-rs/README.md index 610104c0b..2bd177b8d 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -71,7 +71,7 @@ nvx-rs/ | `nvx::guest::nvxs` | `GuestConnection`: the guest's role of the NVXS session over a byte stream, such as the control console, on which NVX's managed agent and other guest agents run their sessions. It never reads past the record in progress, so a caller that polls the stream between records sees every record that it has not returned | | `nvx::guest::spawn` | `Command`: starts a program in a new process for any guest init or agent, with its arguments, environment, root and working directories, standard streams, inherited descriptors, cgroup, and process group, without NVX's control protocol, managed agent, or workload identity. Its `LaunchReport` tells whether the program started, or which `LaunchStage` failed and why. NVX's managed agent is one caller | | `nvx::guest::process` | `Child`: the status of one child that the caller names, by its pidfd or its process ID. It waits for that child alone, so another child's status stays for the caller; a caller that collects every status itself, such as a PID 1 that adopts orphans, hands each one to its `Child` with `observe`. Statuses are `std::process::ExitStatus`, real-time signals included | -| `nvx::guest::cgroup` | `Layout`: the agent's cgroup and the workload cgroup, which the caller names, and which may not hold the agent's. `ExecutionCgroup`: the cgroup of one execution, the workload cgroup itself or a child of it, which it prepares with the caller's resource values, places processes in, checks for occupancy through `pids.current`, kills with `cgroup.kill`, and settles, over the execution's cgroup or the whole workload subtree, as the caller's `Scope` chooses. It holds its cgroups' directories open, so that it never reaches another cgroup that takes one of their paths. Settling calls the caller's status collector between its checks, and collects no status itself | +| `nvx::guest::cgroup` | `Layout`: the agent's cgroup and the workload cgroup, which the caller names, and which may not hold the agent's. `ExecutionCgroup`: the cgroup of one execution, the workload cgroup itself or a child of it, which it prepares with the caller's resource values, places processes in, checks for occupancy through `pids.current`, kills with `cgroup.kill`, and settles, over the execution's cgroup or the whole workload subtree, as the caller's `Scope` chooses. It holds its cgroups' directories open, and the process's removals and preparations of cgroups take turns, so that it never reaches another cgroup that takes one of their paths. Settling calls the caller's status collector between its checks, and collects no status itself | Lowering functions take the names by which messages refer to their inputs, such as `EgressRules::name` and `PathList::name`, so callers' messages name their own diff --git a/nvx-rs/src/guest/cgroup.rs b/nvx-rs/src/guest/cgroup.rs index 3b53e1f31..9e37cd08b 100644 --- a/nvx-rs/src/guest/cgroup.rs +++ b/nvx-rs/src/guest/cgroup.rs @@ -29,7 +29,9 @@ //! A handle holds the directories of its cgroups open, from when it was //! [opened](ExecutionCgroup::open), and each operation opens the interface files that it uses in //! them anew. So a handle stays valid whatever controllers its cgroup gains, and reaches its own -//! cgroups alone, even once the caller removes one and prepares another at its path. +//! cgroups alone, even once the caller removes one and prepares another at its path. A removal, +//! which only a path names, takes its turn with the process's other removals and preparations, +//! so it never reaches such a cgroup either. //! //! ```no_run //! use std::time::Duration; @@ -57,7 +59,7 @@ use std::fs; use std::io; use std::path::{Component, Path, PathBuf}; -use std::sync::Arc; +use std::sync::{Arc, Mutex, MutexGuard, PoisonError}; use std::time::{Duration, Instant}; use super::spawn::Cgroup; @@ -76,6 +78,19 @@ const COUNT_CAPACITY: usize = 32; /// The interval between two checks of a settling cgroup. const SETTLE_INTERVAL: Duration = Duration::from_millis(10); +/// The turns of the process's removals of execution cgroups, and of its preparations of +/// execution cgroups of their own, which take them one at a time: see +/// [`ExecutionCgroup::remove`]. +static TURNS: Mutex<()> = Mutex::new(()); + +#[cfg(test)] +thread_local! { + /// What the next turn on this thread runs once it found its cgroup, before it acts on the + /// cgroup's path: the pause in which a test races it. + static PAUSE: std::cell::Cell>> = + const { std::cell::Cell::new(None) }; +} + /// The cgroups that a guest init or agent uses: its own, and the workload cgroup in which or /// below which its executions run. #[derive(Clone, Debug, PartialEq, Eq)] @@ -202,7 +217,9 @@ impl ExecutionCgroup { /// before anything changes. A failure once the execution's own cgroup exists, such as a value /// that the kernel refuses, removes that cgroup again, so that its name can be prepared anew; /// the workload cgroup stays, with the controllers that it enabled, as the executions share - /// it. + /// it. The preparation of a cgroup of the execution's own takes its turn with the process's + /// removals, as [`remove`](Self::remove) describes, so that the handle holds the cgroup that + /// it created, and a failure removes that cgroup alone. pub fn prepare( layout: &Layout, name: Option<&str>, @@ -219,8 +236,12 @@ impl ExecutionCgroup { let files = || std::iter::once(PIDS_CURRENT).chain(resources.iter().map(|(file, _)| *file)); create(&layout.workload, false)?; provide(&layout.workload, above_workload, files())?; + // In its turn, no removal of the process removes the new cgroup, so its path names it. + let _turn = name.map(|_| turn()); if name.is_some() { create(&path, true)?; + #[cfg(test)] + pause(); } let prepared = Self::open(layout, name).and_then(|execution| { execution.configure(files(), resources)?; @@ -341,12 +362,19 @@ impl ExecutionCgroup { /// Removes the execution's own cgroup unless it is gone already, and fails with `EBUSY` /// while a process, a zombie included, or a cgroup is left in it, so that a removal never /// hides a status that the caller has yet to collect. An execution that runs in the workload - /// cgroup itself leaves it in place, and a cgroup that the caller prepared at the path of a - /// removed one stays. + /// cgroup itself leaves it in place. + /// + /// The removal names the cgroup by its path, which no other cgroup takes while the cgroup + /// exists, as cgroup v2 never renames one. So the process's removals take turns, with its + /// preparations of cgroups of the executions' own: in its turn, a removal finds its cgroup + /// still there before it removes the cgroup at its path, and a cgroup that the caller + /// prepared at the path of a removed one stays, even when clones of one handle remove the + /// cgroup at once. pub fn remove(&self) -> io::Result<()> { if !self.own { return Ok(()); } + let _turn = turn(); match self.is_occupied(Scope::Execution) { Ok(false) => {} Ok(true) => return Err(Errno::EBUSY.into()), @@ -355,6 +383,8 @@ impl ExecutionCgroup { } Err(error) => return Err(error), } + #[cfg(test)] + pause(); match fs::remove_dir(&self.path) { Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(()), result => result, @@ -377,6 +407,20 @@ impl ExecutionCgroup { } } +/// Waits for the process's turn to remove an execution's own cgroup, or to prepare one, which +/// lasts until the guard drops. +fn turn() -> MutexGuard<'static, ()> { + TURNS.lock().unwrap_or_else(PoisonError::into_inner) +} + +/// Runs the pause that a test set for this thread's next turn, if any. +#[cfg(test)] +fn pause() { + if let Some(pause) = PAUSE.take() { + pause(); + } +} + /// Returns the path of an execution's cgroup in `layout`: the workload cgroup itself, or its /// child cgroup `name`, which is one path component, or `EINVAL`. fn execution_path(layout: &Layout, name: Option<&str>) -> io::Result { @@ -1163,4 +1207,94 @@ mod tests { assert_eq!(sibling.status().unwrap().signal(), Some(nix::libc::SIGKILL)); second.remove().unwrap(); } + + /// Runs `racer` on another thread once this thread's next turn pauses, and lets the turn go + /// on once `racer` has returned, or after half a second, as it does when the turn holds the + /// racer off. Joining the thread returns what `racer` returned, or `None` if no turn paused. + fn race( + racer: impl FnOnce() -> T + Send + 'static, + ) -> std::thread::JoinHandle> { + let (go, start) = std::sync::mpsc::channel(); + let (finished, done) = std::sync::mpsc::channel(); + let thread = std::thread::spawn(move || { + start.recv().ok()?; + let result = racer(); + let _ = finished.send(()); + Some(result) + }); + PAUSE.set(Some(Box::new(move || { + go.send(()).unwrap(); + let _ = done.recv_timeout(Duration::from_millis(500)); + }))); + thread + } + + /// Returns what the racer of `thread` returned, once no turn of this thread is left to pause. + fn finish(thread: std::thread::JoinHandle>) -> T { + drop(PAUSE.take()); + thread.join().unwrap().expect("the turn paused") + } + + #[test] + fn a_removal_leaves_a_cgroup_prepared_at_its_path_by_a_concurrent_one() { + // The test changes the real hierarchy, which needs root; elsewhere it does nothing. + let Some(workload) = RealWorkload::new("removals") else { + return; + }; + let layout = workload.0.clone(); + let first = ExecutionCgroup::prepare(&layout, Some("exec"), &[]).unwrap(); + let delayed = first.clone(); + // Once the delayed clone found the cgroup empty, the racer removes it through the other + // clone and prepares another of the same name, unless the delayed removal holds it off. + let racer = race({ + let layout = layout.clone(); + move || { + first.remove().unwrap(); + ExecutionCgroup::prepare(&layout, Some("exec"), &[]).unwrap() + } + }); + delayed.remove().unwrap(); + let replacement = finish(racer); + assert!(replacement.path().is_dir()); + assert!(!replacement.is_occupied(Scope::Execution).unwrap()); + replacement.remove().unwrap(); + assert!(!replacement.path().exists()); + } + + #[test] + fn a_failed_preparation_removes_its_own_cgroup_alone() { + // The test changes the real hierarchy, which needs root; elsewhere it does nothing. + let Some(workload) = RealWorkload::new("preparations") else { + return; + }; + let layout = workload.0.clone(); + // A first preparation gives the workload cgroup's children the `pids` controller. + ExecutionCgroup::prepare(&layout, Some("first"), &[]) + .unwrap() + .remove() + .unwrap(); + // Once the preparation created its cgroup, the racer opens that cgroup, removes it, and + // prepares another of the same name, unless the preparation holds it off. The preparation + // then fails, as the kernel refuses its value, and removes its cgroup again. + let racer = race({ + let layout = layout.clone(); + move || { + let opened = ExecutionCgroup::open(&layout, Some("exec")).unwrap(); + opened.remove().unwrap(); + ExecutionCgroup::prepare(&layout, Some("exec"), &[]).unwrap() + } + }); + assert_eq!( + error(ExecutionCgroup::prepare( + &layout, + Some("exec"), + &[("pids.max", "-1")] + )), + Some(Errno::EINVAL as i32) + ); + let replacement = finish(racer); + assert!(replacement.path().is_dir()); + assert!(!replacement.is_occupied(Scope::Execution).unwrap()); + replacement.remove().unwrap(); + } } From a2094764a1ff313ab4b3880a957660a5621f6718 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Tue, 20 Oct 2026 21:54:29 -0700 Subject: [PATCH 4/4] nvx-rs: state what a guest cgroup removal needs of its caller ExecutionCgroup::remove refuses a cgroup that a process, a zombie included, occupies when it checks. The kernel removes a cgroup that only zombies occupy, so a process that the caller placed in the cgroup after that check, and that ended before the removal, would be a zombie that the removal hides. Its documentation now says that the caller places no process in a cgroup that it removes meanwhile, with `place` or with a spawned child whose launch has yet to report. No lock of the process covers a child's placement, which the child makes itself after `fork`, unless it is held across every spawn. The test of a failed preparation's rollback could fail when its racer started late: the preparation went on after half a second, and removed its cgroup before the racer had opened it. A racer's setup, such as that open, now takes place within the pause, which waits for it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f1cb1a1-0d06-46b2-a35a-54af8eb9583c --- nvx-rs/src/guest/cgroup.rs | 74 ++++++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 23 deletions(-) diff --git a/nvx-rs/src/guest/cgroup.rs b/nvx-rs/src/guest/cgroup.rs index 9e37cd08b..7c1fb1ad5 100644 --- a/nvx-rs/src/guest/cgroup.rs +++ b/nvx-rs/src/guest/cgroup.rs @@ -370,6 +370,12 @@ impl ExecutionCgroup { /// still there before it removes the cgroup at its path, and a cgroup that the caller /// prepared at the path of a removed one stays, even when clones of one handle remove the /// cgroup at once. + /// + /// The caller places no process in the cgroup while it removes it: neither with + /// [`place`](Self::place), nor with a child that it spawned with the cgroup's + /// [`placement`](Self::placement) and whose launch has yet to report. The kernel removes a + /// cgroup that only zombies occupy, so a process that joined the cgroup after the removal's + /// check, and ended before the removal, would be a zombie that the removal hides. pub fn remove(&self) -> io::Result<()> { if !self.own { return Ok(()); @@ -1208,22 +1214,31 @@ mod tests { second.remove().unwrap(); } - /// Runs `racer` on another thread once this thread's next turn pauses, and lets the turn go - /// on once `racer` has returned, or after half a second, as it does when the turn holds the - /// racer off. Joining the thread returns what `racer` returned, or `None` if no turn paused. - fn race( - racer: impl FnOnce() -> T + Send + 'static, + /// Runs a racer on another thread once this thread's next turn pauses, and lets the turn go + /// on once the racer has returned, or half a second after its setup, as it does when the turn + /// holds the racer off. The racer runs `setup` first, which the turn waits for, so that it + /// takes place within the pause however late the thread starts, then `racer`, with what + /// `setup` returned. Joining the thread returns what `racer` returned, or `None` if no turn + /// paused. + fn race( + setup: impl FnOnce() -> S + Send + 'static, + racer: impl FnOnce(S) -> T + Send + 'static, ) -> std::thread::JoinHandle> { let (go, start) = std::sync::mpsc::channel(); + let (ready, set_up) = std::sync::mpsc::channel(); let (finished, done) = std::sync::mpsc::channel(); let thread = std::thread::spawn(move || { start.recv().ok()?; - let result = racer(); + let state = setup(); + let _ = ready.send(()); + let result = racer(state); let _ = finished.send(()); Some(result) }); PAUSE.set(Some(Box::new(move || { go.send(()).unwrap(); + // A setup that fails drops its sender, which ends the wait at once. + let _ = set_up.recv_timeout(Duration::from_secs(10)); let _ = done.recv_timeout(Duration::from_millis(500)); }))); thread @@ -1246,13 +1261,20 @@ mod tests { let delayed = first.clone(); // Once the delayed clone found the cgroup empty, the racer removes it through the other // clone and prepares another of the same name, unless the delayed removal holds it off. - let racer = race({ - let layout = layout.clone(); + let racer = race( move || { - first.remove().unwrap(); - ExecutionCgroup::prepare(&layout, Some("exec"), &[]).unwrap() - } - }); + // The delayed removal pauses between its check and its removal. + assert!(!first.is_occupied(Scope::Execution).unwrap()); + first + }, + { + let layout = layout.clone(); + move |first: ExecutionCgroup| { + first.remove().unwrap(); + ExecutionCgroup::prepare(&layout, Some("exec"), &[]).unwrap() + } + }, + ); delayed.remove().unwrap(); let replacement = finish(racer); assert!(replacement.path().is_dir()); @@ -1273,17 +1295,23 @@ mod tests { .unwrap() .remove() .unwrap(); - // Once the preparation created its cgroup, the racer opens that cgroup, removes it, and - // prepares another of the same name, unless the preparation holds it off. The preparation - // then fails, as the kernel refuses its value, and removes its cgroup again. - let racer = race({ - let layout = layout.clone(); - move || { - let opened = ExecutionCgroup::open(&layout, Some("exec")).unwrap(); - opened.remove().unwrap(); - ExecutionCgroup::prepare(&layout, Some("exec"), &[]).unwrap() - } - }); + // Once the preparation created its cgroup, the racer opens that cgroup, within the pause, + // removes it, and prepares another of the same name, unless the preparation holds it off. + // The preparation then fails, as the kernel refuses its value, and removes its cgroup + // again. + let racer = race( + { + let layout = layout.clone(); + move || ExecutionCgroup::open(&layout, Some("exec")).unwrap() + }, + { + let layout = layout.clone(); + move |opened: ExecutionCgroup| { + opened.remove().unwrap(); + ExecutionCgroup::prepare(&layout, Some("exec"), &[]).unwrap() + } + }, + ); assert_eq!( error(ExecutionCgroup::prepare( &layout,