Fix cgroupfs mount racing with unmount.

Previously, mount could discover a hierarchy being destroyed
concurrently, which resulted in mount attempting to take a ref on an
already destroyed cgroupfs.

Reported-by: syzbot+062c0a67798a200f23ee@syzkaller.appspotmail.com
PiperOrigin-RevId: 374959054

2 files changed, 60 insertions, 4 deletions

diff --git a/pkg/sentry/kernel/cgroup.go b/pkg/sentry/kernel/cgroup.go
index 0fbf27f64..c93ef6ac1 100644
--- a/pkg/sentry/kernel/cgroup.go
+++ b/pkg/sentry/kernel/cgroup.go
@@ -181,7 +181,23 @@ func (r *CgroupRegistry) FindHierarchy(ctypes []CgroupControllerType) *vfs.Files
 
 	for _, h := range r.hierarchies {
 		if h.match(ctypes) {
-			h.fs.IncRef()
+			if !h.fs.TryIncRef() {
+				// Racing with filesystem destruction, namely h.fs.Release.
+				// Since we hold r.mu, we know the hierarchy hasn't been
+				// unregistered yet, but its associated filesystem is tearing
+				// down.
+				//
+				// If we simply indicate the hierarchy wasn't found without
+				// cleaning up the registry, the caller can race with the
+				// unregister and find itself temporarily unable to create a new
+				// hierarchy with a subset of the relevant controllers.
+				//
+				// To keep the result of FindHierarchy consistent with the
+				// uniqueness of controllers enforced by Register, drop the
+				// dying hierarchy now. The eventual unregister by the FS
+				// teardown will become a no-op.
+				return nil
+			}
 			return h.fs
 		}
 	}
@@ -230,12 +246,17 @@ func (r *CgroupRegistry) Register(cs []CgroupController, fs cgroupFS) error {
 	return nil
 }
 
-// Unregister removes a previously registered hierarchy from the registry. If
-// the controller was not previously registered, Unregister is a no-op.
+// Unregister removes a previously registered hierarchy from the registry. If no
+// such hierarchy is registered, Unregister is a no-op.
 func (r *CgroupRegistry) Unregister(hid uint32) {
 	r.mu.Lock()
-	defer r.mu.Unlock()
+	r.unregisterLocked(hid)
+	r.mu.Unlock()
+}
 
+// Precondition: Caller must hold r.mu.
+// +checklocks:r.mu
+func (r *CgroupRegistry) unregisterLocked(hid uint32) {
 	if h, ok := r.hierarchies[hid]; ok {
 		for name, _ := range h.controllers {
 			delete(r.controllers, name)
diff --git a/test/syscalls/linux/cgroup.cc b/test/syscalls/linux/cgroup.cc
index a009ade7e..f29891571 100644
--- a/test/syscalls/linux/cgroup.cc
+++ b/test/syscalls/linux/cgroup.cc
@@ -227,6 +227,41 @@ TEST(Cgroup, MountRace) {
   EXPECT_NO_ERRNO(c.ContainsCallingProcess());
 }
 
+TEST(Cgroup, MountUnmountRace) {
+  SKIP_IF(!CgroupsAvailable());
+
+  TempPath mountpoint = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+
+  const DisableSave ds;  // Too many syscalls.
+
+  auto mount_thread = [&mountpoint]() {
+    for (int i = 0; i < 100; ++i) {
+      mount("none", mountpoint.path().c_str(), "cgroup", 0, 0);
+    }
+  };
+  auto unmount_thread = [&mountpoint]() {
+    for (int i = 0; i < 100; ++i) {
+      umount(mountpoint.path().c_str());
+    }
+  };
+  std::list<ScopedThread> threads;
+  for (int i = 0; i < 10; ++i) {
+    threads.emplace_back(mount_thread);
+  }
+  for (int i = 0; i < 10; ++i) {
+    threads.emplace_back(unmount_thread);
+  }
+  for (auto& t : threads) {
+    t.Join();
+  }
+
+  // We don't know how many mount refs are remaining, since the count depends on
+  // the ordering of mount and umount calls. Keep calling unmount until it
+  // returns an error.
+  while (umount(mountpoint.path().c_str()) == 0) {
+  }
+}
+
 TEST(Cgroup, UnmountRepeated) {
   SKIP_IF(!CgroupsAvailable());