On 7/27/26 08:10, Alyssa Ross wrote: > Demi Marie Obenour writes: > >> On 7/22/26 12:01, Alyssa Ross wrote: >>> Demi Marie Obenour 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, 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 { >>>> + 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 { >>>> + let mut local_cgroup: Vec = 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)