Re: [PATCH v4 02/20] tools: Add control group manager

[Alyssa Ross <hi@alyssa.is>]

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

Demi Marie Obenour <demiobenour@gmail.com> writes:

> 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.
>
> This program can also be used in an s6 finish script.  When passed the
> args of such a script, it automatically purges the correct cgroup.  It
> also tells s6 to not restart the service if it dumped core.  Core dumps
> are often due to memory corruption, and automatically restarting a
> service that dumped core makes memory corruption attacks easier.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
>  .codespellrc                               |   2 +-
>  host/rootfs/default.nix                    |   6 +-
>  host/rootfs/file-list.mk                   |   2 +
>  host/rootfs/image/usr/bin/cgroup-purge     |   1 +
>  host/rootfs/image/usr/bin/cgroup-s6-finish |   1 +
>  pkgs/default.nix                           |   1 +
>  tools/cgroup-setup/Cargo.lock              |  67 ++++++
>  tools/cgroup-setup/Cargo.lock.license      |   2 +
>  tools/cgroup-setup/Cargo.toml              |  11 +
>  tools/cgroup-setup/default.nix             |  18 ++
>  tools/cgroup-setup/src/cgroup.rs           | 349 +++++++++++++++++++++++++++++
>  tools/cgroup-setup/src/main.rs             | 347 ++++++++++++++++++++++++++++
>  12 files changed, 803 insertions(+), 4 deletions(-)

> diff --git a/host/rootfs/file-list.mk b/host/rootfs/file-list.mk
> index 3899d620717fc97f42e669e5313c4100dcf5b1cd..e1280ab56d8797e40b9b1c584ab0daef3cda41d7 100644
> --- a/host/rootfs/file-list.mk
> +++ b/host/rootfs/file-list.mk
> @@ -79,6 +79,8 @@ LINKS = \
>  	image/etc/s6-linux-init/run-image/service/vmm/template/run \
>  	image/lib \
>  	image/sbin \
> +	image/usr/bin/cgroup-purge \
> +	image/usr/bin/cgroup-s6-finish \
>  	image/usr/bin/systemd-udevd
>  
>  S6_RC_FILES = \
> diff --git a/host/rootfs/image/usr/bin/cgroup-purge b/host/rootfs/image/usr/bin/cgroup-purge
> new file mode 120000
> index 0000000000000000000000000000000000000000..a0c8d8e144d72b69c613eb0613e39acc9df979df
> --- /dev/null
> +++ b/host/rootfs/image/usr/bin/cgroup-purge
> @@ -0,0 +1 @@
> +cgroup-setup
> \ No newline at end of file
> diff --git a/host/rootfs/image/usr/bin/cgroup-s6-finish b/host/rootfs/image/usr/bin/cgroup-s6-finish
> new file mode 120000
> index 0000000000000000000000000000000000000000..a0c8d8e144d72b69c613eb0613e39acc9df979df
> --- /dev/null
> +++ b/host/rootfs/image/usr/bin/cgroup-s6-finish
> @@ -0,0 +1 @@
> +cgroup-setup
> \ No newline at end of file

Usually packages that expect to be invoked via symlinks like this
(coreutils, busybox, execline) install their own symlinks, rather than
expecting systems to create them.  I think these would be more
appropriate in a postInstall in tools/cgroup-setup/default.nix.

> diff --git a/pkgs/default.nix b/pkgs/default.nix
> index 44f7b5ff78cb6b9e755292a6a417d0b627ed3fb0..0a13393164ad5d7f752e630763f3f97166479af5 100644
> --- a/pkgs/default.nix
> +++ b/pkgs/default.nix
> @@ -51,6 +51,7 @@ let
>        driverSupport = true;
>      };
>      spectrum-router = self.callSpectrumPackage ../tools/router {};
> +    spectrum-cgroup-setup = self.callSpectrumPackage ../tools/cgroup-setup {};
>      xdg-desktop-portal-spectrum-host =
>        self.callSpectrumPackage ../tools/xdg-desktop-portal-spectrum-host {};
>  
> diff --git a/tools/cgroup-setup/Cargo.lock b/tools/cgroup-setup/Cargo.lock
> new file mode 100644
> index 0000000000000000000000000000000000000000..fe967b3aa02c296c87b6b36ac59253dbe0a32de9
> --- /dev/null
> +++ b/tools/cgroup-setup/Cargo.lock
> @@ -0,0 +1,67 @@
> +# This file is automatically @generated by Cargo.
> +# It is not intended for manual editing.
> +version = 4
> +
> +[[package]]
> +name = "bitflags"
> +version = "2.11.1"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "c4512299f36f043ab09a583e57bceb5a5aab7a73db1805848e8fef3c9e8c78b3"
> +
> +[[package]]
> +name = "cgroup-setup"
> +version = "0.0.0"
> +dependencies = [
> + "libc",
> + "rustix",
> +]
> +
> +[[package]]
> +name = "errno"
> +version = "0.3.14"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb"
> +dependencies = [
> + "libc",
> + "windows-sys",
> +]
> +
> +[[package]]
> +name = "libc"
> +version = "0.2.186"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61ece587de66"
> +
> +[[package]]
> +name = "linux-raw-sys"
> +version = "0.12.1"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53"
> +
> +[[package]]
> +name = "rustix"
> +version = "1.1.4"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190"
> +dependencies = [
> + "bitflags",
> + "errno",
> + "libc",
> + "linux-raw-sys",
> + "windows-sys",
> +]
> +
> +[[package]]
> +name = "windows-link"
> +version = "0.2.1"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5"
> +
> +[[package]]
> +name = "windows-sys"
> +version = "0.61.2"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "ae137229bcbd6cdf0f7b80a31df61766145077ddf49416a728b02cb3921ff3fc"
> +dependencies = [
> + "windows-link",
> +]
> diff --git a/tools/cgroup-setup/Cargo.lock.license b/tools/cgroup-setup/Cargo.lock.license
> new file mode 100644
> index 0000000000000000000000000000000000000000..aa108acd23886d8302eaf7babff90d1b08ae19fb
> --- /dev/null
> +++ b/tools/cgroup-setup/Cargo.lock.license
> @@ -0,0 +1,2 @@
> +SPDX-License-Identifier: EUPL-1.2+
> +SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>

