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

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 49659 bytes --]

On 7/22/26 12:01, Alyssa Ross wrote:
> 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.

Will fix in v5.

>> 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.

Okay, I'll remove this in v5.

>> +# 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:

Cgroup::enable_subtree_control() checks the exclusivity
of the caller-provided depth.  Line 232 of main.rs calls
enable_subtree_control(2).

These bools are only used in assertions, so they could be removed.
I will leave that up to you.  The advantage of keeping them is that a
panic is vastly easier to debug than a race condition due to improper
locking.

>     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…

Path isn't actually guaranteed to not have a NUL byte.
File::open(Path::new("\0")) fails with InvalidInput rather than
panicking.
I'm very used to writing this kind of code in C, so I went with a C-like
style instead of using Rust stdlib APIs.  I don't like having extra
abstractions in this kind of code, as it obscures what is going on
under the hood.  That is less important here, but it's very important
in programs like mount-flatpak.

For instance, RESOLVE_BENEATH or RESOLVE_IN_ROOT can result in
spurious EAGAIN errors, which libpathrs resolves using a retry
loop that has a failure rate of about 0.1% when the system is being
hammered by repeatedly calling rename().  This is needed for container
runtimes that need to follow symlinks in their target filesystems,
but mount-flatpak doesn't need to do that when traversing OSTree
repositories.  For mount-flatpak, an explicit check that the path
doesn't have "." or ".." components, and using RESOLVE_NO_SYMLINKS |
RESOLVE_NO_MAGICLINKS is just as secure and lacks this problem.

>> +    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.
Whoops, sorry about that.  Will fix in v5.

>> +        .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.

Yup!  Do you have a suggestion for improving it?  Maybe open_child()?

>> +            .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?

It probably should.

>> +        )
>> +    }
>> +
>> +    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 {

That results in a borrowcheck error.  The for loop takes ownership
of the iterator, but .peek() is called inside the loop.

>> +            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?

It's the path to the cgroup itself, relative to /sys/fs/cgroup.  Its only
purpose is for logging.

> 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.

I can do this, but it would result in much worse error messages: the
error would only reference the file name, not the full cgroup path.
Which would you prefer?

>> +                    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.

Nice catch!  Will change in v5.

>> +
>> +    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.

In that case, cgroup.events will include a "populated 0"
line, so poll will not be called.

>> +        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.

Correct.  Will rename in v5.

The way to understand this code is that Cgroup has two stacks: one
for file descriptors and one for path components.  All operations
operate at a specified depth from the top of the stack.  1 refers to
the top of the stack, 2 to one level below that, and so on.

This function is really confusing because it performs multiple pushes
and pops on the internal file descriptor stack.  The specific algorithm is:

 1. Start with an exclusive lock.

 2. Try to delete the child directly.

 3. If deletion succeeds, or if it fails with ENOENT, return success.

 4. If deletion fails with anything other than EBUSY, return an error.

 5. Open the child cgroup and take an exclusive lock on it.  This pushes
    the child cgroup's FD onto the stack.  The open_subtree() method
    also pushes the child path onto the stack.

 6. Take a *shared* lock on the FD that is directly below the top
    of the stack.  This is the file descriptor that was initially
    on the top of the stack.

    This releases the exclusive lock, allowing other operations on
    different children to proceed.  Different operations on the cgroup
    being purged will be blocked by the exclusive lock taken in step 5.

 7. Kill all programs in the cgroup by writing 1 to cgroup.kill.

 8. Open cgroup.events.

 9. Read from the FD opened in step 8.  If the file contains the line
    "populated 0", go to step 11.

10. Call poll() on the FD opened in step 8 to wait for POLLERR or
    POLLPRI to happen.  Then go back to step 9.

    This is race-free because the kernel will set the "this is ready"
    flag after every change that affects what would be read from
    the file.

11. Pop the file descriptor to the being-purged cgroup from the stack.

12. Use the just-popped file descriptor to remove all subdirectories
    recursively.  Files must not be deleted, as the kernel doesn't
    allow it.  Then close the file descriptor, releasing the exclusive
    lock held on it.
    
13. Take an exclusive lock on the *parent* of the cgroup that was just purged.

    This must be done after the file descriptor to the cgroup being
    purged has been closed.  Otherwise, there is the potential for
    an ABBA deadlock: another program might hold a shared lock on
    the parent, and be waiting to get an exclusive lock on the child.

14. Delete the being-purged cgroup.  Treat EBUSY and ENOENT as success:
    the first means that a concurrently-running program re-created the
    cgroup, while the second means that a concurrently-running program
    deleted it.  The name of the cgroup being purged is currently at
    the top of the path stack.

15. Pop the name of the cgroup being purged off of the stack.
    
At the end, self is in the same state it was before the operation.

If you are complaining that this is about as readable as Forth,
then I agree with you :).
   
