Re: [PATCH] sched: __fatal_signal_pending() should also check PF_EXITING

["Serge E. Hallyn" <serge@hallyn.com>]

On Wed, Jul 20, 2022 at 02:58:42PM -0600, Tycho Andersen wrote:
> On Wed, Jul 20, 2022 at 10:03:28AM -0500, Serge E. Hallyn wrote:
> > On Wed, Jul 13, 2022 at 11:53:05AM -0600, Tycho Andersen wrote:
> > > The wait_* code uses signal_pending_state() to test whether a thread has
> > > been interrupted, which ultimately uses __fatal_signal_pending() to detect
> > > if there is a fatal signal.
> > > 
> > > When a pid ns dies, it does:
> > > 
> > >     group_send_sig_info(SIGKILL, SEND_SIG_PRIV, task, PIDTYPE_MAX);
> > > 
> > > for all the tasks in the pid ns. That calls through:
> > > 
> > >     group_send_sig_info() ->
> > >       do_send_sig_info() ->
> > >         send_signal_locked() ->
> > >           __send_signal_locked()
> > > 
> > > which does:
> > > 
> > >     pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;
> > > 
> > > which puts sigkill in the set of shared signals, but not the individual
> > > pending ones. When complete_signal() is called at the end of
> > > __send_signal_locked(), if the task already had PF_EXITING (i.e. was
> > > already waiting on something in its fd closing path like a fuse flush),
> > > complete_signal() will not wake up the thread, since wants_signal() checks
> > > PF_EXITING before testing for SIGKILL.
> > > 
> > > If tasks are stuck in a killable wait (e.g. a fuse flush operation), they
> > > won't see this shared signal, and will hang forever, since TIF_SIGPENDING
> > > is set, but the fatal signal can't be detected. So, let's also look for
> > > PF_EXITING in __fatal_signal_pending().
> > > 
> > > Signed-off-by: Tycho Andersen <tycho@tycho.pizza>
> > 
> > Cool, thanks for nailing this down!
> > 
> > I assume you've been running this on some boxes with no weird effects?
> 
> Yes, but I haven't tested all the paths.
> 
> > > ---
> > >  include/linux/sched/signal.h | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> > > index cafbe03eed01..c20b7e1d89ef 100644
> > > --- a/include/linux/sched/signal.h
> > > +++ b/include/linux/sched/signal.h
> > > @@ -402,7 +402,8 @@ static inline int signal_pending(struct task_struct *p)
> > >  
> > >  static inline int __fatal_signal_pending(struct task_struct *p)
> > >  {
> > > -	return unlikely(sigismember(&p->pending.signal, SIGKILL));
> > > +	return unlikely(sigismember(&p->pending.signal, SIGKILL) ||
> > > +			p->flags & PF_EXITING);
> > 
> > Looking around at the callers this does seem safe, but the name does
> > now seem misleading.  Should this be renamed to something like
> > exiting_or_fatal_signal_pending()?  
> 
> This is why I like my original patch better: it is just expanding the
> set of signals to include the shared signals, which are indeed still
> fatal pending signals for the task. I don't really understand Eric's
> argument about kernel threads ignoring SIGKILL, since kernel threads

Oh - I didn't either - checking the sigkill in shared signals *seems*
legit if they can be put there - but since you posted the new patch I
assumed his reasoning was clear to you.  I know Eric's busy, cc:ing Oleg
for his interpretation too.

> can still ignore SIGKILL just fine after this patch.
> 
> But yes, assuming Eric is ok with this venison. I can send a v2 with
> the name change as you suggest.
> 
> Thanks for looking.
> 
> Tycho