From 1a850d5cc7ee102c41be6aead266db88cf419606 Mon Sep 17 00:00:00 2001 From: Konstantin Bogomolov Date: Thu, 3 Sep 2026 11:24:25 -0700 Subject: [PATCH] Make unexpected subprocess kills thread-safe. To kill the subprocess, we kill the syscall thread, which in turn kills all stub threads by virtue of being their parent. This means we have two critical sections that need to be protected: 1. Creating new sysmsg threads -- the function expects that it's new baby will stay alive at least while it's being created; 2. Using the syscall thread itself. We also kick out any currently sleeping task goroutines, and put guard-checks on wait loops and possible entrances to subprocess functionality so that tasks don't come back to a dead subprocess. Additionally, killed subprocesses were not properly decref'ed before, this CL also fixes that. PiperOrigin-RevId: 975832029 --- pkg/sentry/platform/systrap/shared_context.go | 24 +++-- .../platform/systrap/shared_context_test.go | 81 ++++++++++++++++ pkg/sentry/platform/systrap/subprocess.go | 92 +++++++++++++++---- .../platform/systrap/subprocess_unsafe.go | 14 +++ pkg/sentry/platform/systrap/systrap.go | 8 +- 5 files changed, 190 insertions(+), 29 deletions(-) diff --git a/pkg/sentry/platform/systrap/shared_context.go b/pkg/sentry/platform/systrap/shared_context.go index 7f3e82fc540..de33c5290a0 100644 --- a/pkg/sentry/platform/systrap/shared_context.go +++ b/pkg/sentry/platform/systrap/shared_context.go @@ -85,6 +85,9 @@ const ( func (s *subprocess) getSharedContext() (*sharedContext, error) { s.mu.Lock() defer s.mu.Unlock() + if s.dead.Load() { + return nil, errDeadSubprocess + } id, ok := s.threadContextPool.Get() if !ok { @@ -122,6 +125,9 @@ func (sc *sharedContext) isActiveInSubprocess(s *subprocess) bool { } func (sc *sharedContext) interruptStub() (*thread, error) { + if sc.subprocess.dead.Load() { + return nil, errDeadSubprocess + } // If this context is not being worked on right now we need to mark it as // interrupted so the next executor does not start working on it. atomic.StoreUint32(&sc.shared.Interrupt, 1) @@ -155,14 +161,7 @@ func (sc *sharedContext) interruptStub() (*thread, error) { // killSubprocess marks the subprocess dead and kills its syscall thread. func (sc *sharedContext) killSubprocess() { - s := sc.subprocess - s.dead.Store(true) - if !sc.shared.State.CompareAndSwap(sysmsg.ContextStateNone, sysmsg.ContextStateUnexpectedDeath) { - s.syscallThread.thread.Warningf("failed to set context state to ContextStateUnexpectedDeath; context state was %v", sc.state()) - } - s.syscallThreadMu.Lock() - defer s.syscallThreadMu.Unlock() - s.syscallThread.thread.kill() + sc.subprocess.kill() } // NotifyInterrupt implements interrupt.Receiver.NotifyInterrupt. @@ -254,7 +253,11 @@ const ( ) var ( - errDeadSubprocess = fmt.Errorf("subprocess died") + errDeadSubprocess = fmt.Errorf("subprocess died") + errDeadSubprocessContext = &platform.ContextError{ + Err: errDeadSubprocess, + Errno: unix.ECHILD, + } errNoStubThread = fmt.Errorf("no stub thread to interrupt") errStubThreadGone = fmt.Errorf("stub thread does not exist") errStuckContext = fmt.Errorf("systrap context is stuck") @@ -280,6 +283,9 @@ func (sc *sharedContext) sleepOnStateWithTimeout(state sysmsg.ContextState, stuc interruptsSent := 0 deadline := time.Now().Add(stuckTimeout) for sc.state() == state { + if sc.subprocess.dead.Load() { + return errDeadSubprocess + } errno := sc.shared.SleepOnState(state, &timeout) if errno == 0 { continue diff --git a/pkg/sentry/platform/systrap/shared_context_test.go b/pkg/sentry/platform/systrap/shared_context_test.go index bb2fcf11c5c..d4b8523aa81 100644 --- a/pkg/sentry/platform/systrap/shared_context_test.go +++ b/pkg/sentry/platform/systrap/shared_context_test.go @@ -22,6 +22,7 @@ import ( "testing" "time" + "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/sentry/platform/systrap/sysmsg" ) @@ -116,6 +117,86 @@ func TestSleepOnStateRecoveredContext(t *testing.T) { } } +func TestSleepOnStateDeadSubprocess(t *testing.T) { + sc := newTestSharedContext(t) + sc.subprocess.dead.Store(true) + + err := sc.sleepOnState(sysmsg.ContextStateNone) + if !errors.Is(err, errDeadSubprocess) { + t.Fatalf("sleepOnState got error %v, want %v", err, errDeadSubprocess) + } +} + +func TestWaitOnStateDeadSubprocess(t *testing.T) { + sc := newTestSharedContext(t) + sc.subprocess.dead.Store(true) + + err := sc.subprocess.waitOnState(sc) + if !errors.Is(err, errDeadSubprocess) { + t.Fatalf("waitOnState got error %v, want %v", err, errDeadSubprocess) + } +} + +func TestKickSysmsgThreadDeadSubprocess(t *testing.T) { + sc := newTestSharedContext(t) + sc.subprocess.dead.Store(true) + + if sc.subprocess.kickSysmsgThread() { + t.Fatalf("kickSysmsgThread got true, want false when subprocess is dead") + } +} + +func TestWithAliveRLockDeadSubprocess(t *testing.T) { + s := &subprocess{} + s.dead.Store(true) + + called := false + err := s.withAliveRLock(func() error { + called = true + return nil + }) + if !errors.Is(err, errDeadSubprocess) { + t.Fatalf("withAliveRLock got error %v, want %v", err, errDeadSubprocess) + } + if called { + t.Fatalf("withAliveRLock executed callback when dead") + } +} + +func TestSyscallDeadSubprocess(t *testing.T) { + s := &subprocess{} + s.dead.Store(true) + + if _, err := s.syscall(unix.SYS_MMAP); !errors.Is(err, errDeadSubprocess) { + t.Fatalf("syscall got error %v, want %v", err, errDeadSubprocess) + } +} + +func TestCreateSysmsgThreadDeadSubprocess(t *testing.T) { + s := &subprocess{} + s.dead.Store(true) + + if err := s.createSysmsgThread(); !errors.Is(err, errDeadSubprocess) { + t.Fatalf("createSysmsgThread got error %v, want %v", err, errDeadSubprocess) + } +} + +func TestReleaseDeadSubprocessDecRefs(t *testing.T) { + sc := newTestSharedContext(t) + s := sc.subprocess + s.subprocessRefs.InitRefs() + s.dead.Store(true) + + released := false + // Set ref count to 1 and verify DecRef fires. + s.DecRef(func() { + released = true + }) + if !released { + t.Fatalf("expected subprocess to be released") + } +} + func TestStuckSubprocessHelper(t *testing.T) { if os.Getenv("GVISOR_STUCK_SUBPROCESS_HELPER") == "" { return diff --git a/pkg/sentry/platform/systrap/subprocess.go b/pkg/sentry/platform/systrap/subprocess.go index 4de8f9a83d6..d4e89f7f171 100644 --- a/pkg/sentry/platform/systrap/subprocess.go +++ b/pkg/sentry/platform/systrap/subprocess.go @@ -180,6 +180,9 @@ type subprocess struct { // user mode. contextQueue *contextQueue + // aliveMu synchronizes active subprocess operations with termination. + aliveMu sync.RWMutex + // dead indicates whether the subprocess is alive or not. dead atomicbitops.Bool } @@ -499,31 +502,63 @@ func (s *subprocess) unmap() { } } -// Release kills the subprocess. -// -// Just kidding! We can't safely coordinate the detaching of all the -// tracees (since the tracers are random runtime threads, and the process -// won't exit until tracers have been notifier). -// -// Therefore we simply unmap everything in the subprocess and return it to the -// globalPool. This has the added benefit of reducing creation time for new -// subprocesses. +// Release makes the subprocess available for reuse, or cleans it up if it died +// an unexpected death. func (s *subprocess) Release() { - if !s.alive() { - return + if s.alive() { + s.unmap() } - s.unmap() s.DecRef(s.release) } -// release returns the subprocess to the global pool. func (s *subprocess) release() { if s.alive() { globalPool.markAvailable(s) return } - if s.syscallThread != nil && s.syscallThread.seccompNotify != nil { - s.syscallThread.seccompNotify.Close() + if s.syscallThread != nil { + if s.syscallThread.seccompNotify != nil { + s.syscallThread.seccompNotify.Close() + } + wstatus := unix.WaitStatus(0) + unix.Wait4(int(s.syscallThread.thread.tid), &wstatus, unix.WNOHANG, nil) + } +} + +// withAliveRLock executes fn while holding aliveMu.RLock(), ensuring the subprocess is alive. +func (s *subprocess) withAliveRLock(fn func() error) error { + if s.dead.Load() { + return errDeadSubprocess + } + s.aliveMu.RLock() + defer s.aliveMu.RUnlock() + if s.dead.Load() { + return errDeadSubprocess + } + return fn() +} + +// kill marks the subprocess dead, terminates the stub process, and unblocks +// all contexts waiting in waitOnState or sleepOnState. +// +// This is only done on expected events that indicate we can't proceed using this +// subprocess (e.g. stub threads unexpectedly die during execution). Well-behaved +// subprocesses do no call this. +func (s *subprocess) kill() { + if !s.dead.CompareAndSwap(false, true) { + return + } + + // Broadcast to sleeping task goroutines that it's time to go. + s.wakeAllContexts() + + // Ensure in-flight createSysmsgThread and syscalls (MapFile/Unmap) + // complete before killing. + s.aliveMu.Lock() + defer s.aliveMu.Unlock() + + if s.syscallThread != nil && s.syscallThread.thread != nil { + s.syscallThread.thread.kill() } } @@ -843,6 +878,9 @@ func (s *subprocess) switchToApp(c *platformContext, ac *arch.Context64) (isSysc } if err := s.waitOnState(ctx); err != nil { + if errors.Is(err, errDeadSubprocess) { + return false, false, hostarch.NoAccess, errDeadSubprocessContext + } return false, false, hostarch.NoAccess, corruptedSharedMemoryErr(err.Error()) } @@ -895,6 +933,9 @@ func (s *subprocess) switchToApp(c *platformContext, ac *arch.Context64) (isSysc } func (s *subprocess) waitOnState(ctx *sharedContext) error { + if s.dead.Load() { + return errDeadSubprocess + } ctx.kicked = false slowPath := false if !s.contextQueue.fastPathEnabled() || atomic.LoadUint32(&s.contextQueue.numActiveThreads) == 0 { @@ -944,6 +985,9 @@ func (s *subprocess) waitOnState(ctx *sharedContext) error { // The second return value is the expected number of threads after kicking a // new one. func (s *subprocess) canKickSysmsgThread() (bool, uint32) { + if s.dead.Load() { + return false, 0 + } // numActiveContexts and numActiveThreads can be changed from stub // threads that handles the contextQueue without any locks. The idea // here is that any stub thread that gets CPU time can make some @@ -1000,10 +1044,15 @@ func (s *subprocess) kickSysmsgThread() bool { // syscall executes the given system call without handling interruptions. func (s *subprocess) syscall(sysno uintptr, args ...arch.SyscallArgument) (uintptr, error) { - s.syscallThreadMu.Lock() - defer s.syscallThreadMu.Unlock() - - return s.syscallThread.syscall(sysno, args...) + var ret uintptr + err := s.withAliveRLock(func() error { + s.syscallThreadMu.Lock() + defer s.syscallThreadMu.Unlock() + r, err := s.syscallThread.syscall(sysno, args...) + ret = r + return err + }) + return ret, err } // MapFile implements platform.AddressSpace.MapFile. @@ -1068,6 +1117,11 @@ func initSysmsgThreadPriority() { // createSysmsgThread creates a new sysmsg thread. // The thread starts processing any available context in the context queue. func (s *subprocess) createSysmsgThread() error { + return s.withAliveRLock(s.createSysmsgThreadLocked) +} + +// +checklocksread:s.aliveMu +func (s *subprocess) createSysmsgThreadLocked() error { // Create a new seccomp process. var r requestThread r.thread = make(chan *thread) diff --git a/pkg/sentry/platform/systrap/subprocess_unsafe.go b/pkg/sentry/platform/systrap/subprocess_unsafe.go index bdc0b067c69..aa91013941b 100644 --- a/pkg/sentry/platform/systrap/subprocess_unsafe.go +++ b/pkg/sentry/platform/systrap/subprocess_unsafe.go @@ -131,3 +131,17 @@ func (s *subprocess) alive() bool { s.dead.Store(true) return false } + +// wakeAllContexts transitions all active context slots in ContextStateNone to +// ContextStateUnexpectedDeath and wakes their futex. +func (s *subprocess) wakeAllContexts() { + if s.threadContextRegion == 0 { + return + } + for i := uint64(0); i < maxGuestContexts; i++ { + tc := s.getThreadContextFromID(i) + if tc.State.CompareAndSwap(sysmsg.ContextStateNone, sysmsg.ContextStateUnexpectedDeath) { + futexWakeUint32((*uint32)(unsafe.Pointer(&tc.State))) + } + } +} diff --git a/pkg/sentry/platform/systrap/systrap.go b/pkg/sentry/platform/systrap/systrap.go index 0b77e711819..da92ce9b536 100644 --- a/pkg/sentry/platform/systrap/systrap.go +++ b/pkg/sentry/platform/systrap/systrap.go @@ -43,7 +43,10 @@ // // subprocessPool.mu // subprocess.mu -// platformContext.mu +// +// subprocess.aliveMu +// subprocess.syscallThreadMu +// subprocess.sysmsgThreadsMu // // +checkalignedignore package systrap @@ -173,6 +176,9 @@ func (c *platformContext) FullStateChanged() { func (c *platformContext) Switch(ctx pkgcontext.Context, mm platform.MemoryManager, ac *arch.Context64, cpu int32) (*linux.SignalInfo, hostarch.AccessType, error) { as := mm.AddressSpace() s := as.(*subprocess) + if s.dead.Load() { + return nil, hostarch.NoAccess, errDeadSubprocessContext + } if err := s.activateContext(c); err != nil { return nil, hostarch.NoAccess, err }