Messages in this thread | | | From | Kyle Huey <> | Date | Tue, 2 Nov 2021 09:01:19 -0700 | Subject | Re: [PATCH] signal: SIGKILL can cause signal effects to appear at PTRACE_EVENT_EXIT without tracer notification |
| |
On Tue, Nov 2, 2021 at 7:09 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Kyle Huey <me@kylehuey.com> writes: > > > rr, a userspace record and replay debugger[0], uses the recorded register > > state at PTRACE_EVENT_EXIT to find the point in time at which to cease > > executing the program during replay. > > > > If a SIGKILL races with processing another signal in get_signal, it is > > possible for the kernel to decline to notify the tracer of the original > > signal. But if the original signal had a handler, the kernel proceeds > > with setting up a signal handler frame as if the tracer had chosen to > > deliver the signal unmodified to the tracee. When the kernel goes to > > execute the signal handler that it has now modified the stack and registers > > for, it will discover the pending SIGKILL, and terminate the tracee > > without executing the handler. When PTRACE_EVENT_EXIT is delivered to > > the tracer, however, the effects of handler setup will be visible to > > the tracer. > > > > Because rr (the tracer) was never notified of the signal, it is not aware > > that a signal handler frame was set up and expects the state of the program > > at PTRACE_EVENT_EXIT to be a state that will be reconstructed naturally > > by allowing the program to execute from the last event. When that fails > > to happen during replay, rr will assert and die. > > > > The following patches add an explicit check for a newly pending SIGKILL > > after the ptracer has been notified and the siglock has been reacquired. > > If this happens, we stop processing the current signal and proceed > > immediately to handling the SIGKILL. This makes the state reported at > > PTRACE_EVENT_EXIT the unmodified state of the program, and also avoids the > > work to set up a signal handler frame that will never be used. > > > > This issue was originally reported by the credited rr user. > > > > [0] https://rr-project.org/ > > If I read this correctly the problem is not precisely that the rr > debugger is never notified about the signal, but rather that the program > is killed with SIGKILL before rr can read the notification and see which > signal it is.
The precise problem is that the kernel made a modification to the tracee state (setting up the signal handler frame) without telling the tracer about it (delivering the ptrace notification for the pending non-SIGKILL signal). That can be fixed either by not modifying the tracee state here or by telling the tracer about the signal (that will never actually run). I suspect we'll all agree that the former seems preferable.
> This definitely sounds like a quality of implementation issue. > > The solution that is proposed in your patches simply drops the signal > when SIGKILL is pending.
That's right.
> I think we can have a slightly better of quality of implementation > than that (as well as a simpler implementation) by requeuing the > signal instead of simply dropping it. Something like the below.
What is the benefit of requeueing the signal? All pending signals will be dropped when the SIGKILL is processed, no?
> Can you test that and see if it works for you?
It does not work. This triggers an infinite loop in get_signal, as we dequeue the signal, attempt to notify the ptracer, see the pending sigkill, requeue the signal, go around the loop, dequeue the original signal ...
- Kyle
> Eric > > diff --git a/kernel/signal.c b/kernel/signal.c > index 056a107e3cbc..0dff366b9129 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -2610,7 +2610,8 @@ static int ptrace_signal(int signr, kernel_siginfo_t *info) > } > > /* If the (new) signal is now blocked, requeue it. */ > - if (sigismember(¤t->blocked, signr)) { > + if (sigismember(¤t->blocked, signr) || > + signal_group_exit(current->signal)) { > send_signal(signr, info, current, PIDTYPE_PID); > signr = 0; > } >
| |