From 03bebc4402b8bb564a0a3c1607556c9a25448a57 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Tue, 1 Oct 2024 12:44:40 -0700 Subject: [PATCH] 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 | 1 + pkg/sentry/kernel/kernel.go | 4 +- pkg/sentry/kernel/posixtimer.go | 12 ++---- pkg/sentry/kernel/ptrace.go | 8 ++-- pkg/sentry/kernel/signal_handlers.go | 10 ++--- pkg/sentry/kernel/task.go | 2 +- pkg/sentry/kernel/task_clone.go | 8 ++-- pkg/sentry/kernel/task_exec.go | 9 ++-- pkg/sentry/kernel/task_exit.go | 37 +++++++++-------- pkg/sentry/kernel/task_sched.go | 8 ++-- pkg/sentry/kernel/task_signals.go | 26 ++++++------ pkg/sentry/kernel/task_stop.go | 14 ++----- pkg/sentry/kernel/thread_group.go | 52 +++++++++++++++--------- pkg/sentry/kernel/thread_group_unsafe.go | 33 +++++++++++++++ pkg/sentry/kernel/tty.go | 10 +---- 15 files changed, 135 insertions(+), 99 deletions(-) create mode 100644 pkg/sentry/kernel/thread_group_unsafe.go diff --git a/pkg/sentry/kernel/BUILD b/pkg/sentry/kernel/BUILD index 3b85c43a2..80ce99dc1 100644 --- a/pkg/sentry/kernel/BUILD +++ b/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", diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index 26cd4786c..68f640ee4 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/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() diff --git a/pkg/sentry/kernel/posixtimer.go b/pkg/sentry/kernel/posixtimer.go index 07205cc68..c95748967 100644 --- a/pkg/sentry/kernel/posixtimer.go +++ b/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 diff --git a/pkg/sentry/kernel/ptrace.go b/pkg/sentry/kernel/ptrace.go index a8c591146..535fa8f71 100644 --- a/pkg/sentry/kernel/ptrace.go +++ b/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 diff --git a/pkg/sentry/kernel/signal_handlers.go b/pkg/sentry/kernel/signal_handlers.go index 3510b9dea..7bf50b430 100644 --- a/pkg/sentry/kernel/signal_handlers.go +++ b/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{ diff --git a/pkg/sentry/kernel/task.go b/pkg/sentry/kernel/task.go index 2df014447..35b5fc63e 100644 --- a/pkg/sentry/kernel/task.go +++ b/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 diff --git a/pkg/sentry/kernel/task_clone.go b/pkg/sentry/kernel/task_clone.go index 72a2c0322..3a644c57f 100644 --- a/pkg/sentry/kernel/task_clone.go +++ b/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() diff --git a/pkg/sentry/kernel/task_exec.go b/pkg/sentry/kernel/task_exec.go index 1e67e79e6..170df86d2 100644 --- a/pkg/sentry/kernel/task_exec.go +++ b/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))." diff --git a/pkg/sentry/kernel/task_exit.go b/pkg/sentry/kernel/task_exit.go index eb5bbbb28..26c3679f8 100644 --- a/pkg/sentry/kernel/task_exit.go +++ b/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. diff --git a/pkg/sentry/kernel/task_sched.go b/pkg/sentry/kernel/task_sched.go index 62c68f23f..8d6270d41 100644 --- a/pkg/sentry/kernel/task_sched.go +++ b/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)" diff --git a/pkg/sentry/kernel/task_signals.go b/pkg/sentry/kernel/task_signals.go index 22d6bcddf..36d02b6cd 100644 --- a/pkg/sentry/kernel/task_signals.go +++ b/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 } diff --git a/pkg/sentry/kernel/task_stop.go b/pkg/sentry/kernel/task_stop.go index 083b68cf2..345648c93 100644 --- a/pkg/sentry/kernel/task_stop.go +++ b/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() } diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index 4e5dd4f6c..5ec71a7c1 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/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() diff --git a/pkg/sentry/kernel/thread_group_unsafe.go b/pkg/sentry/kernel/thread_group_unsafe.go new file mode 100644 index 000000000..7efab40ab --- /dev/null +++ b/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)) +} diff --git a/pkg/sentry/kernel/tty.go b/pkg/sentry/kernel/tty.go index 48402abee..ab75959ed 100644 --- a/pkg/sentry/kernel/tty.go +++ b/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) }