Re: [PATCH v2] Set up control groups for most services

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 21991 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> 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..d92364d83f4d26f766fcd5b494614c06a630d2fe 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=755	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/service/serial-getty-generator/run
> index bf66ab0878fc6d8e77ca71a4a89e50c139a6bb11..5c9988c994ed9b942329055673b332d0aa918957 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 <hi@alyssa.is>
>  
> +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-getty/run
> index 78f794202bf174f3c036f3e20755ac087a988277..ed0808b9e45b077023f237a0f9c0e5c830015ac2 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 <hi@alyssa.is>
>  
> +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-services/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..89676ac91d3e22db7726fc869a065137b031d0be 100755
> --- a/host/rootfs/image/etc/s6-linux-init/run-image/service/vm-services/template/data/service/dbus/run
> +++ b/host/rootfs/image/etc/s6-linux-init/run-image/service/vm-services/template/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 <hi@alyssa.is>
>  
> +cgroup-setup -- $1
>  importas -i VM VM
>  
>  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..9cce0be706151dfd5264a81afb3ee9a6c851367c
> --- /dev/null
> +++ b/tools/cgroup-setup/Cargo.toml
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: CC0-1.0
> +# SPDX-FileCopyrightText: 2025 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2026 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +[package]
> +name = "cgroup-setup"
> +edition = "2024"
> +
> +[dependencies]
> +libc = "0.2.177"
> +rustix = { version = "1.1.2", features = ["fs"] }
> +
> +[profile.release]
> +split-debuginfo = "symbols"
> +strip = "symbols"
> +lto = true
> +panic = "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/cgroup.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..cd77becc0b40bfdcd983e53b63b7c21e4308d5fc
> --- /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 <demiobenour@gmail.com>
> +
> +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) -> Result<OwnedFd, Errno> {
> +        rustix::fs::openat2(
> +            self.fd.as_fd(),
> +            path,
> +            OFlags::NOATIME
> +                | OFlags::CLOEXEC
> +                | OFlags::NOFOLLOW
> +                | match access {
> +                    Access::Write => OFlags::WRONLY,
> +                    Access::Read => OFlags::RDONLY,
> +                },
> +            Mode::empty(),
> +            ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
> +        )
> +    }
> +
> +    pub fn kill_processes(&self) -> std::io::Result<()> {
> +        let kill_fd = self.open_subtree(std::path::Path::new("cgroup.kill"), Access::Write)?;
> +        let wait_file = self.open_subtree(std::path::Path::new("cgroup.events"), Access::Read)?;
> +        let poll_fd = wait_file.as_raw_fd();
> +        let mut wait_fd = File::from(wait_file);
> +        assert_eq!(rustix::io::write(kill_fd.as_fd(), b"1")?, 1, "kernel bug");
> +        let mut fds = 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 = vec![];
> +        'a: loop {
> +            v.clear();
> +            if unsafe { libc::poll(&raw mut fds, 1, -1) } != 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 == b'\n') {
> +                if substr == b"populated 0" {

Could do this in one line, and avoid the labelled break:

if v.split(|&c| c == b'\n').any(|line| line == 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<Self, Errno> {
> +        let fd = 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 | ResolveFlags::NO_XDEV,
> +        )?;
> +        Ok(Self {
> +            fd,
> +            path: self.path.join(p),
> +        })
> +    }
> +    pub(crate) fn open_cgroup(fd: BorrowedFd, p: &Path) -> Result<Self, Errno> {
> +        let fd = rustix::fs::openat2(
> +            fd,
> +            p,
> +            OFlags::NOATIME
> +                | OFlags::CLOEXEC
> +                | OFlags::NOFOLLOW
> +                | OFlags::RDONLY
> +                | OFlags::DIRECTORY,
> +            Mode::empty(),
> +            //ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::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 = rustix::fs::Dir::new(fd).expect("cannot start iterating");
> +    while let Some(element) = d.next() {
> +        let element = element.expect("Iterating through a cgroup directory failed?");
> +        if element.file_type() != rustix::fs::FileType::Directory {
> +            continue;
> +        }
> +
> +        let remaining_depth = remaining_depth - 1;
> +        let d: &rustix::fs::Dir = &d;
> +        let dirfd = d.fd().unwrap();
> +        let path = element.file_name();
> +        remove_all(remaining_depth, dirfd, path)?;
> +    }
> +    Ok(())
> +}
> +
> +fn remove_all(
> +    remaining_depth: usize,
> +    dirfd: BorrowedFd<'_>,
> +    path: &std::ffi::CStr,
> +) -> Result<(), Errno> {
> +    if path == c"." || path == c".." {
> +        return Ok(());
> +    }
> +    if rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR).is_ok() {
> +        return Ok(());
> +    }
> +    let fd = rustix::fs::openat2(
> +        dirfd,
> +        path,
> +        OFlags::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY | OFlags::DIRECTORY,
> +        Mode::empty(),
> +        ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
> +    )?;
> +    remove_recursively(fd, remaining_depth)?;
> +    rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR)?;
> +    Ok(())
> +}
> diff --git a/tools/cgroup-setup/src/main.rs b/tools/cgroup-setup/src/main.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..358703a73fcbdc38794312fe3987e733f37c2df0
> --- /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 <demiobenour@gmail.com>
> +
> +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)*) => {{
> +        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 = std::env::args_os();
> +    let Some(prog_name) = args.next() else {
> +        fail!("No command line arguments (argv[0] is NULL)");
> +    };
> +    let mut cgroup_relative_path = args.next();
> +    if cgroup_relative_path.as_deref() == Some(OsStr::from_bytes(b"--")) {
> +        cgroup_relative_path = args.next();
> +    }

We could just not support -- and simplify invocation, right?

> +
> +    let Some(cgroup_relative_path) = cgroup_relative_path else {
> +        fail!("{prog_name:?}: Have no positional arguments, expect at least 1")
> +    };
> +    let mut cgroup_relative_path = 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 long");
> +    }
> +    if cgroup_relative_path.is_empty() {
> +        fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} empty");
> +    }
> +    if cgroup_relative_path[0] == b'-' {
> +        fail!("{prog_name:?}: Cgroup name {cgroup_relative_path:?} starts with '-'");
> +    }

