Re: [PATCH v6 02/19] tools: Add control group manager

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 20586 bytes --]

On Wed, Aug 05, 2026 at 09:16:09PM -0400, Demi Marie Obenour wrote:
> The cgroup-setup Rust program can create and purge cgroups.  It can also
> wait for one to become empty, spawn a program in a cgroup, and more.  In
> the future, it will also support cgroup-based resource control.  Locking
> is used to ensure that concurrent invocations are safe.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

Mostly just unclear comments/messages, so we're getting very close,
but some of my questions from last time still stand, too.  We should
figure those out before another round is submitted, because there's no
point in me seeing the same things and asking the same questions again
and again.

> diff --git a/tools/cgroup-setup/src/cgroup.rs b/tools/cgroup-setup/src/cgroup.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..c63d7e5a4aa73429578704401c58bbafc79e7c3f
> --- /dev/null
> +++ b/tools/cgroup-setup/src/cgroup.rs
> @@ -0,0 +1,269 @@
> +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>
> +// SPDX-License-Identifier: EUPL-1.2+
> +
> +use std::ffi::OsStr;
> +use std::fs::File;
> +use std::io::{Read as _, Seek as _, Write as _};
> +use std::os::unix::prelude::*;
> +
> +use std::path::{Component, Path, PathBuf};
> +
> +use rustix::fs::{AtFlags, CWD, Dir, FlockOperation};
> +use rustix::path;
> +use rustix::{
> +    fs::{Mode, OFlags, ResolveFlags},
> +    io::Errno,
> +};
> +
> +pub enum OpenFlags {
> +    Read,
> +    Write,
> +    Directory,
> +}
> +
> +#[derive(Debug)]
> +pub(crate) struct Cgroup {
> +    fd: Vec<OwnedFd>,
> +}
> +
> +impl AsFd for Cgroup {
> +    fn as_fd(&self) -> BorrowedFd<'_> {
> +        self.fd.last().unwrap().as_fd()
> +    }
> +}
> +
> +fn assert_single_component(component: &Path) {
> +    match component.as_os_str().as_bytes() {
> +        b"" | b"." | b".." => panic!("bad component"),
> +        c if c.contains(&b'\0') => panic!("NUL in component"),
> +        c if c.contains(&b'/') => panic!("/ in component"),
> +        _ => {}
> +    }
> +}
> +
> +// Wrapper around openat2() with better defaults.
> +pub fn openat2_simple(
> +    fd: impl AsFd,
> +    path: impl path::Arg,
> +    flags: OpenFlags,
> +) -> Result<OwnedFd, Errno> {
> +    rustix::fs::openat2(
> +        fd.as_fd(),
> +        path,
> +        OFlags::CLOEXEC
> +            | match flags {
> +                OpenFlags::Read => OFlags::RDONLY | OFlags::NOCTTY,
> +                OpenFlags::Write => OFlags::WRONLY | OFlags::NOCTTY,
> +                OpenFlags::Directory => OFlags::RDONLY | OFlags::DIRECTORY,
> +            },
> +        Mode::empty(),
> +        ResolveFlags::NO_SYMLINKS | ResolveFlags::NO_MAGICLINKS | ResolveFlags::NO_XDEV,
> +    )
> +}
> +
> +pub const DEFAULT_LEAF: &str = "$inner.service";
> +
> +pub fn check_path(path: &Path) -> Result<(), String> {
> +    let bytes = path.as_os_str().as_bytes();
> +    // Path::components() skips ., so use string manipulation instead.
> +    for component in bytes[path.is_absolute() as usize..].split(|&b| b == b'/') {
> +        if matches!(component, b"" | b"." | b"..") {
> +            return Err(format!("cgroup path {path:?} isn't canonical"));
> +        }
> +    }
> +    Ok(())
> +}
> +
> +// Remove all subdirectories of the given directory recursively,
> +// but not the directory itself.  The directory file descriptor
> +// is closed.
> +//
> +// This isn't the most efficient possible algorithm, but
> +// simplicity is more important than performance in this
> +// case.  Also, it keeps open more file descriptors than
> +// strictly necessary, but Spectrum runs with a very high
> +// limit for the number of open file descriptors, and it
> +// uses shallow control group hierarchies.
> +//
> +// This uses a recursive algorith, but so does std::fs::remove_dir_all().
> +// Trying to be more robust than the standard library is not worthwhile.
> +// In particular, the standard library function must be safe on systems
> +// where untrusted users (or even network endpoints!) can create deeply
> +// nested directory trees, whereas in Spectrum cgroups are only writeable
> +// by root.

