Re: RFC: Use of user space handler vs. SIG_DFL on forced signals

Marco Elver
2022-03-22T13:53:26Z

On Tue, Mar 22, 2022 at 02:25PM +0100, Dmitry Vyukov wrote:
> On Tue, 22 Mar 2022 at 11:42, Marco Elver <elver@google.com> wrote:
> >
> > Hello,
> >
> > Currently force_sig_info_to_task() will always unblock a blocked signal
> > but deliver the signal to SIG_DFL:
> >
> > [...]
> > * Note: If we unblock the signal, we always reset it to SIG_DFL,
> > * since we do not want to have a signal handler that was blocked
> > * be invoked when user space had explicitly blocked it.
> > [...]
> >
> > Is this requirement part of the POSIX spec? Or is the intent simply to
> > attempt to do the least-bad thing?
> >
> > The reason I'm asking is that we've encountered rare crashes with the
> > new SIGTRAP on perf events, due to patterns like this:
> >
> > <set up SIGTRAP on a perf event>
> > ...
> > sigset_t s;
> > sigemptyset(&s);
> > sigaddset(&s, SIGTRAP | <and others>);
> > sigprocmask(SIG_BLOCK, &s, ...);
> > ...
> > <perf event triggers>
> >
> > When the perf event triggers, while SIGTRAP is blocked, force_sig_perf()
> > will force the signal, but revert back to the default handler, thus
> > terminating the task.
> >
> > For other types of signals, is the assumption here that if user space
> > blocked the signal, it might not be able to handle it in the first
> > place?
> >
> > For SIGTRAP on perf events we found this makes the situation worse,
> > since the cause of the signal wasn't an error condition, but explicitly
> > requested monitoring. In this case, we do in fact want delivery of the
> > signal to user space even if the signal is blocked, i.e.
> > force_sig_perf() should be an unblockable forced synchronous signal to
> > user space!
> >
> > If there is no good reason to choose SIG_DFL, our preference would be to
> > allow this kind of "unblockable forced" signal to the user space handler
> > for force_sig_perf() -- with the caveat whoever requests SIGTRAP on perf
> > events must be able to provide a handler that can always run safely. But
> > we think that's better than crashing.
> >
> > The below patch would do what we want, but would like to first confirm
> > if this is "within spec".
> >
> > Thoughts?
> >
> > Thanks,
> > -- Marco
> >
> > ------ >8 ------
[...]
> > @@ -1332,7 +1335,8 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t,
> > ignored = action->sa.sa_handler == SIG_IGN;
> > blocked = sigismember(&t->blocked, sig);
> > if (blocked || ignored || (handler != HANDLER_CURRENT)) {
> > - action->sa.sa_handler = SIG_DFL;
> > + if (handler != HANDLER_UNBLOCK)
> > + action->sa.sa_handler = SIG_DFL;
> > if (handler == HANDLER_EXIT)
> > action->sa.sa_flags |= SA_IMMUTABLE;
> > if (blocked) {
> > @@ -1816,7 +1820,11 @@ int force_sig_perf(void __user *addr, u32 type, u64 sig_data)
> > info.si_perf_data = sig_data;
> > info.si_perf_type = type;
> >
> > - return force_sig_info(&info);
> > + /*
> > + * Delivering SIGTRAP on perf events must unblock delivery to not
> > + * kill the task, but attempt delivery to the user space handler.
> > + */
> > + return force_sig_info_to_task(&info, current, HANDLER_UNBLOCK);
>
> It seems that in this case we almost don't use any of the logic in
> force_sig_info_to_task(). It effectively reduces to the call to
> send_signal() protected by the lock. Maybe we should call something
> like do_send_sig_info() directly?

Unfortunately not -- without this:

[...]
blocked = sigismember(&t->blocked, sig);
if (blocked || ignored || (handler != HANDLER_CURRENT)) {
[...]
if (blocked) {
sigdelset(&t->blocked, sig);
recalc_sigpending_and_wake(t);
}
}
[...]

, it doesn't work if blocked==true. The alternative is to introduce
another helper, force_sig_info_unblockable() or something, but don't see
the benefit. Having it all in force_sig_info_to_task() seems cleaner
and we avoid replicating any unblock logic for forced signals.

Thanks,
-- Marco