I think this should be CC0-1.0 like every other Cargo.lock.license.
There's nothing copyrightable about it.

> diff --git a/tools/cgroup-setup/Cargo.toml b/tools/cgroup-setup/Cargo.toml
> new file mode 100644
> index 0000000000000000000000000000000000000000..7ed6d6a0ea3bbfc4064b9f39383d0788c4bd84e5
> --- /dev/null
> +++ b/tools/cgroup-setup/Cargo.toml
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: CC0-1.0
> +# SPDX-FileCopyrightText: 2025 Alyssa Ross <hi@alyssa.is>

I surely did not contribute anything copyrightable to this.

> +# SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +[package]
> +name = "cgroup-setup"
> +edition = "2024"
> +
> +[dependencies]
> +libc = "0.2.177"
> +rustix = { version = "1.1.2", features = ["fs"] }

> diff --git a/tools/cgroup-setup/src/cgroup.rs b/tools/cgroup-setup/src/cgroup.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..c953d26badfdac0a1e3d7057a867aec3b3247e18
> --- /dev/null
> +++ b/tools/cgroup-setup/src/cgroup.rs
> @@ -0,0 +1,349 @@
> +// SPDX-License-Identifier: EUPL-1.2+
> +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +use std::ffi::OsStr;
> +use std::fmt::Display;
> +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, FlockOperation, XattrFlags};
> +use rustix::{
> +    fs::{Mode, OFlags, ResolveFlags},
> +    io::Errno,
> +};
> +
> +#[derive(Debug)]
> +pub(crate) struct Cgroup {
> +    path: PathBuf,
> +    fd: Vec<(OwnedFd, bool)>,

There's no point storing all these exclusivity bools, is there?  I think
only the last one is ever checked, so we could make things tighter and
clearer like this, where we only track the exclusivity of the last fd:

    fd: Vec<OwnedFd>,
    exclusive: bool,

> +}
> +
> +impl AsFd for Cgroup {
> +    fn as_fd(&self) -> BorrowedFd<'_> {
> +        self.fd.last().unwrap().0.as_fd()
> +    }
> +}
> +
> +fn assert_single_component(component: &[u8]) {

Why not &Path, which is already guaranteed not to have a NUL byte?
Perhaps this whole thing could be simplified to
Some(component).as_os_str() == component.file_name()?  Maybe that's too
clever, though…

> +    match component {
> +        b"" | b"." | b".." => panic!("bad component"),
> +        _ if component.contains(&b'\0') => panic!("NUL in component"),
> +        _ if component.contains(&b'/') => panic!("/ in component"),
> +        _ => {}
> +    }
> +}
> +
> +impl Cgroup {
> +    pub fn new(exclusive: bool) -> Result<Self, String> {
> +        let cgroup_root = rustix::fs::openat2(
> +            rustix::fs::CWD,
> +            Path::new("/sys/fs/cgroup"),
> +            OFlags::DIRECTORY | OFlags::RDONLY | OFlags::CLOEXEC | OFlags::NOFOLLOW,
> +            Mode::empty(),
> +            ResolveFlags::NO_MAGICLINKS | ResolveFlags::NO_SYMLINKS,
> +        )

I pointed out in my review of v2 that OFlags::NOFOLLOW is redundant with
ResolveFlags::NO_SYMLINKS, but now it seesm to have come back across the
board.

> +        .map_err(|e| format!("Cannot open /sys/fs/cgroup: {e}"))?;
> +
> +        let lock_operation = if exclusive {
> +            FlockOperation::LockExclusive
> +        } else {
> +            FlockOperation::LockShared
> +        };
> +        rustix::fs::flock(cgroup_root.as_fd(), lock_operation)
> +            .map_err(|e| format!("Cannot lock /sys/fs/cgroup: {e}"))?;
> +        Ok(Self {
> +            path: PathBuf::from("/sys/fs/cgroup"),
> +            fd: vec![(cgroup_root, exclusive)],
> +        })
> +    }
> +
> +    pub fn enable_delegation(&self, depth: usize) -> Result<(), Errno> {
> +        let (fd, exclusive) = &self.fd[self.fd.len() - depth];
> +        assert!(exclusive);
> +        rustix::fs::fsetxattr(fd.as_fd(), c"user.delegate", b"1", XattrFlags::empty())
> +    }
> +
> +    pub fn enable_subtree_control(&self, depth: usize) -> Result<(), String> {
> +        let (fd, exclusive) = &self.fd[self.fd.len() - depth];
> +        assert!(exclusive);
> +        let p = Path::new("cgroup.controllers");
> +        let mut buf = self.read_control_file(fd.as_fd(), p)?;
> +        let mut subtree = vec![];
> +        if buf.ends_with(b"\n") {
> +            buf.pop();
> +        }
> +        for controller in buf.split(|&b| b == b' ').filter(|e| !e.is_empty()) {
> +            for &c in controller {
> +                if c <= b' ' || c >= 0x7F {
> +                    return Err(format!("Bad byte {c} in cgroup.controllers"));
> +                }
> +            }
> +            if !subtree.is_empty() {
> +                subtree.push(b' ');
> +            }
> +            subtree.push(b'+');
> +            subtree.extend_from_slice(controller);
> +        }
> +        if !subtree.is_empty() {
> +            self.write_cgroup_value("cgroup.subtree_control", str::from_utf8(&subtree).unwrap())?;
> +        }
> +        Ok(())
> +    }
> +
> +    pub fn read_control_file(&self, fd: BorrowedFd, p: &Path) -> Result<Vec<u8>, String> {
> +        let mut buf = Vec::new();
> +        let err = |e: &dyn Display, p: &Path, msg: &str| {
> +            let path = self.path.join(p);
> +            format!("Cannot {msg} {path:?}: {e}")
> +        };
> +        File::from(open_subtree_raw(Path::new(p), fd.as_fd()).map_err(|e| err(&e, p, "open"))?)

If we're using it for opening files, open_subtree_raw is probably misnamed.

> +            .read_to_end(&mut buf)
> +            .map_err(|e| err(&e, p, "read"))?;
> +        Ok(buf)
> +    }
> +
> +    /// Open a single component as a sub-cgroup
> +    fn open_sub_cgroup_raw(&self, access: OFlags, component: &[u8]) -> Result<OwnedFd, Errno> {
> +        assert_single_component(component);
> +        rustix::fs::openat2(
> +            self.as_fd(),
> +            Path::new(OsStr::from_bytes(component)),
> +            OFlags::CLOEXEC | OFlags::NOFOLLOW | access,
> +            Mode::empty(),
> +            ResolveFlags::NO_SYMLINKS
> +                | ResolveFlags::NO_MAGICLINKS
> +                | ResolveFlags::BENEATH
> +                | ResolveFlags::NO_XDEV,

This is also doing exactly the same thing as open_subtree_raw, except it
allows changing the access mode, and also sets NO_MAGICLINKS.  I don't
think any of the other callers of open_subtree_raw would need to open
magic links, so maybe this is evidence this should be unified with them?

> +        )
> +    }
> +
> +    pub fn open_sub_cgroup(
> +        &mut self,
> +        path: &std::path::Path,
> +        exclusive: bool,
> +        allow_missing: bool,
> +    ) -> Result<bool, String> {
> +        let mut iter = path.components().peekable();
> +        while let Some(component) = iter.next() {

Perhaps would be nicer:

let mut components = path.components().peekable();
for component in components {

> +            let component = match component {
> +                Component::Normal(component) => component,
> +                _ => unreachable!(),
> +            };
> +            let sub_fd = match self
> +                .open_sub_cgroup_raw(OFlags::DIRECTORY | OFlags::RDONLY, component.as_bytes())
> +            {
> +                Ok(sub_fd) => {
> +                    self.path.push(component);

I would really like to not try to store self.path.  It seems very
complicated to track.  It's also very unclear to me from the name (and
the code) what it is.  Is it the path to the cgroup itself, or to its
parent?  It looks to me like it should be the cgroup itself, but then
what's going on in purge?

We could actually improve readability of this quite complicated function
even further if you find it acceptable to just use Errno for the error
type.  In that case, we'd just return Result<(), Errno>, and callers
would check for Errno::NOENT if they wanted to allow missing.  Then we
could just completely drop that argument.  In my opinion it would be
worth it to move complexity out of here.

> +                    sub_fd
> +                }
> +                Err(Errno::NOENT) if allow_missing => return Ok(false),
> +                Err(e) => {
> +                    return Err(format!(
> +                        "Cannot open sub-cgroup {component:?} of {:?}: {e}",
> +                        self.path
> +                    ));
> +                }
> +            };
> +            let exclusive = exclusive && iter.peek().is_none();
> +            let lock_operation = if exclusive {
> +                FlockOperation::LockExclusive
> +            } else {
> +                FlockOperation::LockShared
> +            };
> +            rustix::fs::flock(sub_fd.as_fd(), lock_operation).map_err(|e| {
> +                let msg = format!("Cannot lock sub-cgroup {:?}: {e}", self.path);
> +                self.path.pop();
> +                msg
> +            })?;
> +            self.fd.push((sub_fd, exclusive));
> +        }
> +        Ok(true)
> +    }
> +
> +    pub fn open_subtree(&self, path: &std::path::Path) -> Result<OwnedFd, Errno> {
> +        let dirfd = self.as_fd();
> +        open_subtree_raw(path, dirfd)
> +    }

If open_subtree_raw just took &dyn AsFd, there'd be no need for this
method.

> +
> +    fn exclusive(&self) -> bool {
> +        self.fd.last().unwrap().1
> +    }
> +
> +    pub fn joined_path(&self, p: &Path) -> PathBuf {
> +        let mut owned_p = self.path.clone();
> +        owned_p.push(p);
> +        owned_p
> +    }
> +
> +    pub fn wait_for_empty(&self) -> std::io::Result<()> {
> +        assert!(self.exclusive());
> +        let wait_file = self.open_subtree(std::path::Path::new("cgroup.events"))?;
> +        let poll_fd = wait_file.as_raw_fd();
> +        let mut wait_fd = File::from(wait_file);
> +        let mut fds = libc::pollfd {
> +            fd: poll_fd,
> +            events: libc::POLLPRI | libc::POLLERR,
> +            revents: 0,
> +        };
> +        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");
> +            if v.split(|&c| c == b'\n').any(|line| line == b"populated 0") {
> +                break;
> +            }
> +            // SAFETY: FFI call, valid arguments, fds contains 1 element
> +            if unsafe { libc::poll(&raw mut fds, 1, -1) } != 1 {
> +                panic!("poll failed");
> +            }
> +        }

Are you 100% confident that this doesn't race?  I don't understand why
poll would be triggered in this scenario:

 1. "1" is written to cgroup.kill
 2. Every process in the cgroup exits and is reaped.
 3. cgroup.events is opened, with the cgroup already empty.

Are you not relying on 2 happening after 3?  Presumably if you open
cgroup.events for a cgroup that's already empty, you're not going to get
a poll event to tell you it's empty.

> +        Ok(())
> +    }
> +
> +    pub(crate) fn make_child(&mut self, path: &Path) -> Result<(), Errno> {
> +        assert!(self.exclusive());
> +        let component = path.as_os_str().as_bytes();
> +        assert_single_component(component);
> +        match rustix::fs::mkdirat(
> +            self.as_fd(),
> +            path,
> +            Mode::RUSR
> +                | Mode::WUSR
> +                | Mode::XUSR
> +                | Mode::RGRP
> +                | Mode::XGRP
> +                | Mode::ROTH
> +                | Mode::XOTH,
> +        ) {
> +            Ok(()) | Err(Errno::EXIST) => {}
> +            bad => return bad,
> +        }
> +        let p = self.open_sub_cgroup_raw(OFlags::RDONLY | OFlags::DIRECTORY, component)?;
> +        // exclusive lock on parent acts as exclusive lock on child
> +        self.fd.push((p, true));
> +        self.path.push(path);
> +        Ok(())
> +    }
> +
> +    pub(super) fn purge(&mut self, path: &Path) -> Result<(), String> {

I guess we have to call purge on the parent, rather than on the cgroup
itself, because of the unlink?  Maybe we could call it purge_child?  It
confused me for a while.

> +        assert!(self.exclusive());
> +        match rustix::fs::unlinkat(self.as_fd(), Path::new(path), AtFlags::REMOVEDIR) {
> +            // Trying to purge a deleted cgroup is not an error.
> +            Ok(()) | Err(Errno::NOENT) => return Ok(()),
> +            Err(Errno::BUSY) => {}

This could use a comment.

> +            Err(e) => return Err(format!("Cannot purge {:?}: {e}", self.joined_path(path))),
> +        }
> +        if !self.open_sub_cgroup(path, true, true)? {
> +            return Ok(());
> +        }
> +
> +        rustix::fs::flock(
> +            self.fd[self.fd.len() - 2].0.as_fd(),
> +            FlockOperation::LockShared,

We already must have at least a shared lock on this at this point, no?
I don't think we need another one.

> +        )
> +        .map_err(|e| format!("Cannot relock {:?}: {e}", self.path.parent()))?;
> +        self.write_cgroup_value("cgroup.kill", "1")?;
> +        self.wait_for_empty()
> +            .map_err(|e| format!("Cannot wait for cgroup to become empty: {e}"))?;
> +        let fd = self.fd.pop().unwrap().0;
> +        let v = (|| {
> +            remove_recursively(fd, 1000)
> +                .map_err(|e| format!("Cannot remove {:?}: {e}", self.path))?;
> +            rustix::fs::flock(self.as_fd(), FlockOperation::LockExclusive)
> +                .map_err(|e| format!("Cannot lock {:?}: {e}", self.path))?;

We already checked self.exclusive above, meaning we already have this
lock on self?

> +            match rustix::fs::unlinkat(
> +                self.as_fd(),
> +                Path::new(self.path.file_name().unwrap()),

I am too confused about what self.path is to know what to make of this.
self.as_fd() should be the fd of the cgroup directory, and self.path
sounds like it should be the path to this cgroup, so how can this cgroup
directory have self.path.file_name() within it?  This needs clearer
names or a refactor or something.

> +                AtFlags::REMOVEDIR,
> +            ) {
> +                // something might have re-created the cgroup in the meantime, which is okay
> +                Ok(()) | Err(Errno::BUSY) => Ok(()),
> +                Err(e) => Err(format!("Cannot lock {:?}: {e}", self.path)),
> +            }
> +        })();
> +        assert!(self.path.pop());

I don't get this.  Where was it pushed?  (But would prefer to just not
attempt to track this, as mentioned above.)

