kernel: add ThreadGroup.signalLock()

This allows "remote" locking of ThreadGroup.signalHandlers.mu without needing
to lock TaskSet.mu, analogously to Linux's lock_task_sighand().

This reveals a bug: kernel.Task.sendSignal[Timer]Locked() unintentionally
requires TaskSet.mu to be locked since it reads Task.exitState. To fix this,
use atomic memory operations on Task.exitState when required.

PiperOrigin-RevId: 681128063

pkg/sentry/kernel/BUILD

@@ -304,6 +304,7 @@ go_library(
         "taskset_mutex.go",
         "thread_group.go",
         "thread_group_timer_mutex.go",
+        "thread_group_unsafe.go",
         "threads.go",
         "threads_impl.go",
         "timekeeper.go",

pkg/sentry/kernel/kernel.go

@@ -2018,7 +2018,7 @@ func (k *Kernel) Release() {
 func (k *Kernel) PopulateNewCgroupHierarchy(root Cgroup) {
 	k.tasks.mu.RLock()
 	k.tasks.forEachTaskLocked(func(t *Task) {
-		if t.exitState != TaskExitNone {
+		if t.exitStateLocked() != TaskExitNone {
 			return
 		}
 		t.mu.Lock()
@@ -2040,7 +2040,7 @@ func (k *Kernel) ReleaseCgroupHierarchy(hid uint32) {
 	// We'll have one cgroup per hierarchy per task.
 	releasedCGs = make([]Cgroup, 0, len(k.tasks.Root.tids))
 	k.tasks.forEachTaskLocked(func(t *Task) {
-		if t.exitState != TaskExitNone {
+		if t.exitStateLocked() != TaskExitNone {
 			return
 		}
 		t.mu.Lock()

pkg/sentry/kernel/posixtimer.go

@@ -76,10 +76,8 @@ func (it *IntervalTimer) timerSettingChanged() {
 	if it.target == nil {
 		return
 	}
-	it.target.tg.pidns.owner.mu.RLock()
-	defer it.target.tg.pidns.owner.mu.RUnlock()
-	it.target.tg.signalHandlers.mu.Lock()
-	defer it.target.tg.signalHandlers.mu.Unlock()
+	sh := it.target.tg.signalLock()
+	defer sh.mu.Unlock()
 	it.sigorphan = true
 	it.overrunCur = 0
 	it.overrunLast = 0
@@ -121,10 +119,8 @@ func (it *IntervalTimer) NotifyTimer(exp uint64, setting ktime.Setting) (ktime.S
 		return ktime.Setting{}, false
 	}
 
-	it.target.tg.pidns.owner.mu.RLock()
-	defer it.target.tg.pidns.owner.mu.RUnlock()
-	it.target.tg.signalHandlers.mu.Lock()
-	defer it.target.tg.signalHandlers.mu.Unlock()
+	sh := it.target.tg.signalLock()
+	defer sh.mu.Unlock()
 
 	if it.sigpending {
 		it.overrunCur += exp

pkg/sentry/kernel/ptrace.go

@@ -488,7 +488,7 @@ func (t *Task) ptraceTraceme() error {
 	if !t.parent.canTraceLocked(t, true) {
 		return linuxerr.EPERM
 	}
-	if t.parent.exitState != TaskExitNone {
+	if t.parent.exitStateLocked() != TaskExitNone {
 		// Fail silently, as if we were successfully attached but then
 		// immediately detached. This is consistent with Linux.
 		return nil
@@ -515,7 +515,7 @@ func (t *Task) ptraceAttach(target *Task, seize bool, opts uintptr) error {
 	// Attaching to zombies and dead tasks is not permitted; the exit
 	// notification logic relies on this. Linux allows attaching to PF_EXITING
 	// tasks, though.
-	if target.exitState >= TaskExitZombie {
+	if target.exitStateLocked() >= TaskExitZombie {
 		return linuxerr.EPERM
 	}
 	if seize {
@@ -621,7 +621,7 @@ func (t *Task) forgetTracerLocked() {
 	// of restart from group-stop is currently buggy, but the "as planned"
 	// behavior is to leave tracee stopped and waiting for SIGCONT." -
 	// ptrace(2))
-	if (t.tg.groupStopComplete || t.tg.groupStopPendingCount != 0) && !t.groupStopPending && t.exitState < TaskExitInitiated {
+	if (t.tg.groupStopComplete || t.tg.groupStopPendingCount != 0) && !t.groupStopPending && t.exitStateLocked() < TaskExitInitiated {
 		t.groupStopPending = true
 		// t already participated in the group stop when it unset
 		// groupStopPending.
@@ -959,7 +959,7 @@ func (t *Task) ptraceInterrupt(target *Task) error {
 	}
 	target.tg.signalHandlers.mu.Lock()
 	defer target.tg.signalHandlers.mu.Unlock()
-	if target.killedLocked() || target.exitState >= TaskExitInitiated {
+	if target.killedLocked() || target.exitStateLocked() >= TaskExitInitiated {
 		return nil
 	}
 	target.trapStopPending = true

pkg/sentry/kernel/signal_handlers.go

@@ -50,12 +50,12 @@ func (sh *SignalHandlers) Fork() *SignalHandlers {
 	return sh2
 }
 
-// CopyForExec returns a copy of sh for a thread group that is undergoing an
-// execve. (See comments in Task.finishExec.)
-func (sh *SignalHandlers) CopyForExec() *SignalHandlers {
+// copyForExecLocked returns a copy of sh for a thread group that is undergoing
+// an execve. (See comments in Task.finishExec.)
+//
+// Preconditions: sh.mu must be locked.
+func (sh *SignalHandlers) copyForExecLocked() *SignalHandlers {
 	sh2 := NewSignalHandlers()
-	sh.mu.Lock()
-	defer sh.mu.Unlock()
 	for sig, act := range sh.actions {
 		if act.Handler == linux.SIG_IGN {
 			sh2.actions[sig] = linux.SigAction{

pkg/sentry/kernel/task.go

@@ -292,7 +292,7 @@ type Task struct {
 	//
 	// exitState is protected by the TaskSet mutex. exitState is owned by the
 	// task goroutine.
-	exitState TaskExitState
+	exitState atomicbitops.Uint32
 
 	// exitTracerNotified is true if the exit path has either signaled the
 	// task's tracer to indicate the exit, or determined that no such signal is

pkg/sentry/kernel/task_clone.go

@@ -396,8 +396,8 @@ func getCloneSeccheckInfo(t, nt *Task, flags uint64) (seccheck.FieldSet, *pb.Clo
 //
 // Preconditions: The caller must be running on t's task goroutine.
 func (t *Task) maybeBeginVforkStop(child *Task) {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
+	t.tg.pidns.owner.mu.Lock()
+	defer t.tg.pidns.owner.mu.Unlock()
 	t.tg.signalHandlers.mu.Lock()
 	defer t.tg.signalHandlers.mu.Unlock()
 	if t.killedLocked() {
@@ -410,8 +410,8 @@ func (t *Task) maybeBeginVforkStop(child *Task) {
 }
 
 func (t *Task) unstopVforkParent() {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
+	t.tg.pidns.owner.mu.Lock()
+	defer t.tg.pidns.owner.mu.Unlock()
 	if p := t.vforkParent; p != nil {
 		p.tg.signalHandlers.mu.Lock()
 		defer p.tg.signalHandlers.mu.Unlock()

pkg/sentry/kernel/task_exec.go

@@ -170,12 +170,13 @@ func (r *runSyscallAfterExecStop) execute(t *Task) taskRunState {
 	// we're about to change. Note that we have to stop and destroy timers
 	// without holding any mutexes to avoid circular lock ordering.
 	var its []*IntervalTimer
-	t.tg.signalHandlers.mu.Lock()
+	oldSignalHandlers := t.tg.signalHandlers
+	oldSignalHandlers.mu.Lock()
 	for _, it := range t.tg.timers {
 		its = append(its, it)
 	}
 	clear(t.tg.timers)
-	t.tg.signalHandlers.mu.Unlock()
+	oldSignalHandlers.mu.Unlock()
 	t.tg.pidns.owner.mu.Unlock()
 	for _, it := range its {
 		it.DestroyTimer()
@@ -196,8 +197,10 @@ func (r *runSyscallAfterExecStop) execute(t *Task) taskRunState {
 	//	- "Disposition" only means sigaction::sa_handler/sa_sigaction; flags,
 	//		restorer (if present), and mask are always reset. (See Linux's
 	//		fs/exec.c:setup_new_exec => kernel/signal.c:flush_signal_handlers.)
-	t.tg.signalHandlers = t.tg.signalHandlers.CopyForExec()
+	oldSignalHandlers.mu.Lock() // to ensure ThreadGroup.signalLock()'s correctness
+	t.tg.setSignalHandlersLocked(oldSignalHandlers.copyForExecLocked())
 	t.endStopCond.L = &t.tg.signalHandlers.mu
+	oldSignalHandlers.mu.Unlock()
 	// "Any alternate signal stack is not preserved (sigaltstack(2))." - execve(2)
 	t.signalStack = linux.SignalStack{Flags: linux.SS_DISABLE}
 	// "The termination signal is reset to SIGCHLD (see clone(2))."

pkg/sentry/kernel/task_exit.go

@@ -41,7 +41,7 @@ import (
 // TaskExitState represents a step in the task exit path.
 //
 // "Exiting" and "exited" are often ambiguous; prefer to name specific states.
-type TaskExitState int
+type TaskExitState uint32
 
 const (
 	// TaskExitNone indicates that the task has not begun exiting.
@@ -109,6 +109,7 @@ func (t *Task) killed() bool {
 	return t.killedLocked()
 }
 
+// Preconditions: The signal mutex must be locked.
 func (t *Task) killedLocked() bool {
 	return t.pendingSignals.pendingSet&linux.SignalSetOf(linux.SIGKILL) != 0
 }
@@ -203,13 +204,15 @@ func (ts *TaskSet) IsExiting() bool {
 // sets it to newExit. If t's current exit state is not oldExit,
 // advanceExitStateLocked panics.
 //
-// Preconditions: The TaskSet mutex must be locked.
+// Preconditions: The TaskSet mutex must be locked for writing.
 func (t *Task) advanceExitStateLocked(oldExit, newExit TaskExitState) {
-	if t.exitState != oldExit {
-		panic(fmt.Sprintf("Transitioning from exit state %v to %v: unexpected preceding state %v", oldExit, newExit, t.exitState))
+	// This doesn't need to use atomic CAS (or load) since we hold the TaskSet
+	// mutex.
+	if curExit := t.exitStateLocked(); curExit != oldExit {
+		panic(fmt.Sprintf("Transitioning from exit state %v to %v: unexpected preceding state %v", oldExit, newExit, curExit))
 	}
 	t.Debugf("Transitioning from exit state %v to %v", oldExit, newExit)
-	t.exitState = newExit
+	t.exitState.Store(uint32(newExit))
 }
 
 // runExit is the entry point into the task exit path.
@@ -448,9 +451,10 @@ func (t *Task) findReparentTargetLocked() *Task {
 	return nil
 }
 
+// Preconditions: The TaskSet mutex must be locked.
 func (tg *ThreadGroup) anyNonExitingTaskLocked() *Task {
 	for t := tg.tasks.Front(); t != nil; t = t.Next() {
-		if t.exitState == TaskExitNone {
+		if t.exitStateLocked() == TaskExitNone {
 			return t
 		}
 	}
@@ -624,7 +628,7 @@ func (*runExitNotify) execute(t *Task) taskRunState {
 //
 // Preconditions: The TaskSet mutex must be locked for writing.
 func (t *Task) exitNotifyLocked(fromPtraceDetach bool) {
-	if t.exitState != TaskExitZombie {
+	if t.exitStateLocked() != TaskExitZombie {
 		return
 	}
 	if !t.exitTracerNotified {
@@ -794,10 +798,8 @@ func getExitNotifyParentSeccheckInfo(t *Task) (seccheck.FieldSet, *pb.ExitNotify
 // ExitStatus returns t's exit status, which is only guaranteed to be
 // meaningful if t.ExitState() != TaskExitNone.
 func (t *Task) ExitStatus() linux.WaitStatus {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
-	t.tg.signalHandlers.mu.Lock()
-	defer t.tg.signalHandlers.mu.Unlock()
+	sh := t.tg.signalLock()
+	defer sh.mu.Unlock()
 	return t.exitStatus
 }
 
@@ -1020,7 +1022,7 @@ func (t *Task) waitParentLocked(opts *WaitOptions, parent *Task) (*WaitResult, b
 		if opts.Events&(EventChildGroupStop|EventGroupContinue) == 0 {
 			continue
 		}
-		if child.exitState >= TaskExitInitiated {
+		if child.exitStateLocked() >= TaskExitInitiated {
 			continue
 		}
 		// If the waiter is in the same thread group as the task's
@@ -1060,7 +1062,7 @@ func (t *Task) waitParentLocked(opts *WaitOptions, parent *Task) (*WaitResult, b
 		if opts.Events&(EventTraceeStop|EventGroupContinue) == 0 {
 			continue
 		}
-		if tracee.exitState >= TaskExitInitiated {
+		if tracee.exitStateLocked() >= TaskExitInitiated {
 			continue
 		}
 		anyWaitableTasks = true
@@ -1235,9 +1237,12 @@ func (t *Task) waitCollectTraceeStopLocked(target *Task, opts *WaitOptions) *Wai
 
 // ExitState returns t's current progress through the exit path.
 func (t *Task) ExitState() TaskExitState {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
-	return t.exitState
+	return TaskExitState(t.exitState.Load())
+}
+
+// Preconditions: The TaskSet mutex must be locked.
+func (t *Task) exitStateLocked() TaskExitState {
+	return TaskExitState(t.exitState.RacyLoad())
 }
 
 // ParentDeathSignal returns t's parent death signal.

pkg/sentry/kernel/task_sched.go

@@ -521,9 +521,7 @@ func (tg *ThreadGroup) updateCPUTimersEnabledLocked() {
 func (t *Task) StateStatus() string {
 	switch s := t.TaskGoroutineSchedInfo().State; s {
 	case TaskGoroutineNonexistent, TaskGoroutineRunningSys:
-		t.tg.pidns.owner.mu.RLock()
-		defer t.tg.pidns.owner.mu.RUnlock()
-		switch t.exitState {
+		switch t.ExitState() {
 		case TaskExitZombie:
 			return "Z (zombie)"
 		case TaskExitDead:
@@ -544,8 +542,8 @@ func (t *Task) StateStatus() string {
 	case TaskGoroutineBlockedInterruptible:
 		return "S (sleeping)"
 	case TaskGoroutineStopped:
-		t.tg.signalHandlers.mu.Lock()
-		defer t.tg.signalHandlers.mu.Unlock()
+		sh := t.tg.signalLock()
+		defer sh.mu.Unlock()
 		switch t.stop.(type) {
 		case *groupStop:
 			return "T (stopped)"

pkg/sentry/kernel/task_signals.go

@@ -146,10 +146,8 @@ func (tg *ThreadGroup) discardSpecificLocked(sig linux.Signal) {
 
 // PendingSignals returns the set of pending signals.
 func (t *Task) PendingSignals() linux.SignalSet {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
-	t.tg.signalHandlers.mu.Lock()
-	defer t.tg.signalHandlers.mu.Unlock()
+	sh := t.tg.signalLock()
+	defer sh.mu.Unlock()
 	return t.pendingSignals.pendingSet | t.tg.pendingSignals.pendingSet
 }
 
@@ -377,19 +375,15 @@ func (t *Task) Sigtimedwait(set linux.SignalSet, timeout time.Duration) (*linux.
 //	linuxerr.EINVAL - The signal is not valid.
 //	linuxerr.EAGAIN - THe signal is realtime, and cannot be queued.
 func (t *Task) SendSignal(info *linux.SignalInfo) error {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
-	t.tg.signalHandlers.mu.Lock()
-	defer t.tg.signalHandlers.mu.Unlock()
+	sh := t.tg.signalLock()
+	defer sh.mu.Unlock()
 	return t.sendSignalLocked(info, false /* group */)
 }
 
 // SendGroupSignal sends the given signal to t's thread group.
 func (t *Task) SendGroupSignal(info *linux.SignalInfo) error {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
-	t.tg.signalHandlers.mu.Lock()
-	defer t.tg.signalHandlers.mu.Unlock()
+	sh := t.tg.signalLock()
+	defer sh.mu.Unlock()
 	return t.sendSignalLocked(info, true /* group */)
 }
 
@@ -403,12 +397,14 @@ func (tg *ThreadGroup) SendSignal(info *linux.SignalInfo) error {
 	return tg.leader.sendSignalLocked(info, true /* group */)
 }
 
+// Preconditions: The signal mutex must be locked.
 func (t *Task) sendSignalLocked(info *linux.SignalInfo, group bool) error {
 	return t.sendSignalTimerLocked(info, group, nil)
 }
 
+// Preconditions: The signal mutex must be locked.
 func (t *Task) sendSignalTimerLocked(info *linux.SignalInfo, group bool, timer *IntervalTimer) error {
-	if t.exitState == TaskExitDead {
+	if t.ExitState() == TaskExitDead {
 		return linuxerr.ESRCH
 	}
 	sig := linux.Signal(info.Signo)
@@ -482,6 +478,7 @@ func (t *Task) sendSignalTimerLocked(info *linux.SignalInfo, group bool, timer *
 	return nil
 }
 
+// Preconditions: The signal mutex must be locked.
 func (tg *ThreadGroup) applySignalSideEffectsLocked(sig linux.Signal) {
 	switch {
 	case linux.SignalSetOf(sig)&StopSignals != 0:
@@ -572,6 +569,7 @@ func (t *Task) forceSignal(sig linux.Signal, unconditional bool) {
 	t.forceSignalLocked(sig, unconditional)
 }
 
+// Preconditions: The signal mutex must be locked.
 func (t *Task) forceSignalLocked(sig linux.Signal, unconditional bool) {
 	blocked := linux.SignalSetOf(sig)&linux.SignalSet(t.signalMask.RacyLoad()) != 0
 	act := t.tg.signalHandlers.actions[sig]
@@ -790,7 +788,7 @@ func (t *Task) initiateGroupStop(info *linux.SignalInfo) {
 	}
 	t.tg.groupStopPendingCount = 0
 	for t2 := t.tg.tasks.Front(); t2 != nil; t2 = t2.Next() {
-		if t2.killedLocked() || t2.exitState >= TaskExitInitiated {
+		if t2.killedLocked() || t2.exitStateLocked() >= TaskExitInitiated {
 			t2.groupStopPending = false
 			continue
 		}

pkg/sentry/kernel/task_stop.go

@@ -102,8 +102,6 @@ type TaskStop interface {
 //   - The caller must be running on the task goroutine.
 //   - The task must not already be in an internal stop (i.e. t.stop == nil).
 func (t *Task) beginInternalStop(s TaskStop) {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
 	t.tg.signalHandlers.mu.Lock()
 	defer t.tg.signalHandlers.mu.Unlock()
 	t.beginInternalStopLocked(s)
@@ -143,10 +141,8 @@ func (t *Task) endInternalStopLocked() {
 // BeginExternalStop indicates the start of an external stop that applies to t.
 // BeginExternalStop does not wait for t's task goroutine to stop.
 func (t *Task) BeginExternalStop() {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
-	t.tg.signalHandlers.mu.Lock()
-	defer t.tg.signalHandlers.mu.Unlock()
+	sh := t.tg.signalLock()
+	defer sh.mu.Unlock()
 	t.beginStopLocked()
 	t.interrupt()
 }
@@ -155,10 +151,8 @@ func (t *Task) BeginExternalStop() {
 // call to Task.BeginExternalStop. EndExternalStop does not wait for t's task
 // goroutine to resume.
 func (t *Task) EndExternalStop() {
-	t.tg.pidns.owner.mu.RLock()
-	defer t.tg.pidns.owner.mu.RUnlock()
-	t.tg.signalHandlers.mu.Lock()
-	defer t.tg.signalHandlers.mu.Unlock()
+	sh := t.tg.signalLock()
+	defer sh.mu.Unlock()
 	t.endStopLocked()
 }
 

pkg/sentry/kernel/thread_group.go

@@ -49,14 +49,23 @@ type ThreadGroup struct {
 	// signalHandlers. (This is analogous to Linux's use of struct
 	// sighand_struct::siglock.)
 	//
-	// The signalHandlers pointer can only be mutated during an execve
-	// (Task.finishExec). Consequently, when it's possible for a task in the
-	// thread group to be completing an execve, signalHandlers is protected by
-	// the owning TaskSet.mu. Otherwise, it is possible to read the
-	// signalHandlers pointer without synchronization. In particular,
-	// completing an execve requires that all other tasks in the thread group
-	// have exited, so task goroutines do not need the owning TaskSet.mu to
-	// read the signalHandlers pointer of their thread groups.
+	// The signalHandlers pointer is only mutated during execve
+	// (Task.finishExec), which occurs with TaskSet.mu and (the previous)
+	// signalHandlers.mu locked. Consequently:
+	//
+	// - Completing an execve requires that all other tasks in the thread group
+	// have exited, so task goroutines for non-exiting tasks in the thread
+	// group can read signalHandlers without a race condition.
+	//
+	// - If TaskSet.mu is locked (for reading or writing), any goroutine may
+	// read signalHandlers without a race condition.
+	//
+	// - If it is impossible for a task in the thread group to be completing an
+	// execve for another reason, any goroutine may read signalHandlers without
+	// a race condition.
+	//
+	// - Otherwise, ThreadGroup.signalLock() should be used to non-racily lock
+	// signalHandlers.mu; it also returns the locked signalHandlers.
 	signalHandlers *SignalHandlers
 
 	// pendingSignals is the set of pending signals that may be handled by any
@@ -298,12 +307,19 @@ func (tg *ThreadGroup) loadOldRSeqCritical(_ goContext.Context, r *OldRSeqCritic
 	tg.oldRSeqCritical.Store(r)
 }
 
-// SignalHandlers returns the signal handlers used by tg.
-//
-// Preconditions: The caller must provide the synchronization required to read
-// tg.signalHandlers, as described in the field's comment.
-func (tg *ThreadGroup) SignalHandlers() *SignalHandlers {
-	return tg.signalHandlers
+// signalLock atomically locks tg.SignalHandlers().mu and returns the
+// SignalHandlers.
+func (tg *ThreadGroup) signalLock() *SignalHandlers {
+	sh := tg.SignalHandlers()
+	for {
+		sh.mu.Lock()
+		sh2 := tg.SignalHandlers()
+		if sh == sh2 {
+			return sh
+		}
+		sh.mu.Unlock()
+		sh = sh2
+	}
 }
 
 // Limits returns tg's limits.
@@ -317,7 +333,6 @@ func (tg *ThreadGroup) Release(ctx context.Context) {
 	// since timers send signals with Timer.mu locked.
 	tg.itimerRealTimer.Destroy()
 	var its []*IntervalTimer
-	tg.pidns.owner.mu.Lock()
 	tg.signalHandlers.mu.Lock()
 	for _, it := range tg.timers {
 		its = append(its, it)
@@ -325,7 +340,7 @@ func (tg *ThreadGroup) Release(ctx context.Context) {
 	clear(tg.timers) // nil maps can't be saved
 	// Disassociate from the tty if we have one.
 	if tg.tty != nil {
-		tg.tty.mu.Lock()
+		tg.tty.mu.Lock() // FIXME(b/370763686)
 		if tg.tty.tg == tg {
 			tg.tty.tg = nil
 		}
@@ -333,7 +348,6 @@ func (tg *ThreadGroup) Release(ctx context.Context) {
 		tg.tty = nil
 	}
 	tg.signalHandlers.mu.Unlock()
-	tg.pidns.owner.mu.Unlock()
 	for _, it := range its {
 		it.DestroyTimer()
 	}
@@ -500,8 +514,8 @@ func (tg *ThreadGroup) ForegroundProcessGroupID(tty *TTY) (ProcessGroupID, error
 	tty.mu.Lock()
 	defer tty.mu.Unlock()
 
-	tg.pidns.owner.mu.Lock()
-	defer tg.pidns.owner.mu.Unlock()
+	tg.pidns.owner.mu.RLock()
+	defer tg.pidns.owner.mu.RUnlock()
 	tg.signalHandlers.mu.Lock()
 	defer tg.signalHandlers.mu.Unlock()
 

pkg/sentry/kernel/thread_group_unsafe.go

@@ -0,0 +1,33 @@
+// Copyright 2024 The gVisor Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package kernel
+
+import (
+	"sync/atomic"
+	"unsafe"
+)
+
+// SignalHandlers returns the signal handlers used by tg. The returned value
+// may be racy; see the field comment for ThreadGroup.signalHandlers.
+func (tg *ThreadGroup) SignalHandlers() *SignalHandlers {
+	return (*SignalHandlers)(atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&tg.signalHandlers))))
+}
+
+// Preconditions: The only permitted caller of this function is
+// Task.finishExec(), as described in the field comment for
+// ThreadGroup.signalHandlers.
+func (tg *ThreadGroup) setSignalHandlersLocked(sh *SignalHandlers) {
+	atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(&tg.signalHandlers)), unsafe.Pointer(sh))
+}

pkg/sentry/kernel/tty.go

@@ -37,10 +37,8 @@ type TTY struct {
 // TTY returns the thread group's controlling terminal. If nil, there is no
 // controlling terminal.
 func (tg *ThreadGroup) TTY() *TTY {
-	tg.pidns.owner.mu.RLock()
-	defer tg.pidns.owner.mu.RUnlock()
-	tg.signalHandlers.mu.Lock()
-	defer tg.signalHandlers.mu.Unlock()
+	sh := tg.signalLock()
+	defer sh.mu.Unlock()
 	return tg.tty
 }
 
@@ -60,9 +58,7 @@ func (tty *TTY) SignalForegroundProcessGroup(info *linux.SignalInfo) {
 	}
 
 	tg.pidns.owner.mu.Lock()
-	tg.signalHandlers.mu.Lock()
 	fg := tg.processGroup.session.foreground
-	tg.signalHandlers.mu.Unlock()
 	tg.pidns.owner.mu.Unlock()
 
 	if fg == nil {
@@ -70,8 +66,6 @@ func (tty *TTY) SignalForegroundProcessGroup(info *linux.SignalInfo) {
 		return
 	}
 
-	// SendSignal will take TaskSet.mu and signalHandlers.mu, so we cannot
-	// hold them here.
 	if err := fg.SendSignal(info); err != nil {
 		log.Warningf("failed to signal foreground process group (pgid=%d): %v", fg.id, err)
 	}