cgroupfs: Fix several races with task migration.

Reported-by: syzbot+0be09ce607731f085f73@syzkaller.appspotmail.com
PiperOrigin-RevId: 491920581

pkg/sentry/fsimpl/cgroupfs/pids.go

@@ -161,14 +161,23 @@ func (c *pidsController) Leave(t *kernel.Task) {
 
 // PrepareMigrate implements controller.PrepareMigrate.
 func (c *pidsController) PrepareMigrate(t *kernel.Task, src controller) error {
+	srcC := src.(*pidsController)
+	srcC.mu.Lock()
+	defer srcC.mu.Unlock()
+
+	if _, ok := srcC.pendingPool[t]; ok {
+		// Migrating task isn't fully initialized, return transient failure.
+		return linuxerr.EAGAIN
+	}
+
 	return nil
 }
 
 // CommitMigrate implements controller.CommitMigrate.
 //
-// Migrations can cause a cgroup to exceed its limit. Migration can only be
-// called for tasks with committed charges, as it is not possible to migrate a
-// task prior to Enter.
+// Migrations can cause a cgroup to exceed its limit. CommitMigrate can only be
+// called for tasks with committed charges, PrepareMigrate will deny migrations
+// prior to Enter.
 func (c *pidsController) CommitMigrate(t *kernel.Task, src controller) {
 	// Note: The charge is allowed to exceed max on migration. The charge may
 	// not exceed max when incurred due to a fork/clone, which will call

pkg/sentry/kernel/task_cgroup.go

@@ -89,6 +89,7 @@ func (t *Task) enterCgroupIfNotYetLocked(c Cgroup) {
 
 // LeaveCgroups removes t out from all its cgroups.
 func (t *Task) LeaveCgroups() {
+	t.tg.pidns.owner.mu.Lock() // Prevent migration.
 	t.mu.Lock()
 	cgs := t.cgroups
 	t.cgroups = nil
@@ -96,6 +97,8 @@ func (t *Task) LeaveCgroups() {
 		c.Leave(t)
 	}
 	t.mu.Unlock()
+	t.tg.pidns.owner.mu.Unlock()
+
 	for c := range cgs {
 		c.decRef()
 	}
@@ -229,33 +232,24 @@ func (t *Task) GenerateProcTaskCgroup(buf *bytes.Buffer) {
 }
 
 // +checklocks:t.mu
-func (t *Task) chargeLocked(target *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, uint32, error) {
+func (t *Task) chargeLocked(target *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, Cgroup, error) {
 	// Due to the uniqueness of controllers on hierarchies, at most one cgroup
 	// in t.cgroups will match.
 	for c := range t.cgroups {
 		err := c.Charge(target, c.Dentry, ctl, res, value)
-		return err == nil, c.HierarchyID(), err
+		if err == nil {
+			c.IncRef()
+		}
+		return err == nil, c, err
 	}
-	return false, InvalidCgroupHierarchyID, nil
+	return false, Cgroup{}, nil
 }
 
-// ChargeFor charges t's cgroup on behalf of some other task.
-func (t *Task) ChargeFor(other *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, uint32, error) {
+// ChargeFor charges t's cgroup on behalf of some other task. Returns
+// the cgroup that's charged if any. Returned cgroup has an extra ref
+// that's transferred to the caller.
+func (t *Task) ChargeFor(other *Task, ctl CgroupControllerType, res CgroupResourceType, value int64) (bool, Cgroup, 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

@@ -176,7 +176,11 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error)
 	// We don't construct t.blockingTimer until Task.run(); see that function
 	// for justification.
 
-	var cu cleanup.Cleanup
+	var (
+		cg      Cgroup
+		charged bool
+		cu      cleanup.Cleanup
+	)
 	defer cu.Clean()
 
 	// Reserve cgroup PIDs controller charge. This is either commited when the
@@ -187,22 +191,16 @@ 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 {
-		var (
-			charged bool
-			err     error
-			hid     uint32
-		)
-		if charged, hid, err = srcT.ChargeFor(t, CgroupControllerPIDs, CgroupResourcePID, 1); err != nil {
+		var err error
+		if charged, cg, err = srcT.ChargeFor(t, CgroupControllerPIDs, CgroupResourcePID, 1); err != nil {
 			return nil, 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 {
+				if err := cg.Charge(t, cg.Dentry, CgroupControllerPIDs, CgroupResourcePID, -1); err != nil {
 					panic(fmt.Sprintf("Failed to clean up PIDs charge on task creation failure: %v", err))
 				}
+				cg.DecRef(ctx) // Ref from ChargeFor.
 			})
 		}
 	}
@@ -241,6 +239,11 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error)
 	// srcT may be nil, in which case we default to root cgroups.
 	t.EnterInitialCgroups(srcT)
 
+	cu.Release()
+	if charged {
+		cg.decRef() // Ref from ChargeFor.
+	}
+
 	if tg.leader == nil {
 		// New thread group.
 		tg.leader = t
@@ -272,7 +275,6 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error)
 	// other pieces to be initialized as the task is used the context.
 	t.p = cfg.Kernel.Platform.NewContext(t.AsyncContext())
 
-	cu.Release()
 	return t, nil
 }