> +        v
> +    }
> +
> +    pub(crate) fn write_cgroup_value(&self, name: &str, value: &str) -> Result<(), String> {
> +        let path = Path::new(name);
> +        let fd = rustix::fs::openat2(
> +            self.as_fd(),
> +            path,
> +            OFlags::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::WRONLY,
> +            Mode::empty(),
> +            ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
> +        )
> +        .map_err(|e| format!("Cannot open {:?}: {}", self.joined_path(Path::new(name)), e))?;

Could also use ResolveFlags::MAGIC_LINKS and be unified with the other
openat2 invocations maybe?  I don't understand why this is NOATIME but
others aren't.

> +        File::from(fd).write_all(value.as_bytes()).map_err(|e| {
> +            format!(
> +                "Cannot write {:?} to {:?}: {}",
> +                value,
> +                self.joined_path(Path::new(name)),
> +                e
> +            )
> +        })
> +    }
> +}
> +
> +fn open_subtree_raw(path: &Path, dirfd: BorrowedFd<'_>) -> Result<OwnedFd, Errno> {
> +    rustix::fs::openat2(
> +        dirfd,
> +        path,
> +        OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY,
> +        Mode::empty(),
> +        ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
> +    )
> +}
> +
> +fn remove_recursively(fd: OwnedFd, remaining_depth: usize) -> Result<(), Errno> {
> +    if remaining_depth < 1 {
> +        panic!("control groups too deeply nested");
> +    }
> +    let mut d = rustix::fs::Dir::new(fd).expect("cannot start iterating");
> +    while let Some(element) = d.next() {
> +        let element = element.expect("Iterating through a cgroup directory failed?");
> +        if element.file_type() != rustix::fs::FileType::Directory {
> +            continue;
> +        }
> +
> +        let remaining_depth = remaining_depth - 1;
> +        let d: &rustix::fs::Dir = &d;
> +        let dirfd = d.fd().unwrap();
> +        let path = element.file_name();
> +        remove_all(remaining_depth, dirfd, path)?;
> +    }
> +    drop(d);
> +    Ok(())
> +}
> +
> +fn remove_all(
> +    remaining_depth: usize,
> +    dirfd: BorrowedFd<'_>,
> +    path: &std::ffi::CStr,
> +) -> Result<(), Errno> {
> +    if path == c"." || path == c".." {
> +        return Ok(());
> +    }
> +    if rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR).is_ok() {
> +        return Ok(());
> +    }
> +    let fd = rustix::fs::openat2(
> +        dirfd,
> +        path,
> +        OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY | OFlags::DIRECTORY,
> +        Mode::empty(),
> +        ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
> +    )?;
> +    remove_recursively(fd, remaining_depth)?;
> +    rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR)?;
> +    Ok(())
> +}

