From 47b5915a7b31922d83473156f4679a8891ee98e6 Mon Sep 17 00:00:00 2001
From: Rahat Mahmood <rahat@google.com>
Date: Thu, 28 Apr 2022 13:42:29 -0700
Subject: [PATCH] Break Task.mu -> kernfs.Filesystem.mu lock chain when
 managing cgroups.

This led to circular locking since procfs aquires Task.mu while
holding kernfs.Filesystem.mu. The procfs case is harder to break, as
procfs needs to acquire an mm reference during a filesystem operation.

PiperOrigin-RevId: 445237505
---
 pkg/sentry/fsimpl/kernfs/kernfs.go |  2 ++
 pkg/sentry/kernel/cgroup.go        |  2 ++
 pkg/sentry/kernel/kernel.go        | 16 +++++++++++++++-
 pkg/sentry/kernel/task_cgroup.go   | 18 ++++++++----------
 4 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/pkg/sentry/fsimpl/kernfs/kernfs.go b/pkg/sentry/fsimpl/kernfs/kernfs.go
index 163fa1459..c1b6c7248 100644
--- a/pkg/sentry/fsimpl/kernfs/kernfs.go
+++ b/pkg/sentry/fsimpl/kernfs/kernfs.go
@@ -48,6 +48,8 @@
 // Lock ordering:
 //
 // kernfs.Filesystem.mu
+//   kernel.TaskSet.mu
+//     kernel.Task.mu
 //   kernfs.Dentry.dirMu
 //     vfs.VirtualFilesystem.mountMu
 //       vfs.Dentry.mu
diff --git a/pkg/sentry/kernel/cgroup.go b/pkg/sentry/kernel/cgroup.go
index ec6035e63..70880f708 100644
--- a/pkg/sentry/kernel/cgroup.go
+++ b/pkg/sentry/kernel/cgroup.go
@@ -91,6 +91,8 @@ type Cgroup struct {
 	CgroupImpl
 }
 
+// decRef drops a reference on the cgroup. This must happen outside a Task.mu
+// critical section.
 func (c *Cgroup) decRef() {
 	c.Dentry.DecRef(context.Background())
 }
diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go
index dda7f46ca..5420e1584 100644
--- a/pkg/sentry/kernel/kernel.go
+++ b/pkg/sentry/kernel/kernel.go
@@ -1871,7 +1871,11 @@ func (k *Kernel) PopulateNewCgroupHierarchy(root Cgroup) {
 // hierarchy with the provided id.  This is intended for use during hierarchy
 // teardown, as otherwise the tasks would be orphaned w.r.t to some controllers.
 func (k *Kernel) ReleaseCgroupHierarchy(hid uint32) {
+	var releasedCGs []Cgroup
+
 	k.tasks.mu.RLock()
+	// We'll have one cgroup per hierarchy per task.
+	releasedCGs = make([]Cgroup, 0, len(k.tasks.Root.tids))
 	k.tasks.forEachTaskLocked(func(t *Task) {
 		if t.exitState != TaskExitNone {
 			return
@@ -1879,12 +1883,22 @@ func (k *Kernel) ReleaseCgroupHierarchy(hid uint32) {
 		t.mu.Lock()
 		for cg := range t.cgroups {
 			if cg.HierarchyID() == hid {
-				t.leaveCgroupLocked(cg)
+				cg.Leave(t)
+				delete(t.cgroups, cg)
+				releasedCGs = append(releasedCGs, cg)
+				// A task can't be part of multiple cgroups from the same
+				// hierarchy, so we can skip checking the rest once we find a
+				// match.
+				break
 			}
 		}
 		t.mu.Unlock()
 	})
 	k.tasks.mu.RUnlock()
+
+	for _, c := range releasedCGs {
+		c.decRef()
+	}
 }
 
 func (k *Kernel) ReplaceFSContextRoots(ctx context.Context, oldRoot vfs.VirtualDentry, newRoot vfs.VirtualDentry) {
diff --git a/pkg/sentry/kernel/task_cgroup.go b/pkg/sentry/kernel/task_cgroup.go
index 10c975dd4..efdad19f6 100644
--- a/pkg/sentry/kernel/task_cgroup.go
+++ b/pkg/sentry/kernel/task_cgroup.go
@@ -90,17 +90,15 @@ func (t *Task) enterCgroupIfNotYetLocked(c Cgroup) {
 // LeaveCgroups removes t out from all its cgroups.
 func (t *Task) LeaveCgroups() {
 	t.mu.Lock()
-	defer t.mu.Unlock()
-	for c, _ := range t.cgroups {
-		t.leaveCgroupLocked(c)
+	cgs := t.cgroups
+	t.cgroups = nil
+	for c := range cgs {
+		c.Leave(t)
+	}
+	t.mu.Unlock()
+	for c := range cgs {
+		c.decRef()
 	}
-}
-
-// +checklocks:t.mu
-func (t *Task) leaveCgroupLocked(c Cgroup) {
-	c.Leave(t)
-	delete(t.cgroups, c)
-	c.decRef()
 }
 
 // +checklocks:t.mu