From d0ae59368d8a6c4e668301f0229d6329db52be18 Mon Sep 17 00:00:00 2001
From: Rahat Mahmood <rahat@google.com>
Date: Fri, 2 Dec 2022 16:43:44 -0800
Subject: [PATCH] cgroupfs: Fix lock ordering between kernfs.Filesystem.mu and
 TaskSet.mu.

We can't DecRef a cgroup with TaskSet.mu held as it leads to circular
locking. Restructure task creation to drop cgroup refs outside the
TaskSet.mu critical section.

Reported-by: syzbot+16a334ab1d6873db18f2@syzkaller.appspotmail.com
Reported-by: syzbot+fe1b962d430d1170e671@syzkaller.appspotmail.com
PiperOrigin-RevId: 492589548
---
 pkg/sentry/kernel/task_start.go | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/pkg/sentry/kernel/task_start.go b/pkg/sentry/kernel/task_start.go
index e4d88f13c..f8a525bcb 100644
--- a/pkg/sentry/kernel/task_start.go
+++ b/pkg/sentry/kernel/task_start.go
@@ -19,7 +19,6 @@ import (
 
 	"gvisor.dev/gvisor/pkg/abi/linux"
 	"gvisor.dev/gvisor/pkg/atomicbitops"
-	"gvisor.dev/gvisor/pkg/cleanup"
 	"gvisor.dev/gvisor/pkg/context"
 	"gvisor.dev/gvisor/pkg/errors/linuxerr"
 	"gvisor.dev/gvisor/pkg/hostarch"
@@ -177,11 +176,9 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error)
 	// for justification.
 
 	var (
-		cg      Cgroup
-		charged bool
-		cu      cleanup.Cleanup
+		cg                 Cgroup
+		charged, committed bool
 	)
-	defer cu.Clean()
 
 	// Reserve cgroup PIDs controller charge. This is either commited when the
 	// new task enters the cgroup below, or rolled back on failure.
@@ -196,12 +193,16 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error)
 			return nil, err
 		}
 		if charged {
-			cu.Add(func() {
-				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))
+			defer func() {
+				if !committed {
+					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.
-			})
+				// Ref from ChargeFor. Note that we need to drop this outside of
+				// TaskSet.mu critical sections.
+				cg.DecRef(ctx)
+			}()
 		}
 	}
 
@@ -238,11 +239,7 @@ 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.
-	}
+	committed = true
 
 	if tg.leader == nil {
 		// New thread group.