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 694007040; Wed, 22 Jul 2026 16:01:26 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id 8878D7016; Wed, 22 Jul 2026 16:01:23 +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,SPF_HELO_PASS autolearn=unavailable autolearn_force=no version=4.0.1 Received: from fhigh-b7-smtp.messagingengine.com (fhigh-b7-smtp.messagingengine.com [202.12.124.158]) by atuin.qyliss.net (Postfix) with ESMTPS id E55957014 for ; Wed, 22 Jul 2026 16:01:20 +0000 (UTC) Received: from phl-compute-02.internal (phl-compute-02.internal [10.202.2.42]) by mailfhigh.stl.internal (Postfix) with ESMTP id 4F71B7A0016; Wed, 22 Jul 2026 12:01:18 -0400 (EDT) Received: from phl-frontend-04 ([10.202.2.163]) by phl-compute-02.internal (MEProxy); Wed, 22 Jul 2026 12:01:18 -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=fm1; t=1784736078; x=1784822478; bh=hjNYqivybb nRPLIw7KKYBHN186PGe85Vexq/C29vpr0=; b=QgCwJry9RchiF/F/eiNQYjXrUq 7M1b4kEIA5IxZrIajPhdiyBwtGtLhljBgX6zEYMRLHAKcnyn201uUNReVfIz4YdD L3EGrONRNx8sdZWasfj743YUzrYu3dArc7BCU+KGz1EfRENHb7I7hIqUtKY+/WAH TeEKQRJFXvY3TlaSjX47eMQ+GMDFDuiG4tC5rz3MG1N27NilJEoKZ2yWSH3JjkBB OVUOOrslDB6D7iNnXnIIoDnLlJaUyh32Vgcahim4CNfuBqdJMS8TuXfGnfXaqD4X +CVTdsESRTjmf+uPKCp6HpTRiGxFj1ywSE2dz2HAqWlWUJB1kRCCEL2VNvkQ== 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=fm2; t= 1784736078; x=1784822478; bh=hjNYqivybbnRPLIw7KKYBHN186PGe85Vexq /C29vpr0=; b=gLnDKsvV7+/G3Pk3itkvKpW20aaSXYnSRzDTt7CXpHixyK5CEau KKuOhc2Zrz2EtG0XmfRsiiNTyNL8k5OiT20vrLiwf7pK52KU0nUobBxOcPoQqQ41 XKPNKeEkchyh9cTneNfq9tEkI7QH6+VzHbWCl8CHmsq7ej+eR32sNheA7DBOTruL T0WnJ+/zJjmO6zkZNF0R65iYCBT6W/WTK+y4BGhtMyOOZnJkksRLeHJmiiINVexI CYT7dNwyn+MRuY9QL2HD7qEtIUdV/acWaPLBXHIcU+xmCtaGdiahV28t9UV9hUwu spBIGX3+i5MNSy515Ehcji1D5i6tfCP+SBw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: dmFkZTENsD3G9qLl1nIjsJXX+N8v4bVRmftFnHRSY+Gua2vWNMrop1jHg8CAu6vtfjJhsL IUDOea7pSUr+sBpdG8qRdxoA2QLKoHygQ7K4tzx+JPJ6DYIzrNq3xh/npy78QSKn88mNLU KGXG9SZghvg/qLXjL6ifJvFftaj7qewjfkgscE4uGoWASFxX73Rxn+l7fh0Y0Fbgt4ZEt+ JMpdPvN/O6pY8NMlCVMxg5IbbkoSzvPkYe8DT4gaDRtLX5AMuvV9mo2VJnjuCxBdS/HVXz JVczM1A9Zx/3EhKosAZIoBkN3FnwC6Dn35l7OErchRYCOyoDzSJooKfx4rgTOzxofbyrGT zWhkjtS+6n/gQCqWxKu3pXrZvh1+ToAuOZbPlTl687K2SM48lapR/L/a/nUoUm7NxI8vEc kFYUM7pB5ZqbmhtY3WLKwKZ0cgZVivHO9TW39UHTJbpbBcw5pBvc8ykT6G08m65Y+y6sMs YhFGtMZcPBZFKbQ+myUIlz6pydTAKbSiVN2H/52PZLB9yQySYJ/z6JJc5TWd6udXhHTyU7 Mox9mzXvsaES1B5G81nNmXVcWXJJ1TgqD4zKyA+zeRNj+rU2UOQKB1MBKzh839bmr8OxmQ uvSqLi94KACQNv8mFaUzeu8Dwu1AYLoIaZkro2Jf6599lLWJtNOuZxuGN/LQ X-ME-Proxy: Feedback-ID: i12284293:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 22 Jul 2026 12:01:17 -0400 (EDT) Received: by mbp.qyliss.net (Postfix, from userid 1000) id 5B59C896A8BC; Wed, 22 Jul 2026 18:01:15 +0200 (CEST) From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH v4 02/20] tools: Add control group manager In-Reply-To: <20260721-cgroups-v4-2-46b2e5fff7b6@gmail.com> References: <20260721-cgroups-v4-0-46b2e5fff7b6@gmail.com> <20260721-cgroups-v4-2-46b2e5fff7b6@gmail.com> Date: Wed, 22 Jul 2026 18:01:14 +0200 Message-ID: <87a4rjrp2t.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Message-ID-Hash: WWNERALEX46L6RSJ57IGM56FHMHJIYGB X-Message-ID-Hash: WWNERALEX46L6RSJ57IGM56FHMHJIYGB 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: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Demi Marie Obenour writes: > The cgroup-setup Rust program can create and purge cgroups. It can also > wait for one to become empty, spawn a program in a cgroup, and more. In > the future, it will also support cgroup-based resource control. Locking > is used to ensure that concurrent invocations are safe. > > This program can also be used in an s6 finish script. When passed the > args of such a script, it automatically purges the correct cgroup. It > also tells s6 to not restart the service if it dumped core. Core dumps > are often due to memory corruption, and automatically restarting a > service that dumped core makes memory corruption attacks easier. > > Signed-off-by: Demi Marie Obenour > --- > .codespellrc | 2 +- > host/rootfs/default.nix | 6 +- > host/rootfs/file-list.mk | 2 + > host/rootfs/image/usr/bin/cgroup-purge | 1 + > host/rootfs/image/usr/bin/cgroup-s6-finish | 1 + > pkgs/default.nix | 1 + > tools/cgroup-setup/Cargo.lock | 67 ++++++ > tools/cgroup-setup/Cargo.lock.license | 2 + > tools/cgroup-setup/Cargo.toml | 11 + > tools/cgroup-setup/default.nix | 18 ++ > tools/cgroup-setup/src/cgroup.rs | 349 +++++++++++++++++++++++= ++++++ > tools/cgroup-setup/src/main.rs | 347 +++++++++++++++++++++++= +++++ > 12 files changed, 803 insertions(+), 4 deletions(-) > diff --git a/host/rootfs/file-list.mk b/host/rootfs/file-list.mk > index 3899d620717fc97f42e669e5313c4100dcf5b1cd..e1280ab56d8797e40b9b1c584= ab0daef3cda41d7 100644 > --- a/host/rootfs/file-list.mk > +++ b/host/rootfs/file-list.mk > @@ -79,6 +79,8 @@ LINKS =3D \ > image/etc/s6-linux-init/run-image/service/vmm/template/run \ > image/lib \ > image/sbin \ > + image/usr/bin/cgroup-purge \ > + image/usr/bin/cgroup-s6-finish \ > image/usr/bin/systemd-udevd >=20=20 > S6_RC_FILES =3D \ > diff --git a/host/rootfs/image/usr/bin/cgroup-purge b/host/rootfs/image/u= sr/bin/cgroup-purge > new file mode 120000 > index 0000000000000000000000000000000000000000..a0c8d8e144d72b69c613eb061= 3e39acc9df979df > --- /dev/null > +++ b/host/rootfs/image/usr/bin/cgroup-purge > @@ -0,0 +1 @@ > +cgroup-setup > \ No newline at end of file > diff --git a/host/rootfs/image/usr/bin/cgroup-s6-finish b/host/rootfs/ima= ge/usr/bin/cgroup-s6-finish > new file mode 120000 > index 0000000000000000000000000000000000000000..a0c8d8e144d72b69c613eb061= 3e39acc9df979df > --- /dev/null > +++ b/host/rootfs/image/usr/bin/cgroup-s6-finish > @@ -0,0 +1 @@ > +cgroup-setup > \ No newline at end of file Usually packages that expect to be invoked via symlinks like this (coreutils, busybox, execline) install their own symlinks, rather than expecting systems to create them. I think these would be more appropriate in a postInstall in tools/cgroup-setup/default.nix. > diff --git a/pkgs/default.nix b/pkgs/default.nix > index 44f7b5ff78cb6b9e755292a6a417d0b627ed3fb0..0a13393164ad5d7f752e63076= 3f3f97166479af5 100644 > --- a/pkgs/default.nix > +++ b/pkgs/default.nix > @@ -51,6 +51,7 @@ let > driverSupport =3D true; > }; > spectrum-router =3D self.callSpectrumPackage ../tools/router {}; > + spectrum-cgroup-setup =3D self.callSpectrumPackage ../tools/cgroup-s= etup {}; > xdg-desktop-portal-spectrum-host =3D > self.callSpectrumPackage ../tools/xdg-desktop-portal-spectrum-host= {}; >=20=20 > diff --git a/tools/cgroup-setup/Cargo.lock b/tools/cgroup-setup/Cargo.lock > new file mode 100644 > index 0000000000000000000000000000000000000000..fe967b3aa02c296c87b6b36ac= 59253dbe0a32de9 > --- /dev/null > +++ b/tools/cgroup-setup/Cargo.lock > @@ -0,0 +1,67 @@ > +# This file is automatically @generated by Cargo. > +# It is not intended for manual editing. > +version =3D 4 > + > +[[package]] > +name =3D "bitflags" > +version =3D "2.11.1" > +source =3D "registry+https://github.com/rust-lang/crates.io-index" > +checksum =3D "c4512299f36f043ab09a583e57bceb5a5aab7a73db1805848e8fef3c9e= 8c78b3" > + > +[[package]] > +name =3D "cgroup-setup" > +version =3D "0.0.0" > +dependencies =3D [ > + "libc", > + "rustix", > +] > + > +[[package]] > +name =3D "errno" > +version =3D "0.3.14" > +source =3D "registry+https://github.com/rust-lang/crates.io-index" > +checksum =3D "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b= 63efeb" > +dependencies =3D [ > + "libc", > + "windows-sys", > +] > + > +[[package]] > +name =3D "libc" > +version =3D "0.2.186" > +source =3D "registry+https://github.com/rust-lang/crates.io-index" > +checksum =3D "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61ece5= 87de66" > + > +[[package]] > +name =3D "linux-raw-sys" > +version =3D "0.12.1" > +source =3D "registry+https://github.com/rust-lang/crates.io-index" > +checksum =3D "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25= d58a53" > + > +[[package]] > +name =3D "rustix" > +version =3D "1.1.4" > +source =3D "registry+https://github.com/rust-lang/crates.io-index" > +checksum =3D "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e993= 0ba190" > +dependencies =3D [ > + "bitflags", > + "errno", > + "libc", > + "linux-raw-sys", > + "windows-sys", > +] > + > +[[package]] > +name =3D "windows-link" > +version =3D "0.2.1" > +source =3D "registry+https://github.com/rust-lang/crates.io-index" > +checksum =3D "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d= 0a4fe5" > + > +[[package]] > +name =3D "windows-sys" > +version =3D "0.61.2" > +source =3D "registry+https://github.com/rust-lang/crates.io-index" > +checksum =3D "ae137229bcbd6cdf0f7b80a31df61766145077ddf49416a728b02cb392= 1ff3fc" > +dependencies =3D [ > + "windows-link", > +] > diff --git a/tools/cgroup-setup/Cargo.lock.license b/tools/cgroup-setup/C= argo.lock.license > new file mode 100644 > index 0000000000000000000000000000000000000000..aa108acd23886d8302eaf7bab= ff90d1b08ae19fb > --- /dev/null > +++ b/tools/cgroup-setup/Cargo.lock.license > @@ -0,0 +1,2 @@ > +SPDX-License-Identifier: EUPL-1.2+ > +SPDX-FileCopyrightText: 2026 Demi Marie Obenour I think this should be CC0-1.0 like every other Cargo.lock.license. There's nothing copyrightable about it. > diff --git a/tools/cgroup-setup/Cargo.toml b/tools/cgroup-setup/Cargo.toml > new file mode 100644 > index 0000000000000000000000000000000000000000..7ed6d6a0ea3bbfc4064b9f393= 83d0788c4bd84e5 > --- /dev/null > +++ b/tools/cgroup-setup/Cargo.toml > @@ -0,0 +1,11 @@ > +# SPDX-License-Identifier: CC0-1.0 > +# SPDX-FileCopyrightText: 2025 Alyssa Ross I surely did not contribute anything copyrightable to this. > +# SPDX-FileCopyrightText: 2026 Demi Marie Obenour > + > +[package] > +name =3D "cgroup-setup" > +edition =3D "2024" > + > +[dependencies] > +libc =3D "0.2.177" > +rustix =3D { version =3D "1.1.2", features =3D ["fs"] } > diff --git a/tools/cgroup-setup/src/cgroup.rs b/tools/cgroup-setup/src/cg= roup.rs > new file mode 100644 > index 0000000000000000000000000000000000000000..c953d26badfdac0a1e3d7057a= 867aec3b3247e18 > --- /dev/null > +++ b/tools/cgroup-setup/src/cgroup.rs > @@ -0,0 +1,349 @@ > +// SPDX-License-Identifier: EUPL-1.2+ > +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour > + > +use std::ffi::OsStr; > +use std::fmt::Display; > +use std::fs::File; > +use std::io::{Read as _, Seek as _, Write as _}; > +use std::os::unix::prelude::*; > + > +use std::path::{Component, Path, PathBuf}; > + > +use rustix::fs::{AtFlags, FlockOperation, XattrFlags}; > +use rustix::{ > + fs::{Mode, OFlags, ResolveFlags}, > + io::Errno, > +}; > + > +#[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: fd: Vec, exclusive: bool, > +} > + > +impl AsFd for Cgroup { > + fn as_fd(&self) -> BorrowedFd<'_> { > + self.fd.last().unwrap().0.as_fd() > + } > +} > + > +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() =3D=3D component.file_name()? Maybe that's too clever, though=E2=80=A6 > + match component { > + b"" | b"." | b".." =3D> panic!("bad component"), > + _ if component.contains(&b'\0') =3D> panic!("NUL in component"), > + _ if component.contains(&b'/') =3D> panic!("/ in component"), > + _ =3D> {} > + } > +} > + > +impl Cgroup { > + pub fn new(exclusive: bool) -> Result { > + let cgroup_root =3D rustix::fs::openat2( > + rustix::fs::CWD, > + Path::new("/sys/fs/cgroup"), > + OFlags::DIRECTORY | OFlags::RDONLY | OFlags::CLOEXEC | OFlag= s::NOFOLLOW, > + Mode::empty(), > + ResolveFlags::NO_MAGICLINKS | ResolveFlags::NO_SYMLINKS, > + ) I pointed out in my review of v2 that OFlags::NOFOLLOW is redundant with ResolveFlags::NO_SYMLINKS, but now it seesm to have come back across the board. > + .map_err(|e| format!("Cannot open /sys/fs/cgroup: {e}"))?; > + > + let lock_operation =3D if exclusive { > + FlockOperation::LockExclusive > + } else { > + FlockOperation::LockShared > + }; > + rustix::fs::flock(cgroup_root.as_fd(), lock_operation) > + .map_err(|e| format!("Cannot lock /sys/fs/cgroup: {e}"))?; > + Ok(Self { > + path: PathBuf::from("/sys/fs/cgroup"), > + fd: vec![(cgroup_root, exclusive)], > + }) > + } > + > + pub fn enable_delegation(&self, depth: usize) -> Result<(), Errno> { > + let (fd, exclusive) =3D &self.fd[self.fd.len() - depth]; > + assert!(exclusive); > + rustix::fs::fsetxattr(fd.as_fd(), c"user.delegate", b"1", XattrF= lags::empty()) > + } > + > + pub fn enable_subtree_control(&self, depth: usize) -> Result<(), Str= ing> { > + let (fd, exclusive) =3D &self.fd[self.fd.len() - depth]; > + assert!(exclusive); > + let p =3D Path::new("cgroup.controllers"); > + let mut buf =3D self.read_control_file(fd.as_fd(), p)?; > + let mut subtree =3D vec![]; > + if buf.ends_with(b"\n") { > + buf.pop(); > + } > + for controller in buf.split(|&b| b =3D=3D b' ').filter(|e| !e.is= _empty()) { > + for &c in controller { > + if c <=3D b' ' || c >=3D 0x7F { > + return Err(format!("Bad byte {c} in cgroup.controlle= rs")); > + } > + } > + if !subtree.is_empty() { > + subtree.push(b' '); > + } > + subtree.push(b'+'); > + subtree.extend_from_slice(controller); > + } > + if !subtree.is_empty() { > + self.write_cgroup_value("cgroup.subtree_control", str::from_= utf8(&subtree).unwrap())?; > + } > + Ok(()) > + } > + > + pub fn read_control_file(&self, fd: BorrowedFd, p: &Path) -> Result<= Vec, String> { > + let mut buf =3D Vec::new(); > + let err =3D |e: &dyn Display, p: &Path, msg: &str| { > + let path =3D 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. > + .read_to_end(&mut buf) > + .map_err(|e| err(&e, p, "read"))?; > + Ok(buf) > + } > + > + /// Open a single component as a sub-cgroup > + fn open_sub_cgroup_raw(&self, access: OFlags, component: &[u8]) -> R= esult { > + assert_single_component(component); > + rustix::fs::openat2( > + self.as_fd(), > + Path::new(OsStr::from_bytes(component)), > + OFlags::CLOEXEC | OFlags::NOFOLLOW | access, > + Mode::empty(), > + ResolveFlags::NO_SYMLINKS > + | ResolveFlags::NO_MAGICLINKS > + | ResolveFlags::BENEATH > + | ResolveFlags::NO_XDEV, This is also doing exactly the same thing as open_subtree_raw, except it allows changing the access mode, and also sets NO_MAGICLINKS. I don't think any of the other callers of open_subtree_raw would need to open magic links, so maybe this is evidence this should be unified with them? > + ) > + } > + > + pub fn open_sub_cgroup( > + &mut self, > + path: &std::path::Path, > + exclusive: bool, > + allow_missing: bool, > + ) -> Result { > + let mut iter =3D path.components().peekable(); > + while let Some(component) =3D iter.next() { Perhaps would be nicer: let mut components =3D path.components().peekable(); for component in components { > + let component =3D match component { > + Component::Normal(component) =3D> component, > + _ =3D> unreachable!(), > + }; > + let sub_fd =3D match self > + .open_sub_cgroup_raw(OFlags::DIRECTORY | OFlags::RDONLY,= component.as_bytes()) > + { > + Ok(sub_fd) =3D> { > + 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? 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. > + sub_fd > + } > + Err(Errno::NOENT) if allow_missing =3D> return Ok(false), > + Err(e) =3D> { > + return Err(format!( > + "Cannot open sub-cgroup {component:?} of {:?}: {= e}", > + self.path > + )); > + } > + }; > + let exclusive =3D exclusive && iter.peek().is_none(); > + let lock_operation =3D if exclusive { > + FlockOperation::LockExclusive > + } else { > + FlockOperation::LockShared > + }; > + rustix::fs::flock(sub_fd.as_fd(), lock_operation).map_err(|e= | { > + let msg =3D format!("Cannot lock sub-cgroup {:?}: {e}", = self.path); > + self.path.pop(); > + msg > + })?; > + self.fd.push((sub_fd, exclusive)); > + } > + Ok(true) > + } > + > + pub fn open_subtree(&self, path: &std::path::Path) -> Result { > + let dirfd =3D self.as_fd(); > + open_subtree_raw(path, dirfd) > + } If open_subtree_raw just took &dyn AsFd, there'd be no need for this method. > + > + fn exclusive(&self) -> bool { > + self.fd.last().unwrap().1 > + } > + > + pub fn joined_path(&self, p: &Path) -> PathBuf { > + let mut owned_p =3D self.path.clone(); > + owned_p.push(p); > + owned_p > + } > + > + pub fn wait_for_empty(&self) -> std::io::Result<()> { > + assert!(self.exclusive()); > + let wait_file =3D self.open_subtree(std::path::Path::new("cgroup= .events"))?; > + 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, > + events: libc::POLLPRI | libc::POLLERR, > + revents: 0, > + }; > + let mut v =3D 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 =3D=3D b'\n').any(|line| line =3D=3D b"pop= ulated 0") { > + break; > + } > + // SAFETY: FFI call, valid arguments, fds contains 1 element > + if unsafe { libc::poll(&raw mut fds, 1, -1) } !=3D 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. > + Ok(()) > + } > + > + pub(crate) fn make_child(&mut self, path: &Path) -> Result<(), Errno= > { > + assert!(self.exclusive()); > + let component =3D 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) =3D> {} > + bad =3D> return bad, > + } > + let p =3D self.open_sub_cgroup_raw(OFlags::RDONLY | OFlags::DIRE= CTORY, 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. > + assert!(self.exclusive()); > + match rustix::fs::unlinkat(self.as_fd(), Path::new(path), AtFlag= s::REMOVEDIR) { > + // Trying to purge a deleted cgroup is not an error. > + Ok(()) | Err(Errno::NOENT) =3D> return Ok(()), > + Err(Errno::BUSY) =3D> {} This could use a comment. > + Err(e) =3D> return Err(format!("Cannot purge {:?}: {e}", sel= f.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. > + ) > + .map_err(|e| format!("Cannot relock {:?}: {e}", self.path.parent= ()))?; > + self.write_cgroup_value("cgroup.kill", "1")?; > + self.wait_for_empty() > + .map_err(|e| format!("Cannot wait for cgroup to become empty= : {e}"))?; > + let fd =3D self.fd.pop().unwrap().0; > + let v =3D (|| { > + remove_recursively(fd, 1000) > + .map_err(|e| format!("Cannot remove {:?}: {e}", self.pat= h))?; > + rustix::fs::flock(self.as_fd(), FlockOperation::LockExclusiv= e) > + .map_err(|e| format!("Cannot lock {:?}: {e}", self.path)= )?; We already checked self.exclusive above, meaning we already have this lock on self? > + match rustix::fs::unlinkat( > + self.as_fd(), > + Path::new(self.path.file_name().unwrap()), I am too confused about what self.path is to know what to make of this. self.as_fd() should be the fd of the cgroup directory, and self.path sounds like it should be the path to this cgroup, so how can this cgroup directory have self.path.file_name() within it? This needs clearer names or a refactor or something. > + AtFlags::REMOVEDIR, > + ) { > + // something might have re-created the cgroup in the mea= ntime, which is okay > + Ok(()) | Err(Errno::BUSY) =3D> Ok(()), > + Err(e) =3D> Err(format!("Cannot lock {:?}: {e}", self.pa= th)), > + } > + })(); > + assert!(self.path.pop()); I don't get this. Where was it pushed? (But would prefer to just not attempt to track this, as mentioned above.) > + v > + } > + > + pub(crate) fn write_cgroup_value(&self, name: &str, value: &str) -> = Result<(), String> { > + let path =3D Path::new(name); > + let fd =3D rustix::fs::openat2( > + self.as_fd(), > + path, > + OFlags::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlag= s::WRONLY, > + Mode::empty(), > + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveF= lags::NO_XDEV, > + ) > + .map_err(|e| format!("Cannot open {:?}: {}", self.joined_path(Pa= th::new(name)), e))?; Could also use ResolveFlags::MAGIC_LINKS and be unified with the other openat2 invocations maybe? I don't understand why this is NOATIME but others aren't. > + File::from(fd).write_all(value.as_bytes()).map_err(|e| { > + format!( > + "Cannot write {:?} to {:?}: {}", > + value, > + self.joined_path(Path::new(name)), > + e > + ) > + }) > + } > +} > + > +fn open_subtree_raw(path: &Path, dirfd: BorrowedFd<'_>) -> Result { > + rustix::fs::openat2( > + dirfd, > + path, > + OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY, > + Mode::empty(), > + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags= ::NO_XDEV, > + ) > +} > + > +fn remove_recursively(fd: OwnedFd, remaining_depth: usize) -> Result<(),= Errno> { > + if remaining_depth < 1 { > + panic!("control groups too deeply nested"); > + } > + let mut d =3D rustix::fs::Dir::new(fd).expect("cannot start iteratin= g"); > + while let Some(element) =3D d.next() { > + let element =3D element.expect("Iterating through a cgroup direc= tory failed?"); > + if element.file_type() !=3D rustix::fs::FileType::Directory { > + continue; > + } > + > + let remaining_depth =3D remaining_depth - 1; > + let d: &rustix::fs::Dir =3D &d; > + let dirfd =3D d.fd().unwrap(); > + let path =3D element.file_name(); > + remove_all(remaining_depth, dirfd, path)?; > + } > + drop(d); > + Ok(()) > +} > + > +fn remove_all( > + remaining_depth: usize, > + dirfd: BorrowedFd<'_>, > + path: &std::ffi::CStr, > +) -> Result<(), Errno> { > + if path =3D=3D c"." || path =3D=3D c".." { > + return Ok(()); > + } > + if rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR).is_ok() { > + return Ok(()); > + } > + let fd =3D rustix::fs::openat2( > + dirfd, > + path, > + OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY | OFlags::DI= RECTORY, > + 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. > diff --git a/tools/cgroup-setup/src/main.rs b/tools/cgroup-setup/src/main= .rs > new file mode 100644 > index 0000000000000000000000000000000000000000..2e7a4e25213a4aa2449b292f8= 005e1da23cbb1f9 > --- /dev/null > +++ b/tools/cgroup-setup/src/main.rs > @@ -0,0 +1,347 @@ > +// SPDX-License-Identifier: EUPL-1.2+ > +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour > + > +use std::{ > + ffi::{OsStr, OsString}, > + os::unix::prelude::*, > + path::{Path, PathBuf}, > +}; > + > +use crate::cgroup::Cgroup; > + > +mod cgroup; > + > +fn check_path(path: &OsStr) -> Result<(), String> { > + if path.is_empty() { > + return Ok(()); > + } > + > + for component in path.as_bytes().split(|&b| b =3D=3D b'/') { Would be a lot nicer to take &Path and use Path::components. > + match component { > + b"" | b"." | b".." =3D> { > + return Err(format!("Path {path:?} has empty, ., or .. co= mponent")); > + } > + // Cannot happen: command line arguments have no NUL byte, > + // and /proc/self/cgroup having a NUL byte is a kernel bug. > + _ if component.contains(&b'\0') =3D> panic!("Path {path:?} h= as NUL byte"), It is in fact an invariant of both the Path and OsStr types (on Unix) that there are no NUL bytes, so there is no need to check for it at all. > + _ if component.len() > 255 =3D> { > + return Err(format!( > + "Path {path:?} has component {:?} that is longer tha= n 255 bytes", > + OsStr::from_bytes(component) > + )); > + } Why on earth would we need to check for this? This limit is totally up to the kernel. Our code won't break with an excessively long path component. > + _ =3D> {} > + } > + } > + > + Ok(()) > +} > + > +/// Get the path of the cgroup for the provided command-line argument. > +/// Returns an empty path if the path is "/", or if it is "." and the > +/// current cgroup is "/". Please standardize terminology between "current cgroup" and "local cgroup", or if those are not the same thing use clearer phrasing. I hope all the different modes of this function are actually needed=E2=80=A6 (I assume they are but haven't reviewed the patches that make use of it yet.) > +/// > +/// # Errors > +/// > +/// Fails if the provided path is invalid or empty, or if it is relative > +/// and the local cgroup cannot be determined. > +fn get_cgroup(cgroup_path: OsString) -> Result { Please call this function something clearer. Also using OsString where PathBuf would be more appropriate again=E2=80=A6 > + if cgroup_path.as_bytes().starts_with(b"/") { =E2=80=A6 which would allow using Path::is_absolute if fixed. > + let mut cgroup_path =3D cgroup_path.into_vec(); > + cgroup_path.remove(0); Please use Path functions rather than byte manipulation like this. > + if cgroup_path.is_empty() { > + return Err("cgroup path cannot be /".to_owned()); > + } > + let cgroup_path =3D OsString::from_vec(cgroup_path); > + check_path(&cgroup_path)?; > + Ok(cgroup_path.into()) > + } else if cgroup_path.is_empty() { > + Err("cgroup path cannot be empty".to_owned()) > + } else { > + check_path(&cgroup_path)?; > + let mut local_cgroup =3D local_cgroup()?; Can you reorder all these function definitions to be something more sensible? get_cgroup is here, near the start of main.rs, but local_cgroup, which it is the only direct caller of, is all the way at the other end. (Personally I like to define utility functions directly above their only caller =E2=80=94 I think that creates the most natural rea= ding flow.) > + local_cgroup.push(cgroup_path); > + Ok(local_cgroup) > + } > +} > + > +/// Open the cgroup corresponding to the provided path. > +/// It must have already been made relative to `/sys/fs/cgroup`. > +/// > +/// # Errors > +/// > +/// Fails if the cgroup operation fails. This "Errors" section is just stating the obvious. > +fn open_cgroup(path: &Path, exclusive: bool) -> Result { > + if path.as_os_str().is_empty() { > + Cgroup::new(exclusive) > + } else { > + let mut cgroup =3D Cgroup::new(false)?; > + cgroup.open_sub_cgroup(path, exclusive, false)?; > + Ok(cgroup) > + } > +} I feel like it would be more natural if Cgroup::new just took two parameters and worked this way. > + > +/// Open the cgroup corresponding to the provided path's parent. > +/// It is made relative to the process's own cgroup if needed. > +/// > +/// # Errors > +/// > +/// Fails if the cgroup operation fails. > +fn open_relative_cgroup(arg: OsString) -> Result<(PathBuf, Cgroup), Stri= ng> { > + let path =3D get_cgroup(arg)?; > + let cgroup =3D open_cgroup(path.parent().expect("always has a parent= "), true)?; > + Ok((path, cgroup)) > +} This is a deeply confusing function. Why is opening a cgroup for the path's _parent_ an operation that we should have a dedicated function for? Why not have the caller pass in the actual path of the cgroup it wants to open? This really looks like a function that's doing as many different things at it has lines, and should just be inlined. > + > +fn main() { > + let mut args =3D std::env::args_os(); > + let Some(prog_name) =3D args.next() else { > + eprintln!("No command line arguments (argv[0] is NULL)"); > + std::process::exit(1); > + }; > + match main_(&prog_name, args) { > + Ok(()) =3D> {} > + Err(e) =3D> { > + eprintln!("{prog_name:?}: {}", e); > + std::process::exit(1); > + } > + } > +} > + > +fn main_(prog_name: &OsStr, mut args: std::env::ArgsOs) -> Result<(), St= ring> { main_ is a bit confusingly similar to name. I like "run" for this sort of function, and we use that elsewhere. > + match prog_name > + .as_bytes() > + .split(|&b| b =3D=3D b'/') > + .next_back() > + .unwrap() Another place that should use Path. > + { > + b"cgroup-s6-finish" =3D> { > + return s6_finish(&mut args); > + } > + b"cgroup-setup" =3D> {} It is inconsistent for this not to also be its own function. Then the match could just be an expression that returned the result of the appropriate function. > + b"cgroup-purge" =3D> { > + if args.len() !=3D 1 { > + return Err(format!( > + "cgroup-purge takes one argument, got {}", > + args.len() > + )); > + } > + let cgroup_path =3D args.next().unwrap(); > + let (path, mut cgroup) =3D open_relative_cgroup(cgroup_path)= ?; > + let cgroup_target =3D Path::new(path.file_name().unwrap()); > + return cgroup.purge(cgroup_target); > + } > + _ =3D> { > + return Err(format!( > + "must be invoked as \"cgroup-setup\" \ > + \"cgroup-purge\", or \"cgroup-s6-finish\", \ > + got {prog_name:?}", > + )); > + } > + }; > + let mut leaf =3D false; > + let mut cgroup_path; > + let mut delegate =3D false; > + let mut init_subtree =3D false; > + let mut child_name: Option<&'static OsStr> =3D None; > + let mut wait =3D true; > + loop { > + cgroup_path =3D args.next(); > + let Some(ref arg_) =3D cgroup_path else { > + break; > + }; > + let arg_ =3D arg_.as_bytes(); > + if arg_ =3D=3D b"--" { > + cgroup_path =3D args.next(); > + break; > + } > + if !arg_.starts_with(b"-") { > + break; > + } > + > + if !arg_.starts_with(b"--") { > + return Err("takes no short options".to_owned()); > + } > + > + match &arg_[2..] { > + b"leaf" =3D> leaf =3D true, > + b"delegate" =3D> delegate =3D true, > + b"init-subtree" =3D> init_subtree =3D true, > + b"wait" =3D> wait =3D true, > + b"no-wait" =3D> wait =3D false, > + b"child-name" if child_name.is_none() =3D> match args.next()= { > + Some(arg) =3D> child_name =3D Some(arg.leak()), > + None =3D> return Err("--child-name: missing argument".to= _owned()), > + }, > + b"child-name" =3D> return Err("--child-name: cannot be used = twice".to_owned()), > + arg =3D> match str::from_utf8(arg) { > + Ok(e) =3D> return Err(format!("unknown long option {e:?}= ")), > + Err(_) =3D> return Err("long option isn't UTF-8".to_owne= d()), > + }, > + } > + } > + > + let default_child_name =3D OsStr::from_bytes(b"$inner.service"); > + > + let child_name =3D Path::new(child_name.unwrap_or(default_child_name= )); > + > + let Some(mut cgroup_path) =3D cgroup_path else { > + return Err("have no positional arguments, expected at least 1".t= o_owned()); > + }; > + > + // Allow --init-subtree . > + if cgroup_path.as_bytes() =3D=3D b"." && init_subtree && !leaf { > + cgroup_path =3D child_name.to_owned().into(); > + leaf =3D true; > + } > + > + let (path, mut cgroup) =3D open_relative_cgroup(cgroup_path)?; > + let cgroup_target =3D Path::new(path.file_name().unwrap()); > + cgroup > + .make_child(cgroup_target) > + .map_err(|e| format!("Cannot make child cgroup: {e}"))?; > + if wait { > + cgroup > + .wait_for_empty() > + .map_err(|e| format!("Cannot wait for {path:?} to be empty: = {e}"))?; > + } > + let pid =3D std::process::id().to_string(); > + if leaf { > + if args.len() !=3D 0 { > + // If we aren't delegating any cgroups, don't create a sub-c= group. > + cgroup > + .write_cgroup_value("cgroup.procs", &pid) > + .map_err(|e| format!("Cannot write to {path:?}/cgroup.pr= ocs: {e}"))?; > + } > + } else { > + // If the child process will need to manage cgroups itself, it w= ill need > + // to set up a sub-cgroup due to the "no internal processes" rul= e. It's > + // simplest to just do it automatically. > + cgroup.make_child(Path::new(child_name)).map_err(|e| { > + format!( > + "Cannot create child cgroup {}/{}: {e}", > + path.display(), > + child_name.display() > + ) > + })?; > + if args.len() !=3D 0 { > + cgroup > + .write_cgroup_value("cgroup.procs", &pid) > + .map_err(|e| { > + format!( > + "Cannot write to {}/{}/cgroup.procs: {e}", > + path.display(), > + child_name.display() > + ) > + })?; > + } > + } > + if !leaf { > + cgroup.enable_subtree_control(2)?; > + } > + if init_subtree { > + cgroup.enable_subtree_control(1)?; > + } > + if delegate { > + cgroup > + .enable_delegation(1) > + .map_err(|e| format!("Cannot enable cgroup delegation in {pa= th:?}: {e}"))?; > + } > + let Some(program_name) =3D args.next() else { > + return Ok(()); > + }; > + let e =3D std::process::Command::new(&program_name).args(args).exec(= ); > + Err(format!("Cannot spawn child {:?}: {}", program_name, e)) > +} > + > +fn s6_finish(args: &mut std::env::ArgsOs) -> Result<(), String> { > + if args.len() < 3 { > + return Err(format!( > + "s6 finish scripts take at least 3 arguments, got {}", > + args.len() > + )); > + } > + let status =3D parse_digit_string(&args.next().unwrap(), "exit statu= s")?; > + let signal =3D args.next().unwrap(); > + let signal =3D if status =3D=3D 256 { > + Some(parse_digit_string(&signal, "signal number")?) > + } else { > + None > + }; > + let service =3D args.next().unwrap(); > + > + let (path, mut cgroup) =3D open_relative_cgroup(service)?; > + let cgroup_target =3D Path::new(path.file_name().unwrap()); > + let exit_125 =3D if let Some(signal) =3D signal { > + match signal as libc::c_int { > + libc::SIGBUS > + | libc::SIGFPE > + | libc::SIGABRT > + | libc::SIGTRAP > + | libc::SIGSEGV > + | libc::SIGILL =3D> { > + // Process *crashed*, indicating a *possible exploit att= empt*. > + // s6 should *not* restart it. This is distinct from a = Rust panic, > + // which is much less likely to indicate memory corrupti= on. > + 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). > + _ =3D> false, > + } > + } else { > + false > + }; > + if exit_125 { > + // Ignore panics. Exit status is more important. > + // We already had a core dump. > + let _ =3D std::panic::catch_unwind(std::panic::AssertUnwindSafe(= || { > + match cgroup.purge(cgroup_target) { > + Ok(()) =3D> {} > + Err(e) =3D> { > + eprintln!("cgroup-s6-finish: Failed to purge cgroup:= {e}") > + } > + }; > + })); > + std::process::exit(125) > + } else { > + cgroup.purge(cgroup_target) > + } > +} > + > +fn parse_digit_string(digits: &OsStr, msg: &str) -> Result { > + let checked =3D match str::from_utf8(digits.as_bytes()) { > + Ok(s) =3D> s, > + Err(e) =3D> return Err(format!("{msg} is not UTF-8: {e}")), > + }; > + let r =3D checked > + .parse::() > + .map_err(|e| format!("{msg} {digits:?} is a bad 16-bit number: {= e}"))?; > + match checked.as_bytes() { > + b"0" | [b'1'..=3Db'9', ..] =3D> Ok(r), > + [b'0', ..] =3D> Err(format!("{msg} {} has a leading 0", digits.d= isplay())), > + _ =3D> Err(format!("{msg} {} starts with +", digits.display())), > + } Surely we trust s6 to turn a number into a string. These checks add nothin= g. > +} > + > +fn local_cgroup() -> Result { > + let mut local_cgroup: Vec =3D std::fs::read("/proc/thread-self/c= group") > + .map_err(|e| format!("cannot read /proc/thread-self/cgroup: {e}"= ))?; > + let local_cgroup_len =3D local_cgroup.len(); > + if local_cgroup_len < 5 > + || local_cgroup[..4] !=3D *b"0::/" > + || local_cgroup[local_cgroup_len - 1] !=3D 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.) > + { > + // It's possible to get here if the cgroup path contains a newli= ne, > + // but that never happens in Spectrum. > + return Err(format!( > + "Invalid contents {local_cgroup:?} of /proc/thread-self/cgro= up - \ > + 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 =3D 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? > + Ok(PathBuf::from(local_cgroup)) > +} > > --=20 > 2.55.0 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQRV/neXydHjZma5XLJbRZGEIw/wogUCamDpSwAKCRBbRZGEIw/w otwLAPsGr3d9ma/LeJuShKBmX2HnnlDoImWlH8jddybTL/QS3AD/WVmFkw19jXYo FfQpjSVYb3BkEABtKaXeOXoEXA88kA8= =jbET -----END PGP SIGNATURE----- --=-=-=--