diff --git a/pkg/sentry/platform/systrap/shared_context.go b/pkg/sentry/platform/systrap/shared_context.go index 7f3e82fc54..de33c5290a 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 bb2fcf11c5..d4b8523aa8 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 4de8f9a83d..d4e89f7f17 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 bdc0b067c6..aa91013941 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 0b77e71181..da92ce9b53 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 }