From 46e08207b58f0df9c5ab5d6cc481d1ba10a01e40 Mon Sep 17 00:00:00 2001 From: Rahat Mahmood Date: Tue, 30 Aug 2022 16:00:15 -0700 Subject: [PATCH] cgroupfs: Handle hierachy changes across charge/uncharge. When charging a pids cgroup during thread creation, it is possible for the hierachy containing the pids controller to be destroyed and recreated. If thread creation fails and the charge has to be rolled back, an intervening hierachy change previously caused a charge underflow during the rollback. If the hierachy changes between the charge and uncharge, the uncharge is uncessary. Reported-by: syzbot+b72cc8d190b428e43a03@syzkaller.appspotmail.com PiperOrigin-RevId: 471112484 --- pkg/sentry/fsimpl/cgroupfs/pids.go | 6 ++--- pkg/sentry/kernel/task_cgroup.go | 27 ++++++++++++++----- pkg/sentry/kernel/task_start.go | 22 ++++++++++----- test/syscalls/linux/cgroup.cc | 43 ++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 15 deletions(-) diff --git a/pkg/sentry/fsimpl/cgroupfs/pids.go b/pkg/sentry/fsimpl/cgroupfs/pids.go index 29aa491f8..1a920a73c 100644 --- a/pkg/sentry/fsimpl/cgroupfs/pids.go +++ b/pkg/sentry/fsimpl/cgroupfs/pids.go @@ -204,15 +204,15 @@ func (c *pidsController) Charge(t *kernel.Task, d *kernfs.Dentry, res kernel.Cgr // Negative charge. if value < 0 { if c.pendingTotal+value < 0 { - panic(fmt.Sprintf("cgroupfs: pids controller pending pool would be negative if charge was allowed: current pool: %d, proposed charge: %d", c.pendingTotal, value)) + panic(fmt.Sprintf("cgroupfs: pids controller pending pool would be negative if charge was allowed: current pool: %d, proposed charge: %d, path: %q, task: %p", c.pendingTotal, value, d.FSLocalPath(), t)) } pending, ok := c.pendingPool[t] if !ok { - panic(fmt.Sprintf("cgroupfs: pids controller attempted to remove pending charge for task %+v, but task didn't have pending charges", t)) + panic(fmt.Sprintf("cgroupfs: pids controller attempted to remove pending charge for Task %p, but task didn't have pending charges, path: %q", t, d.FSLocalPath())) } if pending+value < 0 { - panic(fmt.Sprintf("cgroupfs: pids controller attempted to remove pending charge for task %+v, but task didn't have enough pending charges; current charges: %d, proposed charge: %d", t, pending, value)) + panic(fmt.Sprintf("cgroupfs: pids controller attempted to remove pending charge for Task %p, but task didn't have enough pending charges; current charges: %d, proposed charge: %d, path: %q", t, pending, value, d.FSLocalPath())) } diff --git a/pkg/sentry/kernel/task_cgroup.go b/pkg/sentry/kernel/task_cgroup.go index d2747d944..85819a751 100644 --- a/pkg/sentry/kernel/task_cgroup.go +++ b/pkg/sentry/kernel/task_cgroup.go @@ -229,18 +229,33 @@ func (t *Task) GenerateProcTaskCgroup(buf *bytes.Buffer) { } // +checklocks:t.mu -func (t *Task) chargeLocked(target *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) error { +func (t *Task) chargeLocked(target *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, uint32, error) { + // Due to the uniqueness of controllers on hierarchies, at most one cgroup + // in t.cgroups will match. for c := range t.cgroups { - if err := c.Charge(target, c.Dentry, ctl, res, value); err != nil { - return err - } + err := c.Charge(target, c.Dentry, ctl, res, value) + return err == nil, c.HierarchyID(), err } - return nil + return false, InvalidCgroupHierarchyID, nil } // ChargeFor charges t's cgroup on behalf of some other task. -func (t *Task) ChargeFor(other *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) error { +func (t *Task) ChargeFor(other *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, uint32, error) { t.mu.Lock() defer t.mu.Unlock() return t.chargeLocked(other, ctl, res, value) } + +// ChargeForOnHierarchy is like ChargeFor, but only charges a cgroup with the +// matching hierarhcyID. This can be useful when reversing a charge across +// potential hierachy changes. +func (t *Task) ChargeForOnHierarchy(other *Task, hierarhcyID uint32, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, uint32, error) { + t.mu.Lock() + defer t.mu.Unlock() + for c := range t.cgroups { + if c.HierarchyID() == hierarhcyID { + return t.chargeLocked(other, ctl, res, value) + } + } + return false, InvalidCgroupHierarchyID, nil +} diff --git a/pkg/sentry/kernel/task_start.go b/pkg/sentry/kernel/task_start.go index b7497563b..16a6bc6e0 100644 --- a/pkg/sentry/kernel/task_start.go +++ b/pkg/sentry/kernel/task_start.go @@ -187,14 +187,24 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error) // we skip charging the pids controller, as non-userspace task creation // bypasses pid limits. if srcT != nil { - if err := srcT.ChargeFor(t, CgroupControllerPIDs, CgroupResourcePID, 1); err != nil { + var ( + charged bool + err error + hid uint32 + ) + if charged, hid, err = srcT.ChargeFor(t, CgroupControllerPIDs, CgroupResourcePID, 1); err != nil { return nil, err } - cu.Add(func() { - if err := srcT.ChargeFor(t, CgroupControllerPIDs, CgroupResourcePID, -1); err != nil { - panic(fmt.Sprintf("Failed to clean up PIDs charge on task creation failure: %v", err)) - } - }) + if charged { + cu.Add(func() { + // Since ts.mu was dropped after the corresponding charge, the + // hierarchy referenced by hid may no longer exist. If so, this + // uncharge will be a no-op. + if _, _, err := srcT.ChargeForOnHierarchy(t, hid, CgroupControllerPIDs, CgroupResourcePID, -1); err != nil { + panic(fmt.Sprintf("Failed to clean up PIDs charge on task creation failure: %v", err)) + } + }) + } } // Make the new task (and possibly thread group) visible to the rest of diff --git a/test/syscalls/linux/cgroup.cc b/test/syscalls/linux/cgroup.cc index b591ae4f8..ee2de49e9 100644 --- a/test/syscalls/linux/cgroup.cc +++ b/test/syscalls/linux/cgroup.cc @@ -1366,6 +1366,49 @@ TEST(PIDsCgroup, LimitEnforced) { IsPosixErrorOkAndHolds(baseline + 2)); } +// Regression test for b/231312320. +TEST(PIDsCgroup, RaceFSDestructionChargeUncharge) { + SKIP_IF(!CgroupsAvailable()); + + const TempPath mountpoint = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); + + // The goal of this test is to recreate the hierarchy containing the pids + // controller between when a pending charge is added for a new thread, and + // when the pending charge is removed due to a clone failure. + + // Attempt to change the hierachy mid charge/uncharge by repeatedly creating + // new cgroupfs filesystems. + ScopedThread mounter_unmounter([&mountpoint] { + const DisableSave ds; // Too many syscalls. + + for (int i = 0; i < 1000; ++i) { + mount("none", mountpoint.path().c_str(), "cgroup", 0, 0); + umount(mountpoint.path().c_str()); + } + }); + + // Trigger thread creation failure by having the parent of a clone syscall + // segfault intentionally. + ScopedThread cloner_root([] { + const DisableSave ds; // Too many syscalls. + + for (int i = 0; i < 50; ++i) { + ScopedThread cloner([] { + // Asynchronously spawn threads. + ScopedThread noop_threads([] { + for (int i = 0; i < 100; ++i) { + ScopedThread([] { getpid(); }); + } + }); + + // Intentionally bad syscall that will cause the calling thread to be + // aborted with a SIGSEGV. + rename(0, 0); + }); + } + }); +} + } // namespace } // namespace testing } // namespace gvisor