Messages in this thread | | | Date | Tue, 12 Feb 2019 17:50:23 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/2] signal: Always notice exiting tasks |
| |
On 02/12, Eric W. Biederman wrote: > > > Here I was trying for the simple minimal change and I hit this landmine. > > Which leaves me with the question of what should be semantics of signal > > handling after exit.
Yes, currently it is undefined. Even signal_pending() is random.
> > I think from dim memory of previous conversations the desired semantics > > look like: > > a) Ignore all signal state except for SIGKILL. > > b) Letting SIGKILL wake up the process should be sufficient.
signal_wake_up(true) to make fatal_signal_pending() == T, I think.
> Oleg any ideas on how to make PTRACE_EVENT_EXIT reliably killable?
My answer is very simple: PTRACE_EVENT_EXIT must not stop if the tracee was killed by the "real" SIGKILL (not by group_exit/etc), that is all. But this is another user-visible change, it can equally confuse, say, strace (albeit not too much iiuc).
But this needs another discussion.
> diff --git a/kernel/signal.c b/kernel/signal.c > index 99fa8ff06fd9..a1f154dca73c 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -2544,6 +2544,9 @@ bool get_signal(struct ksignal *ksig) > } > > fatal: > + /* No more signals can be pending past this point */ > + sigdelset(¤t->pending.signal, SIGKILL);
Well, this is very confusing. In fact, this is not really correct. Say, we should not remove the pending SIGKILL if we are going to call do_coredump(). This is possible if ptrace_signal() was called, or after is_current_pgrp_orphaned() returns false.
> + clear_tsk_thread_flag(current, TIF_SIGPENDING);
I don't understand this change, it looks irrelevant. Possibly makes sense, but this connects to "semantics of signal handling after exit".
OK, we need a minimal incremental fix for now. I'd suggest to replace
ksig->info.si_signo = signr = SIGKILL; if (signal_group_exit(signal)) goto fatal;
added by this patch with
if (__fatal_signal_pending(current)) { ksig->info.si_signo = signr = SIGKILL; sigdelset(¤t->pending.signal, SIGKILL); goto fatal; }
__fatal_signal_pending() is cheaper and looks more understandable.
Oleg.
| |