From f01bf248c19ffd7e0ea085d499a0b3b7404c6953 Mon Sep 17 00:00:00 2001 From: Konstantin Bogomolov Date: Mon, 13 Mar 2023 11:38:05 -0700 Subject: [PATCH] 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 | 4 +++ pkg/sentry/platform/systrap/BUILD | 12 +++---- pkg/sentry/platform/systrap/subprocess.go | 36 +++++++------------ .../platform/systrap/subprocess_pool.go | 32 +++-------------- pkg/sentry/platform/systrap/systrap.go | 2 ++ 5 files changed, 30 insertions(+), 56 deletions(-) diff --git a/pkg/sentry/kernel/task_run.go b/pkg/sentry/kernel/task_run.go index 1decfb756..109149c0c 100644 --- a/pkg/sentry/kernel/task_run.go +++ b/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. diff --git a/pkg/sentry/platform/systrap/BUILD b/pkg/sentry/platform/systrap/BUILD index 00ceb2605..946beeeb6 100644 --- a/pkg/sentry/platform/systrap/BUILD +++ b/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", diff --git a/pkg/sentry/platform/systrap/subprocess.go b/pkg/sentry/platform/systrap/subprocess.go index cd6a3dbd7..e596bac43 100644 --- a/pkg/sentry/platform/systrap/subprocess.go +++ b/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) } diff --git a/pkg/sentry/platform/systrap/subprocess_pool.go b/pkg/sentry/platform/systrap/subprocess_pool.go index 274f2f469..3a7a24086 100644 --- a/pkg/sentry/platform/systrap/subprocess_pool.go +++ b/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 } diff --git a/pkg/sentry/platform/systrap/systrap.go b/pkg/sentry/platform/systrap/systrap.go index 085608833..4fa37ec89 100644 --- a/pkg/sentry/platform/systrap/systrap.go +++ b/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 })