Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 22 Mar 2022 14:53:10 +0100
From:   Marco Elver <elver@google.com>
To:     Dmitry Vyukov <dvyukov@google.com>
Cc:     "Eric W. Biederman" <ebiederm@xmission.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: RFC: Use of user space handler vs. SIG_DFL on forced signals
Message-ID: <YjnUxv4JVqGLLVNe@elver.google.com>
References: <Yjmn/kVblV3TdoAq@elver.google.com>
 <CACT4Y+Zusg3dMaOZjexExcsyXUaU3Oo9MqgThWd2FG_Jzo=esQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CACT4Y+Zusg3dMaOZjexExcsyXUaU3Oo9MqgThWd2FG_Jzo=esQ@mail.gmail.com>
User-Agent: Mutt/2.1.4 (2021-12-11)
Precedence: bulk

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