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

* RFC: Use of user space handler vs. SIG_DFL on forced signals
@ 2022-03-22 10:42 Marco Elver
  2022-03-22 13:25 ` Dmitry Vyukov
  2022-03-22 14:54 ` Eric W. Biederman
  0 siblings, 2 replies; 9+ messages in thread
From: Marco Elver @ 2022-03-22 10:42 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Dmitry Vyukov,
	linux-perf-users, linux-kernel

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);
 }
 
 /**
-- 
2.35.1.894.gb6a874cedc-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread