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 20D1B975D; Wed, 24 Jun 2026 12:14:07 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id 917EE9756; Wed, 24 Jun 2026 12:14:04 +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 fout-a4-smtp.messagingengine.com (fout-a4-smtp.messagingengine.com [103.168.172.147]) by atuin.qyliss.net (Postfix) with ESMTPS id A3E3D968E for ; Wed, 24 Jun 2026 12:14:02 +0000 (UTC) Received: from phl-compute-03.internal (phl-compute-03.internal [10.202.2.43]) by mailfout.phl.internal (Postfix) with ESMTP id 018A6EC008D; Wed, 24 Jun 2026 08:14:01 -0400 (EDT) Received: from phl-frontend-04 ([10.202.2.163]) by phl-compute-03.internal (MEProxy); Wed, 24 Jun 2026 08:14:01 -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=fm3; t=1782303240; x=1782389640; bh=rkQuIuTpyo 5DaU9CkELTksqyc5Z3nNRyM6PuGOXW90A=; b=QVJG15+RmTw+BZkwhJGvW0kp0B Z6/2TJNUulqMTALu73xZ8dTMiWqYtIDllmqPfC6RCMPakl9kZt3vn2peZ/3Gyk+V jvJjlvYxUElh0Am5bRRJzanVZaE6QuwDPWx9IxTtZz/AP4+sE+V+1WtSzOB0mLVx llBEsGW3qKzYOqacTbdmjvTUn9xWNQmzlCNOG1orztSJ8NEQ0W/7MTR4q6FBP3pM /3vkNt05oI0aUyHM+nRqBRIWpP+e19SpvTR4gLUYjzaotaKDDvQJbo3pWiav9juX qXzVfUwPTF84HVFrxDIpn7U8IjK0SMgeAy2xWmSZhvrTRf+oc9297/Gv2iaA== 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=fm1; t= 1782303240; x=1782389640; bh=rkQuIuTpyo5DaU9CkELTksqyc5Z3nNRyM6P uGOXW90A=; b=YmnDFYJo4IZwNw62GHwLeFtluPFznoXLY3lbLB8MT9tDe7CErr4 mDI2nmeWoNtMhIQt3/2Suq+89m2q0wMrlp+bbD+ASJoDs8PW3xNozpqjFlqB5J86 q0meOLhltlL6fwYz0mK0B5chhlmvvZUbTejT77Oux0FipPdlaV0+dSpVXmFlF9oB PvfsRDlBHK3RJwV23Y0wuKabJQPn0irgluVChpLjexRVAHdamvZUItYY82FFftn6 LuP9haMJTU7KAMNKGQz4p1jjpylgB+QiCoxlbV7fo+lfoP3tm54IpLaJ1SJ0NJdu NEU9vCzXp9ec7+3wlRVGvqYV5L4K6/rFbhQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: dmFkZTEOKInQLFJPVSeGOMAw8naODMjZdiWEJe1peaVzgFNAQ7SQ5m+ItqS+IfC5zkqYcT IsXLT4ZhaAWqmAl7j5F6TG5A5XRfs7+AEkbENVPTo5fGCm6JRFgvhbWVJl7VLO/6Io3fs4 vWt4TrmnmlU43JadNU8lAl4Ufa7TCKxDHGldaAVU2vwVT/Sfph4FwYxBuR8mcURk9Vfga9 NqgMBawq3aEv563jVL+qBzED7ygJq96ctwIZnWlNMZ5bKj8iSx8V3k0Hl6N8NKYtYx4A/G hAGbxmiVeNwkux7KTIVJnviTN5qfXFXkqObgpdKN4zdSpuYZ4/DSuiZTacw8agoBhkqNQe 4GN404NPdbaLLHsn5fU360Nwue0HPxhSGzCfe74GORw0jKsoC8ZGGX3Z+7ac19f1FRTND/ sH/VIj0caQQuikEhxxmT+mKC7YHoEJ0I908SVYEqkNgC8yRcaWu3yRjqifmNFZW0SMmXPa FYwP9AO8P4kxRPGtuQy1MGZ3lmAMs7L1si5ABImsh2cCedXzLo+oxxplCkKTx+LGfV8yyQ iAIzrjHCE2xWFIrY+vu66vR2RrtYmsf+QHuavd/6QsOcdAk61o+5d0Dzr10DD6KJbbZGdH zRE0+6nv4ykl/RRP/XSubgoU+klCCeoohV7syWti5Bf6D2WnUVYKhrv/yMsw X-ME-Proxy: Feedback-ID: i12284293:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 24 Jun 2026 08:14:00 -0400 (EDT) Received: by mbp.qyliss.net (Postfix, from userid 1000) id 5EB2C8350112; Wed, 24 Jun 2026 14:13:57 +0200 (CEST) From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH v2] Set up control groups for most services In-Reply-To: <20260620-cgroups-v2-1-ccae224b6c85@gmail.com> References: <20260620-cgroups-v1-1-0e5abf35101b@gmail.com> <20260620-cgroups-v2-1-ccae224b6c85@gmail.com> Date: Wed, 24 Jun 2026 14:13:54 +0200 Message-ID: <87qzlw5e65.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Message-ID-Hash: 2LRXYDN7LDVMIIVWASVLAR6R5VKEKRAV X-Message-ID-Hash: 2LRXYDN7LDVMIIVWASVLAR6R5VKEKRAV 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 cgroups are handled by a Rust tool. The name of the cgroup is > autogenerated from the service name. > > Using cgroups for process control is not implemented yet. > Perhaps you could include some more details of how the cgroup hierarchy is supposed to be set up and why, either in the patch body or in documentation? As I understood it, the point of using cgroups was that we could bundle all services for a VM, including the VMM, into a single cgroup, but I don't see that here, just a pure translation of the service hierarchy (which the VMM might not even be part of). Are per-VM cgroups coming later? > diff --git a/host/rootfs/image/etc/fstab b/host/rootfs/image/etc/fstab > index 18bb5e45abafeaf43871306ce8233239e5c07f76..d92364d83f4d26f766fcd5b49= 4614c06a630d2fe 100644 > --- a/host/rootfs/image/etc/fstab > +++ b/host/rootfs/image/etc/fstab > @@ -6,3 +6,4 @@ tmpfs /dev/shm tmpfs nosuid,nodev 0 0 > tmpfs /media tmpfs nosuid,nodev,noexec,nosymfollow,mode=3D755 0 0 > sysfs /sys sysfs nosuid,nodev,noexec 0 0 > tmpfs /tmp tmpfs nosuid,nodev 0 0 > +cgroup2 /sys/fs/cgroup cgroup2 nosuid,nodev,noexec,nosymfollow 0 0 Alignment is off here. The rest of the file uses tabs at 8 characters. (I know, I know, but it's traditional.) > diff --git a/host/rootfs/image/etc/s6-linux-init/run-image/service/serial= -getty-generator/run b/host/rootfs/image/etc/s6-linux-init/run-image/servic= e/serial-getty-generator/run > index bf66ab0878fc6d8e77ca71a4a89e50c139a6bb11..5c9988c994ed9b94232905567= 3b332d0aa918957 100755 > --- a/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty-= generator/run > +++ b/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty-= generator/run > @@ -1,7 +1,8 @@ > -#!/bin/execlineb -WP > +#!/bin/execlineb -WS1 > # SPDX-License-Identifier: EUPL-1.2+ > # SPDX-FileCopyrightText: 2024-2025 Alyssa Ross >=20=20 > +cgroup-setup -- $1 > piperw 3 4 > background { > fdclose 3 What benefit does this cgroup provide? > diff --git a/host/rootfs/image/etc/s6-linux-init/run-image/service/serial= -getty/run b/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-g= etty/run > index 78f794202bf174f3c036f3e20755ac087a988277..ed0808b9e45b077023f237a0f= 9c0e5c830015ac2 100755 > --- a/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty/= run > +++ b/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-getty/= run > @@ -1,5 +1,6 @@ > -#!/bin/execlineb -WP > +#!/bin/execlineb -WS1 > # SPDX-License-Identifier: EUPL-1.2+ > # SPDX-FileCopyrightText: 2023 Alyssa Ross >=20=20 > +cgroup-setup -- $1 > s6-svscan -d3 instance We're putting supervisors of instances services in a cgroup? Can you explain to me why that's useful? > diff --git a/host/rootfs/image/etc/s6-linux-init/run-image/service/vm-ser= vices/template/data/service/dbus/run b/host/rootfs/image/etc/s6-linux-init/= run-image/service/vm-services/template/data/service/dbus/run > index f0507dc92aadf25c092f0719eb767542ac0c4451..89676ac91d3e22db7726fc869= a065137b031d0be 100755 > --- a/host/rootfs/image/etc/s6-linux-init/run-image/service/vm-services/t= emplate/data/service/dbus/run > +++ b/host/rootfs/image/etc/s6-linux-init/run-image/service/vm-services/t= emplate/data/service/dbus/run > @@ -1,7 +1,8 @@ > -#!/bin/execlineb -WP > +#!/bin/execlineb -WS1 > # SPDX-License-Identifier: EUPL-1.2+ > # SPDX-FileCopyrightText: 2024-2025 Alyssa Ross >=20=20 > +cgroup-setup -- $1 > importas -i VM VM >=20=20 > if { This seems to get stuck in udevadm wait? > diff --git a/tools/cgroup-setup/Cargo.toml b/tools/cgroup-setup/Cargo.toml > new file mode 100644 > index 0000000000000000000000000000000000000000..9cce0be706151dfd5264a81af= b3ee9a6c851367c > --- /dev/null > +++ b/tools/cgroup-setup/Cargo.toml > @@ -0,0 +1,17 @@ > +# SPDX-License-Identifier: CC0-1.0 > +# SPDX-FileCopyrightText: 2025 Alyssa Ross > +# 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"] } > + > +[profile.release] > +split-debuginfo =3D "symbols" > +strip =3D "symbols" > +lto =3D true > +panic =3D "abort" What's with all this stuff? Why do we set it only for this program, and not for any others? Please don't introduce divergence in build settings between programs without a good reason. > diff --git a/tools/cgroup-setup/src/cgroup.rs b/tools/cgroup-setup/src/cg= roup.rs > new file mode 100644 > index 0000000000000000000000000000000000000000..cd77becc0b40bfdcd983e53b6= 3b7c21e4308d5fc > --- /dev/null > +++ b/tools/cgroup-setup/src/cgroup.rs > @@ -0,0 +1,181 @@ > +// SPDX-License-Identifier: EUPL-1.2+ > +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour > + > +use std::fs::File; > +use std::io::{Read, Seek}; > +use std::os::fd::{AsFd, OwnedFd}; > +use std::os::fd::{AsRawFd, BorrowedFd}; > + > +use std::path::{Path, PathBuf}; > + > +use rustix::fs::AtFlags; > +use rustix::path::Arg; > +use rustix::{ > + fs::{Mode, OFlags, ResolveFlags}, > + io::Errno, > +}; > + > +pub(crate) struct LeafCgroup { > + path: PathBuf, > + fd: OwnedFd, root_fd would be a clearer name, if I'm understanding correctly. > +} > + > +enum Access { > + Read, > + Write, > +} Given this isn't public, it doesn't seem to add any value over passing around OFlags::RDONLY and OFlags::WRONLY directly. > + > +impl LeafCgroup { > + fn open_subtree(&self, path: &std::path::Path, access: Access) -> Re= sult { > + rustix::fs::openat2( > + self.fd.as_fd(), > + path, > + OFlags::NOATIME > + | OFlags::CLOEXEC > + | OFlags::NOFOLLOW > + | match access { > + Access::Write =3D> OFlags::WRONLY, > + Access::Read =3D> OFlags::RDONLY, > + }, > + Mode::empty(), > + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveF= lags::NO_XDEV, > + ) > + } > + > + pub fn kill_processes(&self) -> std::io::Result<()> { > + let kill_fd =3D self.open_subtree(std::path::Path::new("cgroup.k= ill"), Access::Write)?; > + let wait_file =3D self.open_subtree(std::path::Path::new("cgroup= .events"), Access::Read)?; > + let poll_fd =3D wait_file.as_raw_fd(); > + let mut wait_fd =3D File::from(wait_file); > + assert_eq!(rustix::io::write(kill_fd.as_fd(), b"1")?, 1, "kernel= bug"); > + let mut fds =3D libc::pollfd { > + fd: poll_fd, > + events: libc::POLLIN | libc::POLLPRI | libc::POLLRDHUP, > + revents: 0, > + }; > + // SAFETY: FFI call, valid arguments, fds contains 1 element > + let mut v =3D vec![]; > + 'a: loop { > + v.clear(); > + if unsafe { libc::poll(&raw mut fds, 1, -1) } !=3D 1 { > + panic!("poll failed"); > + } Shouldn't we start polling before writing? Otherwise we might miss the notification, if it comes in between the write and the poll. > + 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"); > + for substr in v.split(|&c| c =3D=3D b'\n') { > + if substr =3D=3D b"populated 0" { Could do this in one line, and avoid the labelled break: if v.split(|&c| c =3D=3D b'\n').any(|line| line =3D=3D b"populated 0") { (I think "line" is a clearer name than "substr".) > + break 'a; > + } > + } > + } > + Ok(()) > + } > +} > + > +impl AsFd for LeafCgroup { > + fn as_fd(&self) -> std::os::fd::BorrowedFd<'_> { > + self.fd.as_fd() > + } > +} > + > +impl LeafCgroup { > + pub(crate) fn open_cgroup_at(&self, p: &Path) -> Result= { > + let fd =3D rustix::fs::openat2( > + self.fd.as_fd(), > + p, > + OFlags::NOATIME > + | OFlags::CLOEXEC > + | OFlags::NOFOLLOW > + | OFlags::RDONLY > + | OFlags::DIRECTORY, > + Mode::empty(), > + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveF= lags::NO_XDEV, > + )?; > + Ok(Self { > + fd, > + path: self.path.join(p), > + }) > + } > + pub(crate) fn open_cgroup(fd: BorrowedFd, p: &Path) -> Result { > + let fd =3D rustix::fs::openat2( > + fd, > + p, > + OFlags::NOATIME > + | OFlags::CLOEXEC > + | OFlags::NOFOLLOW > + | OFlags::RDONLY > + | OFlags::DIRECTORY, > + Mode::empty(), > + //ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | Resolv= eFlags::NO_XDEV, Stray comment. > + ResolveFlags::NO_SYMLINKS, > + )?; > + Ok(Self { > + fd, > + path: p.to_owned(), > + }) > + } > + pub(crate) fn make_child(&self, p: &Path) -> Result<(), Errno> { > + rustix::fs::mkdirat( > + self.fd.as_fd(), > + p, > + Mode::RUSR > + | Mode::WUSR > + | Mode::XUSR > + | Mode::RGRP > + | Mode::XGRP > + | Mode::ROTH > + | Mode::XOTH, > + ) > + } > + > + pub(crate) fn delete_child(&self, path: &Path) -> Result<(), Errno> { > + remove_all(100, self.as_fd(), &path.as_cow_c_str().unwrap()) > + } > +} > + > +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)?; > + } > + 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::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::R= DONLY | 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(()) > +} > diff --git a/tools/cgroup-setup/src/main.rs b/tools/cgroup-setup/src/main= .rs > new file mode 100644 > index 0000000000000000000000000000000000000000..358703a73fcbdc38794312fe3= 987e733f37c2df0 > --- /dev/null > +++ b/tools/cgroup-setup/src/main.rs > @@ -0,0 +1,196 @@ > +// SPDX-License-Identifier: EUPL-1.2+ > +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour > + > +use std::{ > + ffi::{OsStr, OsString}, > + fs::File, > + io::Write as _, > + os::unix::prelude::*, > + path::{Component, Path, PathBuf}, > +}; > + > +use rustix::{ > + fs::{FlockOperation, Mode, OFlags, ResolveFlags}, > + io::Errno, > +}; > + > +mod cgroup; > + > +macro_rules! fail { > + ($($arg:tt)*) =3D> {{ > + eprintln!($($arg)*); > + std::process::exit(1)} > + }; > +} I'd find the error handling Rustier if we did error returns with the ? operator, like how mount-flatpak does it. The error type can just be String. > + > +fn main() { > + let mut args =3D std::env::args_os(); > + let Some(prog_name) =3D args.next() else { > + fail!("No command line arguments (argv[0] is NULL)"); > + }; > + let mut cgroup_relative_path =3D args.next(); > + if cgroup_relative_path.as_deref() =3D=3D Some(OsStr::from_bytes(b"-= -")) { > + cgroup_relative_path =3D args.next(); > + } We could just not support -- and simplify invocation, right? > + > + let Some(cgroup_relative_path) =3D cgroup_relative_path else { > + fail!("{prog_name:?}: Have no positional arguments, expect at le= ast 1") > + }; > + let mut cgroup_relative_path =3D cgroup_relative_path.into_vec(); > + > + // slow but we do not care Slow? > + if cgroup_relative_path.len() > 247 { This magic number could perhaps use a name. > + fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} too l= ong"); > + } > + if cgroup_relative_path.is_empty() { > + fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} empty= "); > + } > + if cgroup_relative_path[0] =3D=3D b'-' { > + fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} start= s with '-'"); > + } Is that a problem? > + for &i in &cgroup_relative_path { > + match i { > + b'/' =3D> fail!( > + "{prog_name:?}: Cgroup name {cgroup_relative_path:?} \ > +contains /" > + ), > + b'$' =3D> fail!( > + "{prog_name:?}: Cgroup name {cgroup_relative_path:?} \ > +contains $: did you forget an execline substitution?" > + ), > + b'!'..=3Db'~' =3D> {} > + _ =3D> fail!( > + "{prog_name:?}: Cgroup name {cgroup_relative_path:?} \ > +contains space, non-ASCII character, or control character (byte {i})" > + ), Surely every kernel interface we're going to use is 8-bit clean. If this is about log viewing again, I really don't like it. Even if we're careful about it in first-party code (which itself is a big ask), we can't expect it of every other package that might log something. People viewing logs have to use robust tools to do so regardless, or we have to mitigate this somewhere centrally, e.g. in s6 log. Doing this piecemeal does not meaningfully mitigate the problem, and arguably creates a false sense of security. > + } > + } > + cgroup_relative_path.extend_from_slice(b".service"); What do we need the suffix for? > + let cgroup_relative_path =3D Path::new(OsStr::from_bytes(&cgroup_rel= ative_path)); > + > + let local_cgroup =3D std::fs::read("/proc/thread-self/cgroup") Ooc, why /proc/thread-self instead of /proc/self? > + .unwrap_or_else(|e| fail!("{prog_name:?}: cannot read /proc/thre= ad-self/cgroup: {e}")); > + > + if !local_cgroup.starts_with(b"0::/") > + || !local_cgroup.ends_with(b"\n") > + || local_cgroup.contains(&b'\0') > + { > + fail!("{prog_name:?}: Kernel bug: Bad contents of /proc/thread-s= elf/cgroup"); > + } We are not a kernel fuzzer. We can't reasonably write programs that have to anticipate kernel contract violations. > + > + let mut local_cgroup =3D PathBuf::from(::fr= om_vec( > + local_cgroup[4..local_cgroup.len() - 1].to_owned(), =E2=80=A6 but maybe we could make this clearer, and still satisfy your inst= incts to validate, by using slice::strip_prefix and slice::strip_suffix, unwrapping the results? > + )); > + > + match local_cgroup.components().next_back() { > + Some(Component::Prefix(_)) =3D> unreachable!("does not occur on = Linux"), > + Some(Component::RootDir) | None =3D> {} > + Some(Component::CurDir) =3D> unreachable!("not produced by kerne= l"), > + Some(Component::ParentDir) =3D> unreachable!("not produced by ke= rnel"), > + Some(Component::Normal(os_str)) =3D> { > + if os_str.as_bytes() =3D=3D b"@inner.service" { > + let _ =3D local_cgroup.pop(); > + } > + } > + } Could we not simplify this with local_cgroup.file_name()? If it's None, it's the root directory; otherwise it's the equivalent of normal. > + > + if local_cgroup.as_os_str().is_empty() { > + local_cgroup =3D PathBuf::from("."); > + } Path::is_empty is stable in 1.98. Could we mention that in a TODO/FIXME comment so we can use it when available? > + > + let cgroup_root =3D rustix::fs::openat2( > + rustix::fs::CWD, > + Path::new("/sys/fs/cgroup"), > + OFlags::DIRECTORY | OFlags::RDONLY | OFlags::CLOEXEC | OFlags::N= OFOLLOW, > + Mode::empty(), > + ResolveFlags::NO_MAGICLINKS | ResolveFlags::NO_SYMLINKS, > + ) > + .unwrap_or_else(|e| fail!("{prog_name:?}: cannot open /sys/fs/cgroup= : {e}")); Why do we need to use openat2 for this, but not for reading /proc/thread-self/cgroup? I'm a bit surprised to see openat2 here at all, since only root could manipulate these paths, in which case they have no need to use this program as a confused deputy, right? If openat2 is to stay, isn't OFlags::NOFOLLOW redundant with ResolveFlags::NO_SYMLINKS? > + > + let cgroup =3D match cgroup::LeafCgroup::open_cgroup(cgroup_root.as_= fd(), &local_cgroup) { > + Ok(e) =3D> e, > + Err(e) =3D> fail!( > + "{prog_name:?}: Failed to open {}: {e:?}", > + local_cgroup.display() > + ), > + }; > + > + match rustix::fs::flock(cgroup.as_fd(), FlockOperation::LockExclusiv= e) { > + Ok(()) =3D> {} > + Err(e) =3D> fail!("{prog_name:?}: cannot lock cgroup: {e}"), > + } Would std::fs::File::lock not be a bit Rustier? > + purge_cgroup(&prog_name, &cgroup, cgroup_relative_path); I find it a little weird that we run this program in both service startup and finish, when there are really two completely separable parts to it, right? Why is purging not a separate program that only runs in finish? > + let Some(program_name) =3D args.next() else { > + return; > + }; This could be moved closer to where it's used. > + if let Err(e) =3D cgroup.make_child(cgroup_relative_path) { > + fail!("{prog_name:?}: cannot create child cgroup: {e}") > + } > + let child =3D cgroup > + .open_cgroup_at(cgroup_relative_path) > + .unwrap_or_else(|e| { > + fail!("{prog_name:?}: cannot create child cgroup {cgroup_rel= ative_path:?}: {e}") > + }); > + child > + .make_child(Path::new("@inner.service")) > + .unwrap_or_else(|e| { > + fail!("{prog_name:?}: cannot create child cgroup {cgroup_rel= ative_path:?}/@inner.service: {e}") > + }); > + let child =3D child > + .open_cgroup_at(Path::new("@inner.service")) > + .unwrap_or_else(|e| { > + fail!("{prog_name:?}: cannot open child cgroup {cgroup_relat= ive_path:?}/@inner.service: {e}") > + }); This would really benefit from having been written up, so I don't have to guess what it's for. We only really need it for supervisors, right? > + > + // SAFETY: safe FFI call > + let pid =3D unsafe { libc::getpid() }; Not std::process::id()? > + write_cgroup_value(&prog_name, &child, "cgroup.procs", &pid.to_strin= g()); > + > + let e =3D std::process::Command::new(&program_name).args(args).exec(= ); > + fail!( > + "{prog_name:?}: cannot spawn child {:?}: {}", > + program_name, > + e > + ); > +} > + > +fn write_cgroup_value(prog_name: &OsStr, child: &cgroup::LeafCgroup, nam= e: &str, value: &str) { Perhaps it would be nicer for this to be a method on LeafCgroup? > + let path =3D Path::new(name); > + let fd =3D rustix::fs::openat2( > + child.as_fd(), > + path, > + OFlags::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::W= RONLY, > + Mode::empty(), > + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags= ::NO_XDEV, > + ) > + .unwrap_or_else(|e| fail!("{prog_name:?}: cannot open {:?}: {}", pat= h, e)); > + let () =3D File::from(fd) > + .write_all(value.as_bytes()) > + .unwrap_or_else(|e| { > + fail!( > + "{prog_name:?}: Cannot write {:?} to {:?}: {}", > + name, > + path, > + e > + ) > + }); Similarly to above, if we don't need openat2, this could be done more nicely as std::fs::write. > +} > + > +fn purge_cgroup(prog_name: &OsStr, cgroup: &cgroup::LeafCgroup, cgroup_r= elative_path: &Path) { > + let child =3D match cgroup.open_cgroup_at(cgroup_relative_path) { > + Ok(child_cgroup) =3D> child_cgroup, > + Err(Errno::NOENT) =3D> return, > + Err(other) =3D> { > + fail!("{prog_name:?}: cannot open child cgroup {cgroup_relat= ive_path:?}: {other}") > + } > + }; > + child.kill_processes().unwrap_or_else(|e| { > + fail!("{prog_name:?}: cannot kill programs in {cgroup_relative_p= ath:?}: {e}") > + }); > + > + cgroup > + .delete_child(cgroup_relative_path) > + .unwrap_or_else(|e| { > + fail!("{prog_name:?}: delete child cgroup {cgroup_relative_p= ath:?}: {e}") > + }); > +} --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQRV/neXydHjZma5XLJbRZGEIw/wogUCajvKAgAKCRBbRZGEIw/w omBnAQDOaMq86AGceNfxB75zIQg56bVn3aN5K9Y9mrY/349dPgD/VoGckuz9OSFd 9/k2okkmtGvwG/cVWwvLUECGnyiHyQM= =A3pc -----END PGP SIGNATURE----- --=-=-=--