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

@@ -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()))
 
 		}
 

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
+}

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

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