Messages in this thread | | | From | Dmitry Vyukov <> | Date | Tue, 22 Mar 2022 14:25:59 +0100 | Subject | Re: RFC: Use of user space handler vs. SIG_DFL on forced signals |
| |
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 ------ > > From: Marco Elver <elver@google.com> > Date: Mon, 21 Mar 2022 22:18:09 +0100 > Subject: [PATCH RFC] signal: Always unblock signal to user space for > force_sig_perf() > > With SIGTRAP on perf events, we have encountered termination of > processes due to user space attempting to block delivery of SIGTRAP. > Consider this case: > > <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. > > With forced signals, the whole premise of sigprocmask() is invalidated > since the signal is still delivered, only to the default signal handler. > The assumption here is that if user space blocked the signal, it might > not be able to handle it in the first place. > > However, 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. > > Fixes: 97ba62b27867 ("perf: Add support for SIGTRAP on perf events") > Signed-off-by: Marco Elver <elver@google.com> > --- > kernel/signal.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/kernel/signal.c b/kernel/signal.c > index 38602738866e..04b7a178a5f3 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -1303,6 +1303,7 @@ int do_send_sig_info(int sig, struct kernel_siginfo *info, struct task_struct *p > > enum sig_handler { > HANDLER_CURRENT, /* If reachable use the current handler */ > + HANDLER_UNBLOCK, /* Use the current handler even if blocked */ > HANDLER_SIG_DFL, /* Always use SIG_DFL handler semantics */ > HANDLER_EXIT, /* Only visible as the process exit code */ > }; > @@ -1311,9 +1312,11 @@ enum sig_handler { > * Force a signal that the process can't ignore: if necessary > * we unblock the signal and change any SIG_IGN 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. > + * Note: If we unblock the signal and handler != HANDLER_UNBLOCK, 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. If handler is > + * HANDLER_UNBLOCK, user space will always receive the signal and is expected to > + * provide a handler that is safe in all contexts. > * > * We don't want to have recursive SIGSEGV's etc, for example, > * that is why we also clear SIGNAL_UNKILLABLE. > @@ -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?
> } > > /** > -- > 2.35.1.894.gb6a874cedc-goog >
| |