Could we save a lot of code by calling std::fs::remove_dir_all with a
/proc/self/fd path?  It's already documented to ignore symlinks.

> diff --git a/tools/cgroup-setup/src/main.rs b/tools/cgroup-setup/src/main.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..2e7a4e25213a4aa2449b292f8005e1da23cbb1f9
> --- /dev/null
> +++ b/tools/cgroup-setup/src/main.rs
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: EUPL-1.2+
> +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +use std::{
> +    ffi::{OsStr, OsString},
> +    os::unix::prelude::*,
> +    path::{Path, PathBuf},
> +};
> +
> +use crate::cgroup::Cgroup;
> +
> +mod cgroup;
> +
> +fn check_path(path: &OsStr) -> Result<(), String> {
> +    if path.is_empty() {
> +        return Ok(());
> +    }
> +
> +    for component in path.as_bytes().split(|&b| b == b'/') {

Would be a lot nicer to take &Path and use Path::components.

> +        match component {
> +            b"" | b"." | b".." => {
> +                return Err(format!("Path {path:?} has empty, ., or .. component"));
> +            }
> +            // Cannot happen: command line arguments have no NUL byte,
> +            // and /proc/self/cgroup having a NUL byte is a kernel bug.
> +            _ if component.contains(&b'\0') => panic!("Path {path:?} has NUL byte"),

It is in fact an invariant of both the Path and OsStr types (on Unix)
that there are no NUL bytes, so there is no need to check for it at all.

> +            _ if component.len() > 255 => {
> +                return Err(format!(
> +                    "Path {path:?} has component {:?} that is longer than 255 bytes",
> +                    OsStr::from_bytes(component)
> +                ));
> +            }

Why on earth would we need to check for this?  This limit is totally up
to the kernel.  Our code won't break with an excessively long path
component.

> +            _ => {}
> +        }
> +    }
> +
> +    Ok(())
> +}
> +
> +/// Get the path of the cgroup for the provided command-line argument.
> +/// Returns an empty path if the path is "/", or if it is "." and the
> +/// current cgroup is "/".

