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 844BD9E70; Thu, 25 Jun 2026 09:49:26 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id DF9FE9E4A; Thu, 25 Jun 2026 09:49: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 fout-b6-smtp.messagingengine.com (fout-b6-smtp.messagingengine.com [202.12.124.149]) by atuin.qyliss.net (Postfix) with ESMTPS id AE6DB9E90 for ; Thu, 25 Jun 2026 09:49:21 +0000 (UTC) Received: from phl-compute-06.internal (phl-compute-06.internal [10.202.2.46]) by mailfout.stl.internal (Postfix) with ESMTP id 776BA1D00126; Thu, 25 Jun 2026 05:49:15 -0400 (EDT) Received: from phl-frontend-03 ([10.202.2.162]) by phl-compute-06.internal (MEProxy); Thu, 25 Jun 2026 05:49:15 -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=1782380955; x=1782467355; bh=us6vU7aSfv +k4K5mHUqcUBJqRKLT6DzgyX2x0DdmVzM=; b=s11nbWg7soIQh+DlSg5yJGS6WR tYhTqqXA3Cn4i8SQDx/OxGmXfHf/3rs2LDTJ3XBNlG0XL2Lpy2ZqPUaidhDFy988 YoBcF8d54Pv8HSH6iNVRvBf/l+M80IcACkZqHIYCj/5OA0mSxIxNMF29uG10Mvci a0LOJp+u5iaEorciYSXGYfsC/RayGZXIGmZtmmI9/kWmwsRVXCW+LEow6bbgLYXp VojUBgHkYKB5foi88PIjmCnWal7957RVSLEIOzqhdPzs/HIpwlKz3MpyH+tyfrKk jfv+dpeFpCJ94PIjnxzFEV9/VwQWg9JEblmr7RhzmDVA6NDbH1vup0Tki9qA== 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= 1782380955; x=1782467355; bh=us6vU7aSfv+k4K5mHUqcUBJqRKLT6DzgyX2 x0DdmVzM=; b=XIu0sGZK6aEBwxsnGWrHUXAPZsMx2BgFPi5ajiawJnQ0oGmbXoL LWCs1VZbFP7CoBov76sasVMmKUnSu6w9oaTfbUjogULTBFT7UTFRfOLhE5pVWd2k 3VMVmbXtO34dABB2Tvx/MkJA0paOXp9u0jNpMpWcZvhs3vyZHuyHBslWtvRD0uEn H4VSNyHyYSxXkKPE0MVXO8Z4kauNqYzZMN2/r/qvgAFbOobOI/VY2YdrLGaitM72 YKIOEPU4W58VQ4f1DQIn/0LNUkVPTkTvAj8XMCFRVmGLG2i9M3fWaiYZvrGzPzfV VJHYfPsipz9nHssTSxPCQK0i7ZiY6blO13A== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: dmFkZTGL7mDyhxYOxpnWo9q2UqaIWOc3rmTWZlRRbOlMguNDyv3G3pF4+6iRucc9FE+KUo IhMbHQOgNYxTLh+c/X3EDfYIl42oCngTaWH+pD3G0lekOhMORDKEhcoQMM/liiXHmtt9cP gbPdPy5YYzRnrYFM7263+QjL/RDLuuvtaUgPs744J9iPdkPOWBb2Rqdio9m4IpaXhEtTIC FOBRKl4YZqo/1TwEsH/ysUf3bK/VFNxaondq8cvbUSW0xz+mWBn60GIP7l+QiN584VGXz7 3DBoa5LOxitzmdzWfmQJlCr+uTRPnBkX/h01FK5roPrrY0FghLyZoxuNhSqb6pHdnx2NTU ZXjPFe3pEGQv1riaa9gav/QGbUNEu2etmSzNX3mg8jL/Fo9Vq6SXTbFjoHwEKD1U0wlzOO iQLKloAoEaaF53N1olmnO0yH3fP/3dxWQyb2Cd3ShX5NFXXkoeBDomW8X1+oSFILU609eZ al3Vfn+fa+iEgATx5RwciZI0pOTrBQtoCCj5zEX/HfBqLuhfJ98+iDAbMBT4SmV5Vn9/1v ivDTRh0JXIYm2MHw5ZN1o99kMX3rQMUfrrCSLlFBFE+D1n0U6vCcwE7r59lMsYJ+4i4sfx ePbja1JCHRQifXE9ld1p0MS+MjpD/OH4WRuP5Sd/ZzNVexUNWpWE8MYk3pQg X-ME-Proxy: Feedback-ID: i12284293:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 25 Jun 2026 05:49:14 -0400 (EDT) Received: by mbp.qyliss.net (Postfix, from userid 1000) id EED748369183; Thu, 25 Jun 2026 11:49:12 +0200 (CEST) From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH v2] Set up control groups for most services In-Reply-To: References: <20260620-cgroups-v1-1-0e5abf35101b@gmail.com> <20260620-cgroups-v2-1-ccae224b6c85@gmail.com> <87qzlw5e65.fsf@alyssa.is> Date: Thu, 25 Jun 2026 11:49:00 +0200 Message-ID: <87ldc354s3.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Message-ID-Hash: SUEZSSHRLWNLVYXGVCKAJC4CGTBT6T5M X-Message-ID-Hash: SUEZSSHRLWNLVYXGVCKAJC4CGTBT6T5M 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: > On 6/24/26 08:13, Alyssa Ross wrote: >> Demi Marie Obenour writes: >>=20 >>> 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. >>> >>=20 >> 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? > > Will fix in v3. > >> 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? > > I think it would be significantly simpler to have the VMM be > just another VM service. This would naturally put it under the > vm-services cgroup, solving this problem. Since this would mostly > involve changes to execline scripting, I think it would be better if > you wrote this code. No, I don't think it can work that way, because the VMM is in increasingly many cases not a service at all, since it's run-once. (Think about e.g. an appimage VM.) This is why it's not under vm-services today. (I thought we'd discussed this before, but it would have been months ago given how long ago we planned this=E2=80=A6) >>> diff --git a/host/rootfs/image/etc/s6-linux-init/run-image/service/seri= al-getty/run b/host/rootfs/image/etc/s6-linux-init/run-image/service/serial= -getty/run >>> index 78f794202bf174f3c036f3e20755ac087a988277..ed0808b9e45b077023f237a= 0f9c0e5c830015ac2 100755 >>> --- a/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-gett= y/run >>> +++ b/host/rootfs/image/etc/s6-linux-init/run-image/service/serial-gett= y/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 >>=20 >> We're putting supervisors of instances services in a cgroup? Can you >> explain to me why that's useful? > > This allows configuring limits that apply to the whole instance, and > means that the whole instance can be reliably killed. Services spawned > by the instance will be in their own sub-cgroups, so one can apply > separate limits to them so long as those limits do not exceed those > of the containing cgroup. > > The hierarchy works like this: > > /sys/fs/cgroup > service1.service > @inner.service # processes go here > service2.service > @inner.service # and here > service3.service > @inner.service # and here > > You can apply resource limits at any level of the hierarchy. Limits in > nested cgroups can be smaller but not larger. Terminating processes > in a cgroup with cgroup.kill also terminates all processes in child > cgroups. After terminating the processes in a cgroup, cgroup-setup > will delete everything in the hierarchy recursively. I just don't really understand when we'd want that. Why would we want to limit all instances of vm-services, or all instances of serial-getty? >>> diff --git a/tools/cgroup-setup/src/cgroup.rs b/tools/cgroup-setup/src/= cgroup.rs >>> new file mode 100644 >>> index 0000000000000000000000000000000000000000..cd77becc0b40bfdcd983e53= b63b7c21e4308d5fc >>> --- /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, >>=20 >> root_fd would be a clearer name, if I'm understanding correctly. > > It's the FD for the cgroup directory, but not for the root of the > cgroup tree. But if you open a child cgroup, it's the same FD, right? So it's a root of something. >>> +} >>> + >>> +enum Access { >>> + Read, >>> + Write, >>> +} >>=20 >> Given this isn't public, it doesn't seem to add any value over passing >> around OFlags::RDONLY and OFlags::WRONLY directly. > > Will change in v3. > >>> + >>> +impl LeafCgroup { >>> + fn open_subtree(&self, path: &std::path::Path, access: Access) -> = Result { >>> + 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 | Resolv= eFlags::NO_XDEV, >>> + ) >>> + } >>> + >>> + pub fn kill_processes(&self) -> std::io::Result<()> { >>> + let kill_fd =3D self.open_subtree(std::path::Path::new("cgroup= .kill"), Access::Write)?; >>> + let wait_file =3D self.open_subtree(std::path::Path::new("cgro= up.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, "kern= el 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"); >>> + } >>=20 >> Shouldn't we start polling before writing? Otherwise we might miss the >> notification, if it comes in between the write and the poll. > > I'll test this, but I think it shouldn't matter. If it does matter, I'll > need to switch to epoll (or io_uring, but that's overkill here). Not sure it's something you'll be able to reproduce in a test. We'd need to understand the reason it could never happen. >>> + 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" { >>=20 >> Could do this in one line, and avoid the labelled break: >>=20 >> if v.split(|&c| c =3D=3D b'\n').any(|line| line =3D=3D b"populated 0") { >>=20 >> (I think "line" is a clearer name than "substr".) > > Will fix in v3. > >>> + 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 | Resolv= eFlags::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 | Reso= lveFlags::NO_XDEV, >>=20 >> Stray comment. > > Will fix in v3. > >>> + 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 iterat= ing"); >>> + while let Some(element) =3D d.next() { >>> + let element =3D element.expect("Iterating through a cgroup dir= ectory 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:= :RDONLY | OFlags::DIRECTORY, >>> + Mode::empty(), >>> + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFla= gs::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/ma= in.rs >>> new file mode 100644 >>> index 0000000000000000000000000000000000000000..358703a73fcbdc38794312f= e3987e733f37c2df0 >>> --- /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)} >>> + }; >>> +} >>=20 >> 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. > > Sure! > >>> + >>> +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(); >>> + } >>=20 >> We could just not support -- and simplify invocation, right? > > Only if we never want to support any options in the future. I guess it's nice for out-of-tree users, and it's a trivial amount of code. >>> + >>> + let Some(cgroup_relative_path) =3D cgroup_relative_path else { >>> + fail!("{prog_name:?}: Have no positional arguments, expect at = least 1") >>> + }; >>> + let mut cgroup_relative_path =3D cgroup_relative_path.into_vec(); >>> + >>> + // slow but we do not care >>=20 >> Slow? > > O(n) for a size-n Vec. Not something you want in a hot path, but fine he= re. How is getting the length of a Vec O(n)? It stores it. >>> + if cgroup_relative_path.len() > 247 { >>=20 >> This magic number could perhaps use a name. > > Sure. > >>> + fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} too= long"); >>> + } >>> + if cgroup_relative_path.is_empty() { >>> + fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} emp= ty"); >>> + } >>> + if cgroup_relative_path[0] =3D=3D b'-' { >>> + fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} sta= rts with '-'"); >>> + } >>=20 >> Is that a problem? > > In theory, no, but it's probably a bug in the caller. > >>> + 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})" >>> + ), >>=20 >> 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. > > This was inspired by systemd, which has restrictions on what characters > are allowed in service names. Also, restricting allowed characters > makes it easier to find bugs, such as "${a} ${b}" instead of "${a}" > "${b}" in an execline script. > > Forbidding '$' is very much intentional, and is inspired by me making > the exact mistake described by the error message. It also means that > certain names can be reserved for internal use. In my opinion, one component should not be responsible for ad-hoc bug checks in another, but I won't push back too much. The other character limitations seem completely arbitrary to me though. >>> + } >>> + } >>> + cgroup_relative_path.extend_from_slice(b".service"); >>=20 >> What do we need the suffix for? > > The kernel may add new control files at any time. However, it will > never use the .service suffix to avoid breaking systemd. Therefore, I > chose to use that suffix to avoid breaking in future kernel releases. Makes sense. >>> + let cgroup_relative_path =3D Path::new(OsStr::from_bytes(&cgroup_r= elative_path)); >>> + >>> + let local_cgroup =3D std::fs::read("/proc/thread-self/cgroup") >>=20 >> Ooc, why /proc/thread-self instead of /proc/self? > > Why not? The two are aliases in this case, but in general /proc/thread-s= elf is likely the better choice. I'd just never seen it before. >>> + .unwrap_or_else(|e| fail!("{prog_name:?}: cannot read /proc/th= read-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= -self/cgroup"); >>> + } >>=20 >> We are not a kernel fuzzer. We can't reasonably write programs that >> have to anticipate kernel contract violations. > > Fair! > >>> + >>> + let mut local_cgroup =3D PathBuf::from(::= from_vec( >>> + local_cgroup[4..local_cgroup.len() - 1].to_owned(), >>=20 >> =E2=80=A6 but maybe we could make this clearer, and still satisfy your i= nstincts >> to validate, by using slice::strip_prefix and slice::strip_suffix, >> unwrapping the results? > > That's even better, because it's much easier to read than magic numbers. > >>> + )); >>> + >>> + match local_cgroup.components().next_back() { >>> + Some(Component::Prefix(_)) =3D> unreachable!("does not occur o= n Linux"), >>> + Some(Component::RootDir) | None =3D> {} >>> + Some(Component::CurDir) =3D> unreachable!("not produced by ker= nel"), >>> + Some(Component::ParentDir) =3D> unreachable!("not produced by = kernel"), >>> + Some(Component::Normal(os_str)) =3D> { >>> + if os_str.as_bytes() =3D=3D b"@inner.service" { >>> + let _ =3D local_cgroup.pop(); >>> + } >>> + } >>> + } >>=20 >> 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. > > Yup! > >>> + >>> + if local_cgroup.as_os_str().is_empty() { >>> + local_cgroup =3D PathBuf::from("."); >>> + } >>=20 >> Path::is_empty is stable in 1.98. Could we mention that in a TODO/FIXME >> comment so we can use it when available? > > Will do. > >>> + let cgroup_root =3D rustix::fs::openat2( >>> + rustix::fs::CWD, >>> + Path::new("/sys/fs/cgroup"), >>> + OFlags::DIRECTORY | OFlags::RDONLY | OFlags::CLOEXEC | OFlags:= :NOFOLLOW, >>> + Mode::empty(), >>> + ResolveFlags::NO_MAGICLINKS | ResolveFlags::NO_SYMLINKS, >>> + ) >>> + .unwrap_or_else(|e| fail!("{prog_name:?}: cannot open /sys/fs/cgro= up: {e}")); >>=20 >> 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? >>=20 >> If openat2 is to stay, isn't OFlags::NOFOLLOW redundant with >> ResolveFlags::NO_SYMLINKS? > > openat2() is indeed pointless here. Great =E2=80=94 I think we can really simplify the code if we don't use it = where unnecessary, because we can use a lot more of the standard library. Possibly Rustix isn't even needed? >>> + >>> + let cgroup =3D match cgroup::LeafCgroup::open_cgroup(cgroup_root.a= s_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::LockExclus= ive) { >>> + Ok(()) =3D> {} >>> + Err(e) =3D> fail!("{prog_name:?}: cannot lock cgroup: {e}"), >>> + } >>=20 >> Would std::fs::File::lock not be a bit Rustier? > > Yes :). I know how to=20 >=20=09 >>> + purge_cgroup(&prog_name, &cgroup, cgroup_relative_path); >>=20 >> 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? > > The finish script might fail, time out, etc. Purging at startup > ensures that any previous cgroup state (like attached BPF programs) > is removed, and it ensures that the program this program invokes will > not run concurrently. It's just more robust overall. > > Purging is idempotent, and purging an already-purged cgroup is very > cheap. Also, this allows using cgroup-setup without a finish script, > while still ensuring that if the service restarts there are no stale > processes left behind. Alright. >>> + let Some(program_name) =3D args.next() else { >>> + return; >>> + }; >>=20 >> This could be moved closer to where it's used. > > Will fix in v3. > >>> + 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_r= elative_path:?}: {e}") >>> + }); >>> + child >>> + .make_child(Path::new("@inner.service")) >>> + .unwrap_or_else(|e| { >>> + fail!("{prog_name:?}: cannot create child cgroup {cgroup_r= elative_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_rel= ative_path:?}/@inner.service: {e}") >>> + }); >>=20 >> 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? > > It's only *needed* when the program might be called recursively, > but it's easier to just use it everywhere. Yeah, that makes sense. Would just be nice to have that documented somewhere so the next person doesn't have to spend the time I did figuring it out! >>> + >>> + // SAFETY: safe FFI call >>> + let pid =3D unsafe { libc::getpid() }; >>=20 >> Not std::process::id()? > > Only because I was not aware of it. > >>> + write_cgroup_value(&prog_name, &child, "cgroup.procs", &pid.to_str= ing()); >>> + >>> + let e =3D std::process::Command::new(&program_name).args(args).exe= c(); >>> + fail!( >>> + "{prog_name:?}: cannot spawn child {:?}: {}", >>> + program_name, >>> + e >>> + ); >>> +} >>> + >>> +fn write_cgroup_value(prog_name: &OsStr, child: &cgroup::LeafCgroup, n= ame: &str, value: &str) { >>=20 >> Perhaps it would be nicer for this to be a method on LeafCgroup? > > Sure! > >>> + let path =3D Path::new(name); >>> + let fd =3D rustix::fs::openat2( >>> + child.as_fd(), >>> + path, >>> + OFlags::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags:= :WRONLY, >>> + Mode::empty(), >>> + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFla= gs::NO_XDEV, >>> + ) >>> + .unwrap_or_else(|e| fail!("{prog_name:?}: cannot open {:?}: {}", p= ath, e)); >>> + let () =3D File::from(fd) >>> + .write_all(value.as_bytes()) >>> + .unwrap_or_else(|e| { >>> + fail!( >>> + "{prog_name:?}: Cannot write {:?} to {:?}: {}", >>> + name, >>> + path, >>> + e >>> + ) >>> + }); >>=20 >> Similarly to above, if we don't need openat2, this could be done more >> nicely as std::fs::write. > > std::fs doesn't even expose openat(), and I'm much more comfortable > using a directory FD here. This is a fairly severe limitation in > the stdlib. Fixing it requires using the Native API on Windows, > but that's easy enough. We have a lock on the cgroup though. What are we protecting against by using openat? >>> +} >>> + >>> +fn purge_cgroup(prog_name: &OsStr, cgroup: &cgroup::LeafCgroup, cgroup= _relative_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_rel= ative_path:?}: {other}") >>> + } >>> + }; >>> + child.kill_processes().unwrap_or_else(|e| { >>> + fail!("{prog_name:?}: cannot kill programs in {cgroup_relative= _path:?}: {e}") >>> + }); >>> + >>> + cgroup >>> + .delete_child(cgroup_relative_path) >>> + .unwrap_or_else(|e| { >>> + fail!("{prog_name:?}: delete child cgroup {cgroup_relative= _path:?}: {e}") >>> + }); >>> +} --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQRV/neXydHjZma5XLJbRZGEIw/wogUCajz5jAAKCRBbRZGEIw/w oqJuAP9W3WS9y7FNZ/JpLH/U21RZcJJakLGbZ2PdgMlBRxoEkQEAkYEYeFIhYchg o+a1ZL/vyBPhkgDNfwut+vgIFn+X3ws= =RnTv -----END PGP SIGNATURE----- --=-=-=--