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>,
        linux-kernel@vger.kernel.org
References: <20211101034147.6203-1-khuey@kylehuey.com>
Date:   Tue, 02 Nov 2021 09:08:23 -0500
In-Reply-To: <20211101034147.6203-1-khuey@kylehuey.com> (Kyle Huey's message
        of "Sun, 31 Oct 2021 20:41:45 -0700")
Message-ID: <877ddqabvs.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:

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

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.

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.

Can you test that and see if it works for you?

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;
 	}