This comment seems to be wrapped at two different widths.  Per Rust
style I think it would be idiomatic for it to be wrapped at 83 (80 from
the start of text after the comment marker and space).

> +fn remove_recursively(mut dirfd: Dir, remaining_depth: usize) -> Result<(), Errno> {
> +    if remaining_depth < 1 {
> +        panic!("control groups too deeply nested");
> +    }
> +    while let Some(element) = dirfd.next() {
> +        let parent_fd = dirfd.fd().unwrap();
> +        let element = element.expect("Iterating through a cgroup directory failed?");
> +        let path = element.file_name();
> +        if element.file_type() != rustix::fs::FileType::Directory || path == c"." || path == c".." {
> +            continue;
> +        }
> +        let fd = openat2_simple(parent_fd, path, OpenFlags::Directory)?;
> +        remove_recursively(Dir::new(fd).unwrap(), remaining_depth - 1)?;
> +        match rustix::fs::unlinkat(parent_fd, path, AtFlags::REMOVEDIR) {
> +            Err(Errno::NOTEMPTY | Errno::BUSY | Errno::NOENT) | Ok(()) => {}

I assume this BUSY exception is for the child cgroup reason explained
in a comment later?  It probably ought to be explained here too,
because it's not all obvious that a function named remove_recursively
would silently ignore EBUSY.

> +            bad => return bad,
> +        }
> +    }
> +    Ok(())
> +}
> +
> +// If the path is absolute, make it relative.
> +// Otherwise, read the current cgroup from /proc/thread-self/cgroup
> +// and prepend it to the path.

This makes it sound like it's doing two completely different things,
rather than  putting something into some standardized format.