Is that a problem?

> +    for &i in &cgroup_relative_path {
> +        match i {
> +            b'/' => fail!(
> +                "{prog_name:?}: Cgroup name {cgroup_relative_path:?} \
> +contains /"
> +            ),
> +            b'$' => fail!(
> +                "{prog_name:?}: Cgroup name {cgroup_relative_path:?} \
> +contains $: did you forget an execline substitution?"
> +            ),
> +            b'!'..=b'~' => {}
> +            _ => 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 = Path::new(OsStr::from_bytes(&cgroup_relative_path));
> +
> +    let local_cgroup = 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/thread-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");
> +    }

We are not a kernel fuzzer.  We can't reasonably write programs that
have to anticipate kernel contract violations.

> +
> +    let mut local_cgroup = PathBuf::from(<OsString as OsStringExt>::from_vec(
> +        local_cgroup[4..local_cgroup.len() - 1].to_owned(),

… but maybe we could make this clearer, and still satisfy your instincts
to validate, by using slice::strip_prefix and slice::strip_suffix,
unwrapping the results?

> +    ));
> +
> +    match local_cgroup.components().next_back() {
> +        Some(Component::Prefix(_)) => unreachable!("does not occur on Linux"),
> +        Some(Component::RootDir) | None => {}
> +        Some(Component::CurDir) => unreachable!("not produced by kernel"),
> +        Some(Component::ParentDir) => unreachable!("not produced by kernel"),
> +        Some(Component::Normal(os_str)) => {
> +            if os_str.as_bytes() == b"@inner.service" {
> +                let _ = 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 = 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 = 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/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 = match cgroup::LeafCgroup::open_cgroup(cgroup_root.as_fd(), &local_cgroup) {
> +        Ok(e) => e,
> +        Err(e) => fail!(
> +            "{prog_name:?}: Failed to open {}: {e:?}",
> +            local_cgroup.display()
> +        ),
> +    };
> +
> +    match rustix::fs::flock(cgroup.as_fd(), FlockOperation::LockExclusive) {
> +        Ok(()) => {}
> +        Err(e) => 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) = args.next() else {
> +        return;
> +    };

This could be moved closer to where it's used.

> +    if let Err(e) = cgroup.make_child(cgroup_relative_path) {
> +        fail!("{prog_name:?}: cannot create child cgroup: {e}")
> +    }
> +    let child = cgroup
> +        .open_cgroup_at(cgroup_relative_path)
> +        .unwrap_or_else(|e| {
> +            fail!("{prog_name:?}: cannot create child cgroup {cgroup_relative_path:?}: {e}")
> +        });
> +    child
> +        .make_child(Path::new("@inner.service"))
> +        .unwrap_or_else(|e| {
> +            fail!("{prog_name:?}: cannot create child cgroup {cgroup_relative_path:?}/@inner.service: {e}")
> +        });
> +    let child = child
> +        .open_cgroup_at(Path::new("@inner.service"))
> +        .unwrap_or_else(|e| {
> +            fail!("{prog_name:?}: cannot open child cgroup {cgroup_relative_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 = unsafe { libc::getpid() };

Not std::process::id()?

> +    write_cgroup_value(&prog_name, &child, "cgroup.procs", &pid.to_string());
> +
> +    let e = 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, name: &str, value: &str) {

Perhaps it would be nicer for this to be a method on LeafCgroup?

> +    let path = Path::new(name);
> +    let fd = rustix::fs::openat2(
> +        child.as_fd(),
> +        path,
> +        OFlags::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::WRONLY,
> +        Mode::empty(),
> +        ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFlags::NO_XDEV,
> +    )
> +    .unwrap_or_else(|e| fail!("{prog_name:?}: cannot open {:?}: {}", path, e));
> +    let () = 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_relative_path: &Path) {
> +    let child = match cgroup.open_cgroup_at(cgroup_relative_path) {
> +        Ok(child_cgroup) => child_cgroup,
> +        Err(Errno::NOENT) => return,
> +        Err(other) => {
> +            fail!("{prog_name:?}: cannot open child cgroup {cgroup_relative_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}")
> +        });
> +}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]