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