Make unexpected subprocess kills thread-safe.

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

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

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)

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

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
 	}