Date:   Tue, 2 Nov 2021 09:01:19 -0700
From:   Kyle Huey <me@...ehuey.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Jens Axboe <axboe@...nel.dk>,
        Peter Zijlstra <peterz@...radead.org>,
        Marco Elver <elver@...gle.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Collingbourne <pcc@...gle.com>,
        Alexey Gladkov <legion@...nel.org>,
        "Robert O'Callahan" <rocallahan@...il.com>,
        Marko Mäkelä <marko.makela@...iadb.com>,
        open list <linux-kernel@...r.kernel.org>
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@...ssion.com> wrote:
>
> Kyle Huey <me@...ehuey.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(&current->blocked, signr)) {
> +       if (sigismember(&current->blocked, signr) ||
> +           signal_group_exit(current->signal)) {
>                 send_signal(signr, info, current, PIDTYPE_PID);
>                 signr = 0;
>         }
>

Powered by blists - more mailing lists