Messages in this thread | | | From | (Eric W. Biederman) | Date | Tue, 12 Feb 2019 21:58:51 -0600 | Subject | Re: [PATCH 1/2] signal: Always notice exiting tasks |
| |
Oleg Nesterov <oleg@redhat.com> writes:
> 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.
Yes. Quite.
I will just point out that as described that logic will rebreak Ivan's program.
>> 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.
I don't see bugs in it. But it is certainly subtle and that is not what is needed right now.
The subtlety is that we will never have a per thread SIGKILL pending unless signal_group_exit is true. So removing when it is not there is harmless.
>> + 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".
As on the other the location is too subtle for the regression fix.
The primary motivation is that dequeue_signal calls recalc_sigpending. And in the common case that will result clearing the TIF_SIGPENDING which will result in signal_pending being false.
I have not found a location that cares enough to cause a misbehavior if we don't clear TIF_SIGPENDING but it is a practical change and there might be. So if the word of the day is be very conservative and avoid landminds I expect we need the clearing of TIF_SIGPENDING.
Hmm. Probably using recalc_sigpending() now that I think about it.
> 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.
I definitely agree that it is much less likely to cause a problem if we move all of the work before jumping to fatal.
The cost of both __fatal_signal_pending and signal_group_exit is just a cache line read. So not a big deal wither way.
On the other hand __fatal_signal_pending as currently implemented is insanely subtle and arguably a bit confusing. It tests for a SIGKILL in the current pending sigset, to discover the signal group property of if a process as started exiting.
In the long run we need our data structures not to be subtle and tricky to use. To do that we need a test of something in signal_struct because it is a per signal group property. Further we need to remove the abuse of the per thread SIGKILL.
Since signal_group_exit always implies __fatal_signal_pending in this case and the reverse. I see no reason to use a function that requires we maintain a huge amount of confusing and unnecessary machinery to keep working.
All of that plus the signal_group_exit test has been tested and shown to fix an ignored SIGKILL and the only practical problem is it doesn't do one or two little things that dequeue_signal has done that made it impossible to stop in PTRACE_EVENT_EXIT.
So for the regression fix let's just do the few little things that dequeue_signal used to do. That gives us a strong guarantee that nothing else was missed.
Eric
| |