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

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

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

On 7/27/26 08:10, Alyssa Ross wrote:
> 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.

Init runs "cgroup-setup --init-subtree .", which moves PID 1 to
/$inner.service and enables all controllers in /.  I'll just use sed
to enable the controllers, which avoids special cases around paths
with no file name.

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

Will fix in v5.

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

Will use in v5.

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

Need is obviated in v5 by a refactor.

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

This code is dropped in v5.

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

That version would call poll() before checking that the cgroup is empty,
and that *can* deadlock :).

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

That makes sense.  I'll try to move stuff into helper objects.  You're
correct that the 

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

Will fix in v5.

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

Will fix in v5, making the code simpler.

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

Correct.

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

In Spectrum, the depth is currently limited to a small constant.

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

No argument there!

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

In the case of Spectrum, I think it's okay to just panic if cgroups v2
isn't mounted or isn't working properly.  It's a bug in either
cgroup-setup or the kernel, almost certainly the former.

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

Dropped in v5.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)



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