amadan

carlos / platform Public

zdd plan 2 final fix: bound the cleanup walk while it reads

Members() read a whole directory before checking the group budget, so
a tenant's many empty sub-cgroups cost unbounded allocation and work.
The listing is now read in batches of 64 and every child group is
counted as it is read; past the budget the walk refuses before reading
or opening more. Width-bound tests: one group over the bound at one
level is refused before any child is opened, exactly at the bound is
read.

Final review finding D1 (and triage T2 width-cap test).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 792eb01a0d59ee0b22f206c5c4336680e9b69f10 parent b1914ba
2 files changed, +81 −18
  • internal/cgroup/cleanup_linux.go +43 −18
  • internal/cgroup/cleanup_linux_test.go +38 −0
diff --git a/internal/cgroup/cleanup_linux.go b/internal/cgroup/cleanup_linux.go
index fbef3fd17..f11b99e96 100644
--- a/internal/cgroup/cleanup_linux.go
+++ b/internal/cgroup/cleanup_linux.go
@@ -7,6 +7,7 @@ import (
"context"
"errors"
"fmt"
+ "io"
"os"
"slices"
"strconv"
@@ -201,30 +202,19 @@ func readProcs(f *os.File) ([]int, error) {
return pids, scan.Err()
}
+// cleanupReadBatch is how many directory entries one read takes, so the
+// group budget is checked as the listing is read, not after all of it.
+const cleanupReadBatch = 64
+
// walkMembers appends the members of every cgroup below dir. A child that
// vanishes between the listing and its open was removed, so it is empty.
func walkMembers(dir *os.File, depth int, groups *int, members *[]int) error {
- fd, err := unix.Openat(int(dir.Fd()), ".", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+ names, err := childGroups(dir, depth, groups)
if err != nil {
return err
}
- list := os.NewFile(uintptr(fd), dir.Name())
- entries, err := list.ReadDir(-1)
- list.Close()
- if err != nil {
- return err
- }
- for _, e := range entries {
- if !e.IsDir() {
- continue
- }
- if depth > maxCleanupDepth {
- return fmt.Errorf("%w: cgroup tree deeper than %d", ErrStrictRefused, maxCleanupDepth)
- }
- if *groups++; *groups > maxCleanupGroups {
- return fmt.Errorf("%w: more than %d cgroups below one host", ErrStrictRefused, maxCleanupGroups)
- }
- child, err := strictOpen(dir, e.Name(), unix.O_RDONLY|unix.O_DIRECTORY)
+ for _, name := range names {
+ child, err := strictOpen(dir, name, unix.O_RDONLY|unix.O_DIRECTORY)
if errors.Is(err, unix.ENOENT) {
continue
}
@@ -254,6 +244,41 @@ func walkMembers(dir *os.File, depth int, groups *int, members *[]int) error {
return nil
}
+// childGroups lists the child cgroups of dir in bounded batches, counting
+// each against the walk's budget as it is read. A tree past the depth or
+// group bound is refused before any more of it is read, so a tenant's many
+// empty cgroups cost at most the budget.
+func childGroups(dir *os.File, depth int, groups *int) ([]string, error) {
+ fd, err := unix.Openat(int(dir.Fd()), ".", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+ if err != nil {
+ return nil, err
+ }
+ list := os.NewFile(uintptr(fd), dir.Name())
+ defer list.Close()
+ var names []string
+ for {
+ entries, err := list.ReadDir(cleanupReadBatch)
+ for _, e := range entries {
+ if !e.IsDir() {
+ continue
+ }
+ if depth > maxCleanupDepth {
+ return nil, fmt.Errorf("%w: cgroup tree deeper than %d", ErrStrictRefused, maxCleanupDepth)
+ }
+ if *groups++; *groups > maxCleanupGroups {
+ return nil, fmt.Errorf("%w: more than %d cgroups below one host", ErrStrictRefused, maxCleanupGroups)
+ }
+ names = append(names, e.Name())
+ }
+ if errors.Is(err, io.EOF) {
+ return names, nil
+ }
+ if err != nil {
+ return nil, err
+ }
+ }
+}
+
func (c *Cleanup) KillAndEmpty(ctx context.Context) error {
if c == nil || c.state == nil {
return ErrStrictState
diff --git a/internal/cgroup/cleanup_linux_test.go b/internal/cgroup/cleanup_linux_test.go
index 429b7d02f..b3c00601d 100644
--- a/internal/cgroup/cleanup_linux_test.go
+++ b/internal/cgroup/cleanup_linux_test.go
@@ -242,6 +242,44 @@ func TestCleanupMembersRefuseATreeTooDeep(t *testing.T) {
}
}
+// The group budget is a width bound too, and it is enforced while the
+// directory is read: a tree one group too wide is refused before any child
+// is opened. Every child here is unreadable (mode 0), so a walk that
+// opened one before counting them all would fail with EACCES instead.
+func TestCleanupMembersRefuseATreeTooWide(t *testing.T) {
+ if os.Geteuid() == 0 {
+ t.Skip("root opens a mode-0 directory; the probe needs an ordinary user")
+ }
+ host, root := "wide.example", t.TempDir()
+ dir := filepath.Join(root, ExecName(host))
+ mkCgroupFixture(t, dir, "", "populated 1\n")
+ for i := range maxCleanupGroups + 1 {
+ if err := os.Mkdir(filepath.Join(dir, "w"+strconv.Itoa(i)), 0); err != nil {
+ t.Fatal(err)
+ }
+ }
+ c := fixtureCleanupAt(t, root, host, "")
+ if _, err := c.Members(); !errors.Is(err, ErrStrictRefused) {
+ t.Fatalf("%d groups at one level: %v, want ErrStrictRefused before any child is opened", maxCleanupGroups+1, err)
+ }
+}
+
+func TestCleanupMembersAcceptATreeExactlyAtTheBound(t *testing.T) {
+ host, root := "bound.example", t.TempDir()
+ dir := filepath.Join(root, ExecName(host))
+ mkCgroupFixture(t, dir, "", "populated 1\n")
+ for i := range maxCleanupGroups {
+ if err := os.Mkdir(filepath.Join(dir, "w"+strconv.Itoa(i)), 0o700); err != nil {
+ t.Fatal(err)
+ }
+ }
+ mkCgroupFixture(t, filepath.Join(dir, "w7"), "77\n", "populated 1\n")
+ c := fixtureCleanupAt(t, root, host, "")
+ if members, err := c.Members(); err != nil || !reflect.DeepEqual(members, []int{77}) {
+ t.Fatalf("%d groups: %v, %v; want [77]", maxCleanupGroups, members, err)
+ }
+}
+
// KillAndEmpty on a generation acts only on that generation's child.
func TestCleanupChildKillsOnlyItsGeneration(t *testing.T) {
host, root := "kill.example", t.TempDir()