Please standardize terminology between "current cgroup" and "local
cgroup", or if those are not the same thing use clearer phrasing.

I hope all the different modes of this function are actually needed…
(I assume they are but haven't reviewed the patches that make use of it
yet.)

> +///
> +/// # Errors
> +///
> +/// Fails if the provided path is invalid or empty, or if it is relative
> +/// and the local cgroup cannot be determined.
> +fn get_cgroup(cgroup_path: OsString) -> Result<PathBuf, String> {

Please call this function something clearer.  Also using OsString where
PathBuf would be more appropriate again…

> +    if cgroup_path.as_bytes().starts_with(b"/") {

… which would allow using Path::is_absolute if fixed.

> +        let mut cgroup_path = cgroup_path.into_vec();
> +        cgroup_path.remove(0);

Please use Path functions rather than byte manipulation like this.

> +        if cgroup_path.is_empty() {
> +            return Err("cgroup path cannot be /".to_owned());
> +        }
> +        let cgroup_path = OsString::from_vec(cgroup_path);
> +        check_path(&cgroup_path)?;
> +        Ok(cgroup_path.into())
> +    } else if cgroup_path.is_empty() {
> +        Err("cgroup path cannot be empty".to_owned())
> +    } else {
> +        check_path(&cgroup_path)?;
> +        let mut local_cgroup = local_cgroup()?;

Can you reorder all these function definitions to be something more
sensible?  get_cgroup is here, near the start of main.rs, but
local_cgroup, which it is the only direct caller of, is all the way at
the other end.  (Personally I like to define utility functions directly
above their only caller — I think that creates the most natural reading
flow.)

> +        local_cgroup.push(cgroup_path);
> +        Ok(local_cgroup)
> +    }
> +}
> +
> +/// Open the cgroup corresponding to the provided path.
> +/// It must have already been made relative to `/sys/fs/cgroup`.
> +///
> +/// # Errors
> +///
> +/// Fails if the cgroup operation fails.

