From: ebiederm@xmission.com (Eric W. Biederman)
To: Kyle Huey <me@kylehuey.com>
Cc: "Jens Axboe" <axboe@kernel.dk>,
"Peter Zijlstra" <peterz@infradead.org>,
"Marco Elver" <elver@google.com>,
"Oleg Nesterov" <oleg@redhat.com>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Peter Collingbourne" <pcc@google.com>,
"Alexey Gladkov" <legion@kernel.org>,
"Robert O'Callahan" <rocallahan@gmail.com>,
"Marko Mäkelä" <marko.makela@mariadb.com>,
"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] signal: SIGKILL can cause signal effects to appear at PTRACE_EVENT_EXIT without tracer notification
Date: Sun, 14 Nov 2021 11:19:03 -0600 [thread overview]
Message-ID: <87czn2k648.fsf@email.froward.int.ebiederm.org> (raw)
In-Reply-To: <CAP045AqsstnxfTyXhhCGDSucqGN7BTtfHJ5s6ZxUQC5K-JU56A@mail.gmail.com> (Kyle Huey's message of "Mon, 8 Nov 2021 15:58:21 -0800")
Kyle Huey <me@kylehuey.com> writes:
> On Tue, Nov 2, 2021 at 12:09 PM Kyle Huey <me@kylehuey.com> wrote:
>>
>> On Tue, Nov 2, 2021 at 11:07 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> >
>> > Kyle Huey <me@kylehuey.com> writes:
>> >
>> > > 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).
>> >
>> > Except the kernel did make it to ptrace_stop. The stop just did not
>> > fully happen because of SIGKILL. I expect SIGCHLD was sent to the
>> > tracer as part of that stop that never fully happened.
>>
>> I don't know whether SIGCHLD was sent to the tracer (rr doesn't use it
>> directly) but waiting on the process does not produce a wait status
>> corresponding to the signal delivery stop for the original signal.
>> Waiting on the tracee skips immediately from whatever the preceding
>> ptrace event was to the PTRACE_EVENT_EXIT.
>>
>> (In our particular case, if it had been notified of the signal, we
>> would have chosen to suppress the signal, because the signal in
>> question is a SIGSEGV from an rdtsc instruction that has been disabled
>> via prctl(PR_SET_TSC, PR_TSC_SIGSEGV) and we emulate it in the tracer
>> due to its non-deterministic behavior. So we really don't expect to
>> see the tracee signal handler.)
>>
>> > > 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?
>> >
>> > Not before PTRACE_EVENT_EXIT. In fact the pending signals are not
>> > actually flushed until the thread or the entire process is reaped.
>> >
>> > Further the coredump code makes some attempt to write out the
>> > pending signals. The code appears to predate siginfo support
>> > in the kernel so it misses a lot but it is there.
>> >
>> > The real advantage is that it keeps the logic of dealing with weird
>> > ptrace_stop logic in ptrace_signal where it belongs. It also allows the
>> > common (and missing in this case) idiom of goto relock to be used.
>> >
>> > So I think changing ptrace_signal will be much more maintainable.
>>
>> Ok.
>>
>> > >> 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 ...
>> >
>> > Apologies I made a bit of a thinko. That change also needs to change
>> > the handling of if (signr == 0) after ptrace_signal.
>> >
>> > Which means it would need to be something like the below.
>> >
>> > diff --git a/kernel/signal.c b/kernel/signal.c
>> > index 056a107e3cbc..eddb745b34a7 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;
>> > }
>> > @@ -2764,8 +2765,10 @@ bool get_signal(struct ksignal *ksig)
>> > if (unlikely(current->ptrace) && (signr != SIGKILL) &&
>> > !(sighand->action[signr -1].sa.sa_flags & SA_IMMUTABLE)) {
>> > signr = ptrace_signal(signr, &ksig->info);
>> > - if (!signr)
>> > - continue;
>> > + if (!signr) {
>> > + spin_unlock_irq(&sighand->siglock);
>> > + goto relock;
>> > + }
>> > }
>> >
>> > ka = &sighand->action[signr-1];
>> >
>> > Eric
>>
>> Yeah that appears to fix the issue.
>>
>> - Kyle
>
> Is there anything else I need to do here or are you going to take it
> from here?
No pinging me is the helpful thing to do. I sometimes get distracted.
The merge window closes sometime later today and then v5.16-rc1 will be
out. Then I will have a good base to work against.
Until then I can't really merge anything.
Eric
next prev parent reply other threads:[~2021-11-14 17:19 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-01 3:41 [PATCH] signal: SIGKILL can cause signal effects to appear at PTRACE_EVENT_EXIT without tracer notification Kyle Huey
2021-11-01 3:41 ` [PATCH 1/2] signal: factor out SIGKILL generation in get_signal Kyle Huey
2021-11-01 3:41 ` [PATCH 2/2] signal: after notifying a ptracer of a signal, recheck for pending SIGKILLs Kyle Huey
2021-11-02 14:08 ` [PATCH] signal: SIGKILL can cause signal effects to appear at PTRACE_EVENT_EXIT without tracer notification Eric W. Biederman
2021-11-02 16:01 ` Kyle Huey
2021-11-02 18:06 ` Eric W. Biederman
2021-11-02 19:09 ` Kyle Huey
2021-11-08 23:58 ` Kyle Huey
2021-11-14 17:19 ` Eric W. Biederman [this message]
2021-11-16 5:29 ` [PATCH 0/3] signal: requeuing undeliverable signals Eric W. Biederman
2021-11-16 5:32 ` [PATCH 1/3] signal: In get_signal test for signal_group_exit every time through the loop Eric W. Biederman
2021-11-16 18:23 ` Kees Cook
2021-11-17 16:31 ` Eric W. Biederman
2021-11-16 5:33 ` [PATCH 2/3] signal: Requeue signals in the appropriate queue Eric W. Biederman
2021-11-16 18:30 ` Kees Cook
2021-11-17 16:42 ` Eric W. Biederman
2021-11-16 5:34 ` [PATCH 3/3] signal: Requeue ptrace signals Eric W. Biederman
2021-11-16 18:31 ` Kees Cook
2021-11-17 16:44 ` Eric W. Biederman
2021-11-17 16:24 ` [PATCH 0/3] signal: requeuing undeliverable signals Kyle Huey
2021-11-17 16:51 ` Eric W. Biederman
2021-11-18 6:12 ` Marko Mäkelä
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87czn2k648.fsf@email.froward.int.ebiederm.org \
--to=ebiederm@xmission.com \
--cc=axboe@kernel.dk \
--cc=elver@google.com \
--cc=legion@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marko.makela@mariadb.com \
--cc=me@kylehuey.com \
--cc=oleg@redhat.com \
--cc=pcc@google.com \
--cc=peterz@infradead.org \
--cc=rocallahan@gmail.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).