Fix data race in subprocess pool.

This is a relatively benign data race that's possible when two of the last
remaining contexts are calling s.unregisterContext at the same time, and both
decrement s.numContexts. One continues to release() the subprocess which puts it
into the available pool, but the other one has yet to read s.released. Then,
since the subprocess is in the available pool it can be retrieved with
fetchAvailable, which did not lock s.mu, which is where the data race happens,
because fetchAvailable flips s.released but one of the exiting contexts has yet
to read it.

The proper fix for this is to start holding references onto the subprocess. We
cannot reuse the subprocess while any systrap.contexts are still holding onto
sysmsg.ThreadContexts, so each context get's a reference. Each subprocess is
held by a MemoryManager as an AddressSpace interface, which calls Release on the
subprocess once.

This CL also removes subprocessPool.active because it is unnecessary at this
moment.

PiperOrigin-RevId: 516272892

pkg/sentry/kernel/task_run.go

@@ -23,6 +23,7 @@ import (
 	"gvisor.dev/gvisor/pkg/errors/linuxerr"
 	"gvisor.dev/gvisor/pkg/goid"
 	"gvisor.dev/gvisor/pkg/hostarch"
+	"gvisor.dev/gvisor/pkg/refs"
 	"gvisor.dev/gvisor/pkg/sentry/hostcpu"
 	ktime "gvisor.dev/gvisor/pkg/sentry/kernel/time"
 	"gvisor.dev/gvisor/pkg/sentry/memmap"
@@ -58,6 +59,9 @@ type taskRunState interface {
 func (t *Task) run(threadID uintptr) {
 	t.goid.Store(goid.Get())
 
+	refs.CleanupSync.Add(1)
+	defer refs.CleanupSync.Done()
+
 	// Construct t.blockingTimer here. We do this here because we can't
 	// reconstruct t.blockingTimer during restore in Task.afterLoad(), because
 	// kernel.timekeeper.SetClocks() hasn't been called yet.

pkg/sentry/platform/systrap/BUILD

@@ -7,14 +7,13 @@ package(
 )
 
 go_template_instance(
-    name = "subprocess_list",
-    out = "subprocess_list.go",
+    name = "subprocess_refs",
+    out = "subprocess_refs.go",
     package = "systrap",
     prefix = "subprocess",
-    template = "//pkg/ilist:generic_list",
+    template = "//pkg/refs:refs_template",
     types = {
-        "Linker": "*subprocess",
-        "Element": "*subprocess",
+        "T": "subprocess",
     },
 )
 
@@ -39,8 +38,8 @@ go_library(
         "subprocess_arm64_unsafe.go",
         "subprocess_linux.go",
         "subprocess_linux_unsafe.go",
-        "subprocess_list.go",
         "subprocess_pool.go",
+        "subprocess_refs.go",
         "subprocess_unsafe.go",
         "syscall_thread.go",
         "syscall_thread_amd64.go",
@@ -67,6 +66,7 @@ go_library(
         "//pkg/log",
         "//pkg/memutil",
         "//pkg/pool",
+        "//pkg/refs",
         "//pkg/safecopy",
         "//pkg/seccomp",
         "//pkg/sentry/arch",

pkg/sentry/platform/systrap/subprocess.go

@@ -22,7 +22,6 @@ import (
 
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/abi/linux"
-	"gvisor.dev/gvisor/pkg/atomicbitops"
 	"gvisor.dev/gvisor/pkg/hostarch"
 	"gvisor.dev/gvisor/pkg/log"
 	"gvisor.dev/gvisor/pkg/pool"
@@ -115,24 +114,14 @@ const (
 // subprocess is a collection of threads being traced.
 type subprocess struct {
 	platform.NoAddressSpaceIO
-	subprocessEntry
+	subprocessRefs
 
 	// requests is used to signal creation of new threads.
 	requests chan any
 
-	// numContexts counts the number of contexts currently active within the
-	// subprocess. A subprocess should not be fully released to be reused until
-	// numContexts reaches 0.
-	numContexts atomicbitops.Int32
-
 	// mu protects the following fields.
 	mu sync.Mutex
 
-	// released marks this subprocess as having been released.
-	// A subprocess can be both released and active because we cannot allow it to
-	// reused until all tied contexts have been unregistered.
-	released bool
-
 	// faultedContexts is the set of contexts for which it's possible that
 	// context.lastFaultSP == this subprocess.
 	faultedContexts map[*context]struct{}
@@ -253,6 +242,8 @@ func (s *subprocess) handlePtraceSyscallRequest(req any) {
 // to happen with the runtime thread locked.
 func newSubprocess(create func() (*thread, error), memoryFile *pgalloc.MemoryFile) (*subprocess, error) {
 	if sp := globalPool.fetchAvailable(); sp != nil {
+		sp.subprocessRefs.InitRefs()
+		sp.usertrap = usertrap.New()
 		return sp, nil
 	}
 
@@ -271,6 +262,7 @@ func newSubprocess(create func() (*thread, error), memoryFile *pgalloc.MemoryFil
 		memoryFile:        memoryFile,
 		sysmsgThreads:     make(map[uint32]*sysmsgThread),
 	}
+	sp.subprocessRefs.InitRefs()
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
@@ -299,7 +291,6 @@ func newSubprocess(create func() (*thread, error), memoryFile *pgalloc.MemoryFil
 	sp.usertrap = usertrap.New()
 	sp.mapSharedRegions()
 
-	globalPool.add(sp)
 	return sp, nil
 }
 
@@ -372,10 +363,13 @@ func (s *subprocess) unmap() {
 // globalPool. This has the added benefit of reducing creation time for new
 // subprocesses.
 func (s *subprocess) Release() {
-	go func() { // S/R-SAFE: Platform.
-		s.unmap()
-		globalPool.release(s)
-	}()
+	s.unmap()
+	s.DecRef(s.release)
+}
+
+// release returns the subprocess to the global pool.
+func (s *subprocess) release() {
+	globalPool.markAvailable(s)
 }
 
 // newThread creates a new traced thread.
@@ -963,7 +957,7 @@ func (s *subprocess) registerContext(c *context) error {
 	if !ok {
 		return fmt.Errorf("subprocess has too many active threads (%d); failed to create a new one", maxGuestContexts)
 	}
-	s.numContexts.Add(1)
+	s.IncRef()
 	c.cid = id
 	c.subprocess = s
 	unlock()
@@ -989,11 +983,7 @@ func (s *subprocess) unregisterContext(c *context) {
 	s.mu.Lock()
 	delete(s.faultedContexts, c)
 	s.threadContextPool.Put(cid)
-	s.numContexts.Add(-1)
-	released := s.released
 	s.mu.Unlock()
 
-	if released && s.numContexts.Load() == 0 {
-		globalPool.release(s)
-	}
+	s.DecRef(s.release)
 }

pkg/sentry/platform/systrap/subprocess_pool.go

@@ -16,8 +16,6 @@ package systrap
 
 import (
 	"sync"
-
-	"gvisor.dev/gvisor/pkg/sentry/platform/systrap/usertrap"
 )
 
 // subprocessPool exists to solve these distinct problems:
@@ -30,49 +28,29 @@ import (
 // 2) Any seccomp filters that have been installed will apply to subprocesses
 // created here. Therefore we use the intermediary (source), which is created
 // on initialization of the platform.
-//
-// 3) Contexts are used in potentially many subprocesses, and upon
-// context.Release their resources need to be cleaned up from each subprocess.
 type subprocessPool struct {
 	mu     sync.Mutex
 	source *subprocess
 	// available stores all subprocesses that are available for reuse.
 	// +checklocks:mu
 	available []*subprocess
-	// active stores all subprocesses that are currently active.
-	// +checklocks:mu
-	active subprocessList
 }
 
-func (p *subprocessPool) add(s *subprocess) {
-	p.mu.Lock()
-	p.active.PushBack(s)
-	p.mu.Unlock()
-}
-
-func (p *subprocessPool) release(s *subprocess) {
+func (p *subprocessPool) markAvailable(s *subprocess) {
 	p.mu.Lock()
 	defer p.mu.Unlock()
-	s.mu.Lock()
-	defer s.mu.Unlock()
 
-	s.released = true
-	if s.numContexts.Load() == 0 {
-		p.active.Remove(s)
-		p.available = append(p.available, s)
-	}
+	p.available = append(p.available, s)
 }
 
 func (p *subprocessPool) fetchAvailable() *subprocess {
 	p.mu.Lock()
 	defer p.mu.Unlock()
 	if len(p.available) > 0 {
-		sp := p.available[len(p.available)-1]
+		s := p.available[len(p.available)-1]
 		p.available = p.available[:len(p.available)-1]
-		p.active.PushBack(sp)
-		sp.usertrap = usertrap.New()
-		sp.released = false
-		return sp
+
+		return s
 	}
 	return nil
 }

pkg/sentry/platform/systrap/systrap.go

@@ -369,6 +369,8 @@ func New() (*Systrap, error) {
 			// Should never happen.
 			panic("unable to initialize systrap source: " + err.Error())
 		}
+		// The source subprocess is never released explicitly by a MM.
+		source.DecRef(nil)
 
 		globalPool.source = source
 	})