This "Errors" section is just stating the obvious.

> +fn open_cgroup(path: &Path, exclusive: bool) -> Result<Cgroup, String> {
> +    if path.as_os_str().is_empty() {
> +        Cgroup::new(exclusive)
> +    } else {
> +        let mut cgroup = Cgroup::new(false)?;
> +        cgroup.open_sub_cgroup(path, exclusive, false)?;
> +        Ok(cgroup)
> +    }
> +}

I feel like it would be more natural if Cgroup::new just took two
parameters and worked this way.

> +
> +/// Open the cgroup corresponding to the provided path's parent.
> +/// It is made relative to the process's own cgroup if needed.
> +///
> +/// # Errors
> +///
> +/// Fails if the cgroup operation fails.
> +fn open_relative_cgroup(arg: OsString) -> Result<(PathBuf, Cgroup), String> {
> +    let path = get_cgroup(arg)?;
> +    let cgroup = open_cgroup(path.parent().expect("always has a parent"), true)?;
> +    Ok((path, cgroup))
> +}

This is a deeply confusing function.  Why is opening a cgroup for the
path's _parent_ an operation that we should have a dedicated function
for?  Why not have the caller pass in the actual path of the cgroup it
wants to open?  This really looks like a function that's doing as many
different things at it has lines, and should just be inlined.