>> +        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.

Will add in v5.

>> +            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.

We actually have an exclusive lock.  If it succeeds,
Cgroup::open_sub_cgroup() pushes a file descriptor onto self.fd.
Therefore, the fd being locked here is the one that was initially on the
top of the stack.  We assert that an exclusive lock is held on that FD.

Waiting for the control group to become empty is a blocking operation,
so this downgrades the lock to a shared one.  Otherwise, an in-progress
purge of /a/b would prevent /a/c from being created.

>> +        )
>> +        .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?

self.fd.pop() removes the FD from the stack, and remove_recursively closes it.
self.fd() returns the FD whose lock was downgraded to a shared one above.

>> +            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.

self.open_sub_cgroup() pushes both the path and the FD, but
self.fd.pop() only pops the FD.  This means that self.fd() is currently
the path to the *parent* of the cgroup.

>> +                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.)

It was pushed in self.open_sub_cgroup().

>> +        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.

Yes, it indeed should be.

>> +        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.

I tried, but that tries to delete files too, and that isn't allowed.

>> 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.

Will change in v5.  I'm way more used to doing this in C.

>> +        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.

It actually isn't.  OsStr::from_bytes("\0") doesn't panic.

>> +            _ 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.

It provides better error messages than ENAMETOOLONG, but I will remove it in v5.

>> +            _ => {}
>> +        }
>> +    }
>> +
>> +    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.)

All of them are indeed used.

>> +///
>> +/// # 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…

Fair!  Will fix in v5.

>> +    if cgroup_path.as_bytes().starts_with(b"/") {
> 
> … which would allow using Path::is_absolute if fixed.

Will fix in v5.

>> +        let mut cgroup_path = cgroup_path.into_vec();
>> +        cgroup_path.remove(0);
> 
> Please use Path functions rather than byte manipulation like this.

Will change in v5.

>> +        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.)

Will fix in v5.

>> +        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.

Will remove in v5.

>> +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.

Will change in v5.

>> +
>> +/// 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.

Will inline in v5.

>> +
>> +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.

Will fix in v5.

>> +    match prog_name
>> +        .as_bytes()
>> +        .split(|&b| b == b'/')
>> +        .next_back()
>> +        .unwrap()
> 
> Another place that should use Path.

Will fix in v5.

>> +    {
>> +        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.

Will fix in v5.

>> +        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).

This program is a multi-call binary, so the various things it can do
aren't necessarily super tightly related.  For instance, all of the
execline binaries can be built as one program, as can most if not
all busybox applets.  When invoked as cgroup-setup or cgroup-purge,
it indeed only does cgroup-related tasks.  cgroup-s6-finish not
only handles cgroups, but also other tasks related to being an s6
finish script.

That said, using this changes behavior in a way that isn't related
to cgroups, so if it is to be used at all it should be in a separate
patch series.  I'll remove this from v5.

>> +            _ => 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.

Yes, we can.  This is part of cgroup-s6-finish, which will be removed in v5.

>> +}
>> +
>> +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.)

If cgroups v1 is in use, the file can contain multiple lines, one for
each cgroup the program is in.  I also am not sure if starting with
"0::/" is an invariant in that case.

Using this program with cgroups v1 mounted is user error and will
never happen on Spectrum, but if this tool is used outside of Spectrum,
it could happen.

>> +    {
>> +        // 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?

Originally, I was going to create a wrapper around `Path` that
guaranteed no `.` or `..` components were present.  Its constructor
would have checked this invariant.  However, this turned out to be
more work due to the amount of wrapper functions required.

>> +    Ok(PathBuf::from(local_cgroup))
>> +}
>>
>> -- 
>> 2.55.0
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]