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

[Alyssa Ross <hi@alyssa.is>]

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

On Tue, Aug 04, 2026 at 09:36:00PM -0400, Demi Marie Obenour wrote:
> On 8/3/26 08:47, Alyssa Ross wrote:
> > Demi Marie Obenour <demiobenour@gmail.com> writes:
> >
> >> +    // The rustix source code shows that Dir::new() never fails.
> >> +    let child_fd = Rc::new(RefCell::new(Dir::new(fd).unwrap()));
> >> +    while let Some(element) = child_fd.borrow_mut().next() {
> >> +        let element = element.expect("Iterating through a cgroup directory failed?");
> >> +        if element.file_type() != rustix::fs::FileType::Directory {
> >> +            continue;
> >> +        }
> >> +        match element.file_name().to_bytes() {
> >> +            b"." | b".." => {}
> >> +            other => {
> >> +                let other = Path::new(OsStr::from_bytes(other)).to_owned();
> >> +                assert_single_component(&other);
> >> +                fds.push((child_fd.clone(), other));
> >
> > The data structures used here are still very confusing.  Why are we
> > storing a reference to the same file descriptor in every entry in the
> > Vec?
>
> Consider the recursive implementation (in pseudo-Rust):
>
> fn recursive_remove(fd) {
>     for entry in get_entries(&fd) {
>         if entry.is_dir_and_not_dot_or_dotdot() {
>             let directory = open_dir(&fd, &entry.path())?;
>             recursive_remove(directory)?;
>             remove_dir(&fd, entry.path())?;
>         }
>     }
> }
>
> The compiler knows that fd will stay open through recursive calls,
> so this doesn't need any unsafe code.  Using an explicit stack takes
> away this information from the compiler, so unsafe code is required.
> Using Rc<RefCell<Dir>> avoids the need for unsafe code at a cost
> in performance.

Hmm, but isn't it exactly child_fd that we're storing in the stack every
time?  Why store it in the stack at all rather than just using the
child_fd binding that exists for the whole life

> For what it is worth, the standard library implementation of
> fs::remove_dir_all() is recursive.  Standard library security hole?

Depends on their security model.  Doesn't seem ideal though, unless they
can use unstable features to do tail recursion or something, if that
would even be possible in this case.

> >> +            }
> >> +        }
> >> +    }
> >> +}
> >> +
> >> +// Remove all subdirectories of the given directory recursively,
> >> +// but not the directory itself.  The directory file descriptor
> >> +// is closed.
> >> +//
> >> +// This isn't the most efficient possible algorithm, but
> >> +// simplicity is more important than performance in this
> >> +// case.  Also, it keeps open more file descriptors than
> >> +// strictly necessary, but Spectrum runs with a very high
> >> +// limit for the number of open file descriptors, and it
> >> +// uses shallow control group hierarchies.
> >> +fn remove_child_directories(dirfd: OwnedFd) -> Result<(), Errno> {
> >> +    let mut fds = Vec::new();
> >> +    // Push the children of this directory onto the stack.
> >> +    push_child_fds(&mut fds, dirfd);
> >> +    while let Some((d, path)) = fds.pop() {
> >
> > Couldn't we call push_child_fds() once here, rather than twice as is
> > currently done?  (And then consider inlining it, depending on how
> > complex it's looking at the time.)
>
> Can you provide an example?  I don't see how to make this change
> while preserving semantics.  Only directories meant for deletion
> can appear on the stack, and the root of the traversal must not
> be deleted (yet).

Again I think I probably misunderstood, sorry.

> >> +            let sub_fd = cgroup
> >> +                .open_beneath(Path::new(component), OFlags::RDONLY | OFlags::DIRECTORY)
> >> +                .map_err(|e| format!("Cannot open sub-cgroup {component:?}: {e}"))?;
> >> +            // Take a shared lock on the *previous* file descriptor.
> >> +            rustix::fs::flock(&cgroup, FlockOperation::LockShared)
> >> +                .map_err(|e| format!("Cannot lock sub-cgroup {component:?}: {e}"))?;
> >> +            cgroup.fd.push(sub_fd);
> >> +        }
> >> +        // Take an exclusive lock on the final file descriptor.
> >> +        rustix::fs::flock(&cgroup, FlockOperation::LockExclusive)
> >> +            .map_err(|e| format!("Cannot lock {path:?}: {e}"))?;
> >> +        Ok(cgroup)
> >> +    }
> >> +
> >> +    pub fn wait_for_empty(fd: &dyn AsFd) -> std::io::Result<()> {
> >> +        let wait_file = openat2_simple(fd, Path::new("cgroup.events"), OFlags::RDONLY)?;
> >> +        let poll_fd = wait_file.as_raw_fd();
> >> +        let mut wait_fd = File::from(wait_file);
> >> +        let mut fds = libc::pollfd {
> >> +            fd: poll_fd,
> >
> > I would inline poll_fd here.  RawFd is easy to misuse, so I like to
> > avoid having them hang around.
>
> I will move `fds` into the inner loop.

I'd still like to see fd: wait_file.as_raw_fd() as well.

> >> +    pub fn purge_child(&mut self, path: &Path) -> Result<(), String> {
> >> +        assert_single_component(path);
> >> +        // See if we can just delete the child directly.
> >> +        match rustix::fs::unlinkat(&self, Path::new(path), AtFlags::REMOVEDIR) {
> >> +            // If the cgroup was successfully deleted, or if it
> >> +            // has already been deleted, we are done.
> >> +            Ok(()) | Err(Errno::NOENT) => return Ok(()),
> >> +            // If this cgroup is in use, keep going.
> >> +            Err(Errno::BUSY) => {}
> >> +            Err(e) => return Err(format!("Cannot purge {path:?}: {e}")),
> >> +        }
> >> +
> >> +        let sub_fd = match self.open_beneath(path, OFlags::RDONLY | OFlags::DIRECTORY) {
> >> +            Ok(sub_fd) => sub_fd,
> >> +            Err(Errno::NOENT) => return Ok(()),
> >> +            Err(e) => {
> >> +                return Err(format!("Cannot open sub-cgroup {path:?}: {e}",));
> >> +            }
> >> +        };
> >> +
> >> +        // Take an exclusive lock on the cgroup that is about to be
> >> +        // removed.  This avoids concurrent executions of this program
> >> +        // operating on deleted sub-cgroups.
> >> +        rustix::fs::flock(&sub_fd, FlockOperation::LockExclusive)
> >> +            .map_err(|e| format!("Cannot lock sub-cgroup: {e}"))?;
> >> +
> >> +        // Drop the exclusive lock on the original cgroup,
> >> +        // This avoids blocking concurrent operations on other
> >> +        // child cgroups while the cgroup is being purged,
> >> +        // or while waiting for programs to exit.
> >> +        rustix::fs::flock(&self, FlockOperation::LockShared)
> >> +            .map_err(|e| format!("Cannot relock: {e}"))?;
> >
> > Could you add some extra explanation here of why it's okay for the
> > exclusive lock to be temporarily dropped here?
> >
> > I'm wondering whether taking a lock, then dropping it temporarily is a
> > sign that we're taking the lock too early in the first place, and should
> > scope it better to where it's actually needed.
>
> Indeed so.  Programs that are modifying a cgroup need an exclusive
> lock on it.  Adding or removing to the cgroup does *not* count as
> modification: both operations are idempotent, removing an in-use
> cgroup fails with -EBUSY, and operating on a deleted cgroup fails
> with -ENODEV or -ENOENT depending on what one is doing.  Operations on
> control files *do* require an exclusive lock.

Good, let's have that written down somehow.  Preferably it'd be encoded
in the type system but I don't know if that's easily achievable.

> >> +
> >> +fn enable_subtree_control(fd: &dyn AsFd) -> Result<(), String> {
> >
> > Would it not make sense for this to be an instance method on Cgroup,
> > since it's a Cgroup-specific operation?
>
> We don't create a Cgroup struct for the child cgroup
> FD on which this function is called.

But we could!

> >> +    let mut subtree = vec![];
> >> +    for controller in buf.split(|&b| b == b' ').filter(|e| !e.is_empty()) {
> >
> > Are there ever likely to be empty works in this file?
>
> No, there will not be unless there is a kernel bug.

Right, so then we don't need the filter?

> > So looking at this I still see several different modes and am wondering
> > whether we could simplify this further.
> >
> >  • Why do we need a separate leaf mode?  Why not just still use a
> >    $inner.service in that case?
>
> $inner.service is just wasteful and makes it harder to inspect the cgroup
> tree by hand.

It can't be that wasteful, can it?  Surely cgroups are designed to
scale.  I'd rather have the consistency.

> >  • What would the consequences be if we took the systemd_compat branch
> >    for a non-cgroup-aware Spectrum program?
>
> Non-cgroup-aware programs would be fine, but nested calls to cgroup-setup
> would break because they need the enable_subtree_control() call.  However,
> in the future, I would like to check the user.delegate xattr to determine
> if one can safely write to the control files of the cgroup or if the cgroup
> is owned by another program.

And it wouldn't be correct to write to cgroup.subtree_control in all
cases?  Programs that expect cgroup delegation expect it to start empty,
and so won't disable controllers that they need to be disabled?  Or
would that be fine?

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