Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
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 =?utf-8?B?TcOka2Vsw6Q=?= <marko.makela@mariadb.com>,
        open list <linux-kernel@vger.kernel.org>
References: <20211101034147.6203-1-khuey@kylehuey.com>
        <877ddqabvs.fsf@disp2133>
        <CAP045AqJVXA60R9RF8Gb2PWGBsK6bZ7tVBkdCcPYYrp6rOkG-Q@mail.gmail.com>
Date:   Tue, 02 Nov 2021 13:06:32 -0500
In-Reply-To: <CAP045AqJVXA60R9RF8Gb2PWGBsK6bZ7tVBkdCcPYYrp6rOkG-Q@mail.gmail.com>
        (Kyle Huey's message of "Tue, 2 Nov 2021 09:01:19 -0700")
Message-ID: <87fsse8maf.fsf@disp2133>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] signal: SIGKILL can cause signal effects to appear at PTRACE_EVENT_EXIT without tracer notification
Precedence: bulk

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.

> 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.

>> 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(&current->blocked, signr)) {
+	if (sigismember(&current->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