Messages in this thread | | | Date | Wed, 20 Jul 2022 20:54:59 -0500 | From | "Serge E. Hallyn" <> | Subject | Re: [PATCH] sched: __fatal_signal_pending() should also check PF_EXITING |
| |
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
| |