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

[Alyssa Ross <hi@alyssa.is>]

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

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

> On 7/22/26 12:01, Alyssa Ross wrote:
>> Demi Marie Obenour <demiobenour@gmail.com> writes:

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

Can you explain to me why we need to support enabling subtree control
for different depths?  Intuitively, I'd expect this program to only ever
operate on the cgroup associated with the service that's invoking it.

>>     fd: Vec<OwnedFd>,
>>     exclusive: bool,
>> 
>>> +}

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

Oh, my mistake.  You are right.

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

I did say myself that my suggestion here might be too clever, because it
uses the standard library in a way that's unintuitive.  In general,
though, we can expect readers of Rust code to be more familiar with the
standard library than with byte-by-byte stringy patch processing.  Would
still be nicer to take &Path here, I think, even if you then walk
through it byte by byte, just to make it slightly clearer what this does.

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

open_beneath?

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

Ah, okay.  This is fine then.

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

I would much prefer code I can easily understand.  There's always strace
for getting the full paths when debugging.

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

You are correct. :)

Perhaps could be written more clearly with a while, something like this:

    while !v.split(|&c| c == b'\n').any(|line| line == b"populated 0") {
        // SAFETY: FFI call, valid arguments, fds contains 1 element
        if unsafe { libc::poll(&raw mut fds, 1, -1) } != 1 {
            panic!("poll failed");
        }
        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");
    }

But if you prefer the current way (which avoids a redundant initial
check of an empty Vec, although that should be cheap) that's also fine.

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

1-indexing is a little unintuitive for Rust, no?

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

Could this be made clearer by not mutating self, and either just storing
the child stuff in local variables or another Cgroup object?  I think a
big part of the confusion here is that this function temporarily changes
which cgroup the Cgroup object it's called on refers to while it's
running.  That's extremely difficult to reason about.

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

Ah, didn't realise it would downgrade.  Makes sense.

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

I'd inline these variables into the function call the extent possible.
Otherwise I have to follow a lot of shuffling around.

>>> +    }
>>> +    drop(d);
>>> +    Ok(())
>>> +}
>>> +
>>> +fn remove_all(
>>> +    remaining_depth: usize,
>>> +    dirfd: BorrowedFd<'_>,
>>> +    path: &std::ffi::CStr,
>>> +) -> Result<(), Errno> {
>>> +    if path == c"." || path == c".." {
>>> +        return Ok(());
>>> +    }

It's a bit weird that calling remove_all on . or .. does not fail.
Maybe would be clearer to move this check to the call site?

>>> +    if rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR).is_ok() {
>>> +        return Ok(());
>>> +    }

We could drop this, right?  A few extra syscalls, but less to wrap my
head around.

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

Ah, alright.  That could use an explanatory comment.

How can we be confident we have enough stack for this?  Is there a way
it could be done non-recursively, with state on the heap?

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

Thank you.  If we want to have a big multi-call binary that does lots of
different things, only some of which are cgroup-specific, cgroup-setup
is not the name for that program.

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

I see.  It seems like with cgroups v1, it _could_ start with 0::/, but
probably wouldn't.  I think it may not be possible to tell from this
file whether cgroups v1 is in use.

So I suppose it depends what you want to happen if cgroups v1 is in use.
If it looks enough like cgroups v2, do you continue, or do you
explicitly check for cgroups v1?  If the latter (sounds more sensible to
me), you need to explicitly check for cgroups v1 somehow I think.  Can
cgroups v1 and v2 be in use at the same time?  If so, checking might be
complicated, but if not, you can just check what type of filesystem is
mounted at /sys/fs/cgroup, or see if it has a
/sys/fs/cgroup/cgroup.controllers file.

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

Wise not to proceed with that, I think.  But I don't think we need this
particular instance of the check, given it comes from the kernel.

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