> +fn prepend_current_cgroup_if_needed(path: &Path) -> PathBuf {
> +    if let Ok(suffix) = path.strip_prefix("/") {
> +        suffix.to_owned()
> +    } else {
> +        // /proc/thread-self is the same as /proc/self, except for the current
> +        // thread instead of the initial thread.  In this case, the two are
> +        // identical, but using /proc/thread-self is better practice as it is
> +        // correct in more cases.  Reading /proc/thread-self/cgroup should
> +        // never fail unless the system is seriously broken.
> +        let current_cgroup = std::fs::read("/proc/thread-self/cgroup")
> +            .expect("cannot read /proc/thread-self/cgroup");
> +        // Using this on a system without cgroups v2 mounted is user error
> +        // and not supported.
> +        let current_cgroup = current_cgroup
> +            .strip_prefix(b"0::/")
> +            .and_then(|e| e.strip_suffix(b"\n"))
> +            .expect("you don't have cgroups v2 mounted");

Not strictly the cause of the error.  You don't need cgroupfs mounted
to read /proc/thread-self/cgroup I assume.  The actual problem would
be that you're in a v1 cgroup, right?

> +        let mut current_cgroup = PathBuf::from(OsStr::from_bytes(current_cgroup));
> +        // Strip the implied $inner.service suffix.
> +        // This is used to satisfy the "no internal processes" rule.
> +        if current_cgroup.ends_with(Path::new(DEFAULT_LEAF)) {
> +            assert!(current_cgroup.pop());
> +        }
> +        current_cgroup.push(path);
> +        current_cgroup
> +    }
> +}
> +
> +pub(crate) fn write_value(fd: &dyn AsFd, name: &Path, value: &[u8]) -> Result<(), String> {
> +    let fd = openat2_simple(fd, name, OpenFlags::Write)
> +        .map_err(|e| format!("Cannot open {name:?}: {e}"))?;
> +    File::from(fd).write_all(value).map_err(|e| {
> +        format!(
> +            "Cannot write {:?} to {name:?}: {e}",
> +            OsStr::from_bytes(value)
> +        )
> +    })
> +}
> +
> +impl Cgroup {
> +    pub fn new(path: &Path) -> Result<Self, String> {
> +        let cgroup_root = rustix::fs::openat2(
> +            CWD,
> +            Path::new("/sys/fs/cgroup"),
> +            OFlags::CLOEXEC | OFlags::DIRECTORY | OFlags::RDONLY,
> +            Mode::empty(),
> +            ResolveFlags::NO_SYMLINKS | ResolveFlags::NO_MAGICLINKS,
> +        )
> +        .map_err(|e| format!("Cannot open /sys/fs/cgroup: {e}"))?;
> +        let mut cgroup = Self {
> +            fd: vec![(cgroup_root)],
> +        };

We don't take a lock on this, so what's it kept around for?

> +
> +        let path = prepend_current_cgroup_if_needed(path);
> +        for component in path.components() {
> +            let Component::Normal(component) = component else {
> +                unreachable!()
> +            };
> +            let sub_fd = openat2_simple(&cgroup, component, OpenFlags::Directory)
> +                .map_err(|e| format!("Cannot open sub-cgroup {component:?}: {e}"))?;
> +            // Take a shared lock on the cgroup.
> +            rustix::fs::flock(&sub_fd, FlockOperation::LockShared)
> +                .map_err(|e| format!("Cannot lock sub-cgroup {component:?}: {e}"))?;
> +            cgroup.fd.push(sub_fd);
> +        }
> +        Ok(cgroup)
> +    }
> +
> +    pub fn wait_for_empty(fd: &dyn AsFd) -> std::io::Result<()> {
> +        let wait_file = openat2_simple(fd, c"cgroup.events", OpenFlags::Read)?;
> +        let mut wait_fd = File::from(wait_file);
> +        let mut v = vec![];
> +        loop {
> +            v.clear();
> +            wait_fd
> +                .seek(std::io::SeekFrom::Start(0))
> +                .expect("Seek on control group file should succeed");
> +            wait_fd
> +                .read_to_end(&mut v)
> +                .expect("reading from control group should work");
> +            // Check that the cgroup isn't already empty.  If it was,
> +            // the kernel would not send an event and poll() would wait
> +            // forever.
> +            if v.split(|&c| c == b'\n').any(|line| line == b"populated 0") {
> +                break;
> +            }
> +            let mut fds = libc::pollfd {
> +                fd: wait_fd.as_raw_fd(),
> +                events: libc::POLLPRI | libc::POLLERR,
> +                revents: 0,
> +            };
> +            // SAFETY: FFI call, valid arguments, fds contains 1 element
> +            if unsafe { libc::poll(&raw mut fds, 1, -1) } != 1 {
> +                panic!("poll failed");
> +            }
> +        }
> +        drop(wait_fd);

This happens automatically.

> +        Ok(())
> +    }
> +
> +    pub fn purge_child(&mut self, path: &Path) -> Result<(), String> {
> +        assert_single_component(path);
> +        // See if we can just delete the child directly.
> +        match rustix::fs::unlinkat(&self, Path::new(path), AtFlags::REMOVEDIR) {

path is already a Path.

> +            // If the cgroup was successfully deleted, or if it
> +            // has already been deleted, we are done.
> +            Ok(()) | Err(Errno::NOENT) => return Ok(()),
> +            // If this cgroup is in use, keep going.
> +            Err(Errno::BUSY) => {}
> +            Err(e) => return Err(format!("Cannot purge {path:?}: {e}")),
> +        }
> +
> +        let sub_fd = match openat2_simple(&self, path, OpenFlags::Directory) {
> +            Ok(sub_fd) => sub_fd,

Maybe it would be nicer to do the Dir::new here?

> +            Err(Errno::NOENT) => return Ok(()),
> +            Err(e) => {
> +                return Err(format!("Cannot open sub-cgroup {path:?}: {e}",));
> +            }
> +        };
> +
> +        // Take an exclusive lock on the cgroup that is about to be
> +        // removed.  This avoids concurrent executions of this program
> +        // operating on deleted sub-cgroups.
> +        rustix::fs::flock(&sub_fd, FlockOperation::LockExclusive)
> +            .map_err(|e| format!("Cannot lock sub-cgroup: {e}"))?;

Wow, if this is all the locking we need, that's a great simplification!

> +
> +        // Kill all processes in the child cgroup.
> +        write_value(&sub_fd, Path::new("cgroup.kill"), b"1")?;
> +
> +        // Wait for the child cgroup to become empty.
> +        Self::wait_for_empty(&sub_fd)
> +            .map_err(|e| format!("Cannot wait for cgroup to become empty: {e}"))
> +            .inspect_err(|_| {
> +                self.fd.pop().unwrap();
> +            })?;
> +
> +        // Remove the child cgroup and its contents recursively.
> +        remove_recursively(Dir::new(sub_fd).unwrap(), 1000)
> +            .map_err(|e| format!("Cannot remove: {e}"))?;
> +
> +        // Delete the cgroup.  If it's been re-created in the meantime
> +        // and is currently in use, this is not an error.  Another
> +        // process deleting the cgroup is also not an error.  Both of
> +        // these can happen because of the time period between
> +        // remove_child_directories() closing the file descriptor
> +        // (releasing its lock) and the above call to flock().
> +        match rustix::fs::unlinkat(&self, path, AtFlags::REMOVEDIR) {
> +            Ok(()) | Err(Errno::BUSY) | Err(Errno::NOENT) => Ok(()),
> +            Err(e) => Err(format!("Cannot delete: {e}")),
> +        }
> +    }
> +}
> diff --git a/tools/cgroup-setup/src/main.rs b/tools/cgroup-setup/src/main.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..58ca7295bb348a2c92640a68c60b285b2d7a1494
> --- /dev/null
> +++ b/tools/cgroup-setup/src/main.rs
> @@ -0,0 +1,167 @@
> +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>
> +// SPDX-License-Identifier: EUPL-1.2+
> +
> +mod cgroup;
> +
> +use cgroup::{Cgroup, OpenFlags, openat2_simple, write_value};
> +use rustix::{
> +    fs::{FlockOperation, Mode, XattrFlags},
> +    io::Errno,
> +};
> +use std::{
> +    env::ArgsOs,
> +    fs::File,
> +    io::Read as _,
> +    os::unix::prelude::*,
> +    path::{Path, PathBuf},
> +};
> +
> +// Check that the path is canonical,
> +// then split it into basename and filename.
> +fn split_path(path: &Path) -> Result<(&Path, &Path), String> {
> +    cgroup::check_path(path)?;
> +    Ok((path.parent().unwrap(), Path::new(path.file_name().unwrap())))
> +}
> +
> +fn enable_subtree_control(fd: &dyn AsFd) -> Result<(), String> {
> +    let mut buf = Vec::new();
> +    File::from(
> +        openat2_simple(fd, c"cgroup.controllers", OpenFlags::Read)
> +            .map_err(|e| format!("Cannot open cgroup.controllers: {e}"))?,
> +    )
> +    .read_to_end(&mut buf)
> +    .map_err(|e| format!("Cannot read cgroup.controllers: {e}"))?;
> +    let mut subtree = vec![];
> +    for controller in buf.split(|&b| b == b' ') {
> +        if !subtree.is_empty() {
> +            subtree.push(b' ');
> +        }
> +        subtree.push(b'+');
> +        subtree.extend_from_slice(controller);
> +    }
> +    if !subtree.is_empty() {
> +        write_value(&fd, Path::new("cgroup.subtree_control"), &subtree)?;
> +    }
> +    Ok(())
> +}
> +
> +fn cgroup_setup(args: ArgsOs) -> Result<(), String> {
> +    let mut leaf = false;
> +    let mut systemd_delegate = false;
> +    let mut wait = true;
> +    let mut args = args.peekable();
> +    while let Some(arg) = args.peek() {
> +        if !arg.as_bytes().starts_with(b"-") {
> +            break;
> +        }
> +        let arg = args.next().unwrap();
> +        let Some(arg_) = arg.as_bytes().strip_prefix(b"--") else {

Why not just shadow arg?

> +            return Err("takes no short options".to_owned());
> +        };
> +        match arg_ {
> +            b"" => break,
> +            b"leaf" => leaf = true,
> +            b"no-wait" => wait = false,
> +            b"systemd-delegate" => systemd_delegate = true,
> +            _ => return Err(format!("unknown long option {arg:?}")),
> +        }
> +    }

Lovely and straightforward now.

> +    let Some(cgroup_path) = args.next().map(PathBuf::from) else {
> +        return Err("have no positional arguments, expected at least 1".to_owned());
> +    };
> +
> +    let (parent_cgroup_path, child_cgroup_path) = split_path(&cgroup_path)?;
> +    let cgroup = Cgroup::new(parent_cgroup_path)?;
> +    match rustix::fs::mkdirat(&cgroup, child_cgroup_path, Mode::from_raw_mode(0o755)) {
> +        Ok(()) | Err(Errno::EXIST) => {}
> +        Err(e) => {
> +            return Err(format!(
> +                "Cannot make child cgroup {child_cgroup_path:?}: {e}"
> +            ));
> +        }
> +    }
> +    let child = openat2_simple(&cgroup, child_cgroup_path, OpenFlags::Directory)
> +        .map_err(|e| format!("Cannot make child cgroup: {e}"))?;

It's not correct to say "make" here.

> +    // While waiting, hold an exclusive lock on the child.
> +    // This avoids two processes both waiting for the same cgroup to become
> +    // empty, then spawning processes in the same cgroup.
> +    rustix::fs::flock(&child, FlockOperation::LockExclusive)
> +        .map_err(|e| format!("Cannot take an exclusive lock on child cgroup: {e}"))?;
> +    if wait {
> +        Cgroup::wait_for_empty(&child)
> +            .map_err(|e| format!("Cannot wait for {parent_cgroup_path:?} to be empty: {e}"))?;
> +    }
> +    let pid = std::process::id().to_string();
> +    if leaf {
> +        if args.len() != 0 {
> +            // If we aren't delegating any cgroups, don't create a sub-cgroup.
> +            write_value(&child, Path::new("cgroup.procs"), pid.as_bytes())
> +                .map_err(|e| format!("Cannot move process to child cgroup: {e}"))?;
> +        }
> +    } else {
> +        // If the child process will need to manage cgroups itself, it will need
> +        // to set up a sub-cgroup due to the "no internal processes" rule.  It's
> +        // simplest to just do it automatically.  If the cgroup already exists,
> +        // that isn't an error.
> +        match rustix::fs::mkdirat(&child, cgroup::DEFAULT_LEAF, Mode::from_raw_mode(0o755)) {
> +            Ok(()) | Err(Errno::EXIST) => {}
> +            Err(e) => return Err(format!("Cannot make child cgroup: {e}")),
> +        }
> +        if args.len() != 0 {
> +            let child_proc_path = Path::new(cgroup::DEFAULT_LEAF).join(Path::new("cgroup.procs"));
> +            write_value(&child, &child_proc_path, pid.as_bytes())
> +                .map_err(|e| format!("Cannot move process to child cgroup: {e}"))?;
> +        }

We're still complicating this by insisting on leaf mode being
different, for the extremely nebulous cause of not wasting cgroups.

> +        if systemd_delegate {
> +            // systemd-aware programs expect to have user.delegate=1
> +            // and to set cgroup.subtree_control themselves
> +            rustix::fs::fsetxattr(&child, c"user.delegate", b"1", XattrFlags::empty()).map_err(
> +                |e| format!("Cannot enable cgroup delegation in {parent_cgroup_path:?}: {e}"),
> +            )?
> +        } else {
> +            // Spectrum's programs do not check for user.delegate=1
> +            // and expect the caller to set cgroup.subtree_control.
> +            enable_subtree_control(&child)?;

My question from last time about doing this unconditionally has not
been answered as far as I can see.

> +        }
> +    }
> +    let Some(program_name) = args.next() else {
> +        return Ok(());
> +    };
> +    let e = std::process::Command::new(&program_name).args(args).exec();
> +    Err(format!("Cannot spawn child {program_name:?}: {e}",))
> +}
> +
> +fn cgroup_purge(mut args: ArgsOs) -> Result<(), String> {
> +    if args.len() != 1 {
> +        return Err("usage: cgroup-purge CGROUP_TO_PURGE".to_owned());
> +    }
> +    let arg = args.next().unwrap();
> +    let (parent, child) = split_path(Path::new(&arg))?;
> +    Cgroup::new(parent)?.purge_child(child)
> +}
> +
> +fn run(prog_name: &Path, args: ArgsOs) -> Result<(), String> {
> +    match prog_name.file_name().map(|f| f.as_bytes()) {
> +        Some(b"cgroup-setup") => cgroup_setup(args),
> +        Some(b"cgroup-purge") => cgroup_purge(args),
> +        _ => Err(format!(
> +            "must be invoked as \"cgroup-setup\" or \
> +                 \"cgroup-purge\", got {prog_name:?}",
> +        )),
> +    }
> +}
> +
> +fn main() {
> +    let mut args = std::env::args_os();
> +    let Some(prog_name) = args.next() else {
> +        eprintln!("No command line arguments (argv[0] is NULL)");
> +        std::process::exit(1);
> +    };
> +    match run(Path::new(&prog_name), args) {
> +        Ok(()) => {}
> +        Err(e) => {
> +            eprintln!("{prog_name:?}: {}", e);
> +            std::process::exit(1);
> +        }
> +    }
> +}
>
> --
> 2.55.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]