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 })