From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from atuin.qyliss.net (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id EAA898E61; Wed, 05 Aug 2026 16:39:54 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id E7EFD8DCC; Wed, 05 Aug 2026 16:39:52 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DMARC_MISSING,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H2, SPF_HELO_PASS autolearn=unavailable autolearn_force=no version=4.0.1 Received: from fout-a8-smtp.messagingengine.com (fout-a8-smtp.messagingengine.com [103.168.172.151]) by atuin.qyliss.net (Postfix) with ESMTPS id EADC18DC8 for ; Wed, 05 Aug 2026 16:39:50 +0000 (UTC) Received: from phl-compute-03.internal (phl-compute-03.internal [10.202.2.43]) by mailfout.phl.internal (Postfix) with ESMTP id 2CE96EC02CC; Wed, 5 Aug 2026 12:39:50 -0400 (EDT) Received: from phl-frontend-03 ([10.202.2.162]) by phl-compute-03.internal (MEProxy); Wed, 05 Aug 2026 12:39:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alyssa.is; h=cc :cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm2; t=1785947990; x=1786034390; bh=I7i6Njhtrw Tr8AsbLbouFMC8nSwcCsZ6z8OZQAYu2hw=; b=Bzq5k3/FOyRJPfqBqbzmW4BTHa zGTkvn8iX6SCJwOQE2hChjkNF5tMCINsaLqxixl81P+ReOcxJXYsHDg6TF0BV4kj 3Y6+OnMvsK16/m2AYUP3heQaHXsYWhmKlT/Gz563b45wwxDuJErv3egR0aXtenVG xRplNkHn/0BdOxK+dBs43K5niyMLhQ94yghsI7Gkc7Xn879WWjtq5Bm3+aZrbKWw nwXv3VS94HbSnI7eiHobMf28jzVQhS4d73Pv4OJXd4v9CREvdLCBNjrNjFd4AFzu X+h3UsLA1WJ5VtIs9nrkmxFHvyz3Pc7830zX75q3q+TzB2EjnQOKbL+3rRHg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t= 1785947990; x=1786034390; bh=I7i6NjhtrwTr8AsbLbouFMC8nSwcCsZ6z8O ZQAYu2hw=; b=TpZWf1RVwz56UC7zk/5HPli1qa+0NDCjHxBF4ZClg6luBRCzl3M u4U9wKlsUcK+Yy7dKEeKFO/kY7GoK4MZTxvLoUhFlt5IO1NucWY3Mq3cLDZAamBl SJ4IEnQIprMktVCHQAKo74x1q7dIXbleSUOIlEZJLcocHYb854ZmZt9Gt0H+UaXX S/oZGdclXTl+R4kUpF83XdLDYrRw9+vTUWmFENOG9W/gSiomd6BvgEvKrAST3pRu JZ6O9GM77D3/J2apfjkohFKIpUd+85jVO51ntCbfHHTpRaIUvdbE6yMmU13c6SoT kaOOgPq0/qZkjXueOJJd5LJEYiUHnu1xS1w== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: dmFkZTEtHjMraByvWfh4AwjsYsrhDrC3hd2SRPF9HT8I3sfGqiYcofiv5hWHsA/Os+tr1w pjrvnIYw9vuHAziAkTYVrAxrDYB8IYvHnE48A2YxkAggUOYIQIIEEyfWmcpz7Aeq9UiHWY OTYwFP1i76CN+FKE3lflD4oOGsIWg1jZ67hKhbcmrZClSYYz7UOwUc52ah1PWzeIdrUHsx 8ABkExGjz6IQw8yPj0rGpikNwZHW+lJsP400zjv7qNCb9vUH8Uggy6DZvCsFhsiVpRiQJf JasYIghfbDO7diZkJvEuRQ6NwtsOKf0bJztN6QzM85BnLkBIpNYQctamU6ofWVldz861RP d//yha3FeB3flNJXFPGP8IkSDsLAljgtrrhuHBRKgKctIBE4IalRENg3OEPiGSvoYATeZN ajtPjfdTw5zr6ZpvmK90ISBWChTPoHLxG7Y30/WAWG64KJxzxSKuaNMLGe7R8tu3FWulRv TUsqjc5Wm9bmbC7yVMtSrwek6A7Yv9wXeo73lW1wvsR+bLKNMiEfSGQOeHzeMK2rw98BWn C4cyI6Nrkjem3EbliCp/BP0UHYRd/06ikubGvBxR9ZL+wbMeWL0flH40OlYv/wh81C6x89 kKE/2sytiD/nks6LKXIQ6ogCo8LDnqS1azZFoJHFkgE/f41ocvBF901zeWxw X-ME-Proxy: Feedback-ID: i12284293:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 5 Aug 2026 12:39:49 -0400 (EDT) Received: by fw12.qyliss.net (Postfix, from userid 1000) id 5BCA0C7809A6; Wed, 05 Aug 2026 18:39:48 +0200 (CEST) Date: Wed, 5 Aug 2026 18:39:48 +0200 From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH v5 02/19] tools: Add control group manager Message-ID: References: <20260731-cgroups-v5-0-b325bac9d34f@gmail.com> <20260731-cgroups-v5-2-b325bac9d34f@gmail.com> <2ce7b61d-faad-49b1-9f15-019140e2dca1@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="5z5xvszvdasm6vgy" Content-Disposition: inline In-Reply-To: <2ce7b61d-faad-49b1-9f15-019140e2dca1@gmail.com> Message-ID-Hash: JWAU23NWYZZ47N5GE7RMDZYKGS2PNSGC X-Message-ID-Hash: JWAU23NWYZZ47N5GE7RMDZYKGS2PNSGC X-MailFrom: hi@alyssa.is X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; header-match-devel.spectrum-os.org-0; header-match-devel.spectrum-os.org-1; header-match-devel.spectrum-os.org-2; header-match-devel.spectrum-os.org-3; header-match-devel.spectrum-os.org-4; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Spectrum OS Development X-Mailman-Version: 3.3.10 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --5z5xvszvdasm6vgy Content-Type: text/plain; protected-headers=v1; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [PATCH v5 02/19] tools: Add control group manager MIME-Version: 1.0 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 writes: > > > >> + // The rustix source code shows that Dir::new() never fails. > >> + let child_fd =3D Rc::new(RefCell::new(Dir::new(fd).unwrap())); > >> + while let Some(element) =3D child_fd.borrow_mut().next() { > >> + let element =3D element.expect("Iterating through a cgroup di= rectory failed?"); > >> + if element.file_type() !=3D rustix::fs::FileType::Directory { > >> + continue; > >> + } > >> + match element.file_name().to_bytes() { > >> + b"." | b".." =3D> {} > >> + other =3D> { > >> + let other =3D 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 =3D 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> 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 =3D Vec::new(); > >> + // Push the children of this directory onto the stack. > >> + push_child_fds(&mut fds, dirfd); > >> + while let Some((d, path)) =3D 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 =3D cgroup > >> + .open_beneath(Path::new(component), OFlags::RDONLY | = OFlags::DIRECTORY) > >> + .map_err(|e| format!("Cannot open sub-cgroup {compone= nt:?}: {e}"))?; > >> + // Take a shared lock on the *previous* file descriptor. > >> + rustix::fs::flock(&cgroup, FlockOperation::LockShared) > >> + .map_err(|e| format!("Cannot lock sub-cgroup {compone= nt:?}: {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 =3D openat2_simple(fd, Path::new("cgroup.events= "), OFlags::RDONLY)?; > >> + let poll_fd =3D wait_file.as_raw_fd(); > >> + let mut wait_fd =3D File::from(wait_file); > >> + let mut fds =3D 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::R= EMOVEDIR) { > >> + // If the cgroup was successfully deleted, or if it > >> + // has already been deleted, we are done. > >> + Ok(()) | Err(Errno::NOENT) =3D> return Ok(()), > >> + // If this cgroup is in use, keep going. > >> + Err(Errno::BUSY) =3D> {} > >> + Err(e) =3D> return Err(format!("Cannot purge {path:?}: {e= }")), > >> + } > >> + > >> + let sub_fd =3D match self.open_beneath(path, OFlags::RDONLY |= OFlags::DIRECTORY) { > >> + Ok(sub_fd) =3D> sub_fd, > >> + Err(Errno::NOENT) =3D> return Ok(()), > >> + Err(e) =3D> { > >> + 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 =3D vec![]; > >> + for controller in buf.split(|&b| b =3D=3D 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. > > > > =E2=80=A2 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. > > =E2=80=A2 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 cgro= up > 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? --5z5xvszvdasm6vgy Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQQGoGac7QfI+H5ZtFCZddwkt31pFQUCanNnUwAKCRCZddwkt31p FfJQAQCkWATwjEbhVO8/z4eI0NH4TG2k2cFBUnVpXiMlcwIL9QEAtdGpWaM/V2hB pPe4zaFWExZH3TQNyZsAcrFigGvDmA8= =0QOq -----END PGP SIGNATURE----- --5z5xvszvdasm6vgy--