> +
> +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 main_(&prog_name, args) {
> +        Ok(()) => {}
> +        Err(e) => {
> +            eprintln!("{prog_name:?}: {}", e);
> +            std::process::exit(1);
> +        }
> +    }
> +}
> +
> +fn main_(prog_name: &OsStr, mut args: std::env::ArgsOs) -> Result<(), String> {

main_ is a bit confusingly similar to name.  I like "run" for this sort
of function, and we use that elsewhere.

> +    match prog_name
> +        .as_bytes()
> +        .split(|&b| b == b'/')
> +        .next_back()
> +        .unwrap()

Another place that should use Path.

> +    {
> +        b"cgroup-s6-finish" => {
> +            return s6_finish(&mut args);
> +        }
> +        b"cgroup-setup" => {}

It is inconsistent for this not to also be its own function.  Then the
match could just be an expression that returned the result of the
appropriate function.

> +        b"cgroup-purge" => {
> +            if args.len() != 1 {
> +                return Err(format!(
> +                    "cgroup-purge takes one argument, got {}",
> +                    args.len()
> +                ));
> +            }
> +            let cgroup_path = args.next().unwrap();
> +            let (path, mut cgroup) = open_relative_cgroup(cgroup_path)?;
> +            let cgroup_target = Path::new(path.file_name().unwrap());
> +            return cgroup.purge(cgroup_target);
> +        }
> +        _ => {
> +            return Err(format!(
> +                "must be invoked as \"cgroup-setup\" \
> +                 \"cgroup-purge\", or \"cgroup-s6-finish\", \
> +                 got {prog_name:?}",
> +            ));
> +        }
> +    };
> +    let mut leaf = false;
> +    let mut cgroup_path;
> +    let mut delegate = false;
> +    let mut init_subtree = false;
> +    let mut child_name: Option<&'static OsStr> = None;
> +    let mut wait = true;
> +    loop {
> +        cgroup_path = args.next();
> +        let Some(ref arg_) = cgroup_path else {
> +            break;
> +        };
> +        let arg_ = arg_.as_bytes();
> +        if arg_ == b"--" {
> +            cgroup_path = args.next();
> +            break;
> +        }
> +        if !arg_.starts_with(b"-") {
> +            break;
> +        }
> +
> +        if !arg_.starts_with(b"--") {
> +            return Err("takes no short options".to_owned());
> +        }
> +
> +        match &arg_[2..] {
> +            b"leaf" => leaf = true,
> +            b"delegate" => delegate = true,
> +            b"init-subtree" => init_subtree = true,
> +            b"wait" => wait = true,
> +            b"no-wait" => wait = false,
> +            b"child-name" if child_name.is_none() => match args.next() {
> +                Some(arg) => child_name = Some(arg.leak()),
> +                None => return Err("--child-name: missing argument".to_owned()),
> +            },
> +            b"child-name" => return Err("--child-name: cannot be used twice".to_owned()),
> +            arg => match str::from_utf8(arg) {
> +                Ok(e) => return Err(format!("unknown long option {e:?}")),
> +                Err(_) => return Err("long option isn't UTF-8".to_owned()),
> +            },
> +        }
> +    }
> +
> +    let default_child_name = OsStr::from_bytes(b"$inner.service");
> +
> +    let child_name = Path::new(child_name.unwrap_or(default_child_name));
> +
> +    let Some(mut cgroup_path) = cgroup_path else {
> +        return Err("have no positional arguments, expected at least 1".to_owned());
> +    };
> +
> +    // Allow --init-subtree .
> +    if cgroup_path.as_bytes() == b"." && init_subtree && !leaf {
> +        cgroup_path = child_name.to_owned().into();
> +        leaf = true;
> +    }
> +
> +    let (path, mut cgroup) = open_relative_cgroup(cgroup_path)?;
> +    let cgroup_target = Path::new(path.file_name().unwrap());
> +    cgroup
> +        .make_child(cgroup_target)
> +        .map_err(|e| format!("Cannot make child cgroup: {e}"))?;
> +    if wait {
> +        cgroup
> +            .wait_for_empty()
> +            .map_err(|e| format!("Cannot wait for {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.
> +            cgroup
> +                .write_cgroup_value("cgroup.procs", &pid)
> +                .map_err(|e| format!("Cannot write to {path:?}/cgroup.procs: {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.
> +        cgroup.make_child(Path::new(child_name)).map_err(|e| {
> +            format!(
> +                "Cannot create child cgroup {}/{}: {e}",
> +                path.display(),
> +                child_name.display()
> +            )
> +        })?;
> +        if args.len() != 0 {
> +            cgroup
> +                .write_cgroup_value("cgroup.procs", &pid)
> +                .map_err(|e| {
> +                    format!(
> +                        "Cannot write to {}/{}/cgroup.procs: {e}",
> +                        path.display(),
> +                        child_name.display()
> +                    )
> +                })?;
> +        }
> +    }
> +    if !leaf {
> +        cgroup.enable_subtree_control(2)?;
> +    }
> +    if init_subtree {
> +        cgroup.enable_subtree_control(1)?;
> +    }
> +    if delegate {
> +        cgroup
> +            .enable_delegation(1)
> +            .map_err(|e| format!("Cannot enable cgroup delegation in {path:?}: {e}"))?;
> +    }
> +    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 s6_finish(args: &mut std::env::ArgsOs) -> Result<(), String> {
> +    if args.len() < 3 {
> +        return Err(format!(
> +            "s6 finish scripts take at least 3 arguments, got {}",
> +            args.len()
> +        ));
> +    }
> +    let status = parse_digit_string(&args.next().unwrap(), "exit status")?;
> +    let signal = args.next().unwrap();
> +    let signal = if status == 256 {
> +        Some(parse_digit_string(&signal, "signal number")?)
> +    } else {
> +        None
> +    };
> +    let service = args.next().unwrap();
> +
> +    let (path, mut cgroup) = open_relative_cgroup(service)?;
> +    let cgroup_target = Path::new(path.file_name().unwrap());
> +    let exit_125 = if let Some(signal) = signal {
> +        match signal as libc::c_int {
> +            libc::SIGBUS
> +            | libc::SIGFPE
> +            | libc::SIGABRT
> +            | libc::SIGTRAP
> +            | libc::SIGSEGV
> +            | libc::SIGILL => {
> +                // Process *crashed*, indicating a *possible exploit attempt*.
> +                // s6 should *not* restart it.  This is distinct from a Rust panic,
> +                // which is much less likely to indicate memory corruption.
> +                true
> +            }

This has absolutely nothing to do with cgroups.  If you want to have
some common finish behaviour, a program called cgroup-setup is not the
place for it.  I don't think there's any need for a separate
cgroup-s6-finish mode (as opposed to cgroup-purge).

> +            _ => false,
> +        }
> +    } else {
> +        false
> +    };
> +    if exit_125 {
> +        // Ignore panics.  Exit status is more important.
> +        // We already had a core dump.
> +        let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
> +            match cgroup.purge(cgroup_target) {
> +                Ok(()) => {}
> +                Err(e) => {
> +                    eprintln!("cgroup-s6-finish: Failed to purge cgroup: {e}")
> +                }
> +            };
> +        }));
> +        std::process::exit(125)
> +    } else {
> +        cgroup.purge(cgroup_target)
> +    }
> +}
> +
> +fn parse_digit_string(digits: &OsStr, msg: &str) -> Result<u16, String> {
> +    let checked = match str::from_utf8(digits.as_bytes()) {
> +        Ok(s) => s,
> +        Err(e) => return Err(format!("{msg} is not UTF-8: {e}")),
> +    };
> +    let r = checked
> +        .parse::<u16>()
> +        .map_err(|e| format!("{msg} {digits:?} is a bad 16-bit number: {e}"))?;
> +    match checked.as_bytes() {
> +        b"0" | [b'1'..=b'9', ..] => Ok(r),
> +        [b'0', ..] => Err(format!("{msg} {} has a leading 0", digits.display())),
> +        _ => Err(format!("{msg} {} starts with +", digits.display())),
> +    }

Surely we trust s6 to turn a number into a string.  These checks add nothing.

> +}
> +
> +fn local_cgroup() -> Result<PathBuf, String> {
> +    let mut local_cgroup: Vec<u8> = std::fs::read("/proc/thread-self/cgroup")
> +        .map_err(|e| format!("cannot read /proc/thread-self/cgroup: {e}"))?;
> +    let local_cgroup_len = local_cgroup.len();
> +    if local_cgroup_len < 5
> +        || local_cgroup[..4] != *b"0::/"
> +        || local_cgroup[local_cgroup_len - 1] != b'\n'
> +        || local_cgroup[4..local_cgroup_len - 1].contains(&b'\n')

Last time I suggested a clearer way of doing this, but it has instead
got even less clear.

(I'm not sure why we'd care if there's a newline specifically, as
opposed to any other control character.)

> +    {
> +        // It's possible to get here if the cgroup path contains a newline,
> +        // but that never happens in Spectrum.
> +        return Err(format!(
> +            "Invalid contents {local_cgroup:?} of /proc/thread-self/cgroup - \
> +             do you have cgroups v1 mounted instead of cgroups v2?"
> +        ));
> +    }
> +
> +    local_cgroup.copy_within(4..local_cgroup_len - 1, 0);
> +    local_cgroup.truncate(local_cgroup_len - 5);
> +    let local_cgroup = OsString::from_vec(local_cgroup);
> +    check_path(&local_cgroup).unwrap();

Why do we need to do this?  You're worried the kernel is going to start
including .. components in /proc/thread-self/cgroup?

> +    Ok(PathBuf::from(local_cgroup))
> +}
>
> -- 
> 2.55.0

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