Messages in this thread | | | From | (Eric W. Biederman) | Date | Thu, 26 Jul 2018 09:42:13 -0500 | Subject | Re: [PATCH v3 20/20] signal: Don't restart fork when signals come in. |
| |
Oleg Nesterov <oleg@redhat.com> writes:
> On 07/24, Eric W. Biederman wrote: >> >> @@ -1979,6 +1983,8 @@ static __latent_entropy struct task_struct *copy_process( >> attach_pid(p, PIDTYPE_TGID); >> attach_pid(p, PIDTYPE_PGID); >> attach_pid(p, PIDTYPE_SID); >> + p->signal->shared_pending.signal = delayed.signal; > > Again, in this case we do not hold p->sighand->siglock (unless CLONE_SIGHAND), > so this should be done before list_add_tail/attach_pid above. Plus we need some > sort of barrier. > > Or we can do > > if (!CLONE_SIGHAND) > spin_lock_nested(child->siglock); > > at the start of "if (likely(p->pid))" block.
Good point. We want to exclude races between new signals comming in and gathering the information from the old queued signals.
I will take a look.
>> --- a/kernel/signal.c >> +++ b/kernel/signal.c >> @@ -1123,6 +1123,15 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, >> out_set: >> signalfd_notify(t, sig); >> sigaddset(&pending->signal, sig); >> + >> + /* Let multiprocess signals appear after on-going forks */ >> + if (type > PIDTYPE_TGID) { >> + struct multiprocess_signals *delayed; >> + hlist_for_each_entry(delayed, &t->signal->multiprocess, node) { >> + sigaddset(&delayed->signal, sig); > > This is not enough, I think... > > Suppose you send SIGSTOP and then SIGCONT to some process group. The 1st SIGSTOP > will be queued correctly, but the 2nd SIGCONT won't flush the pending SIGSTOP, you > need to modify prepare_signal() too.
Good point. We can't have both SIGCONT and a stop signal (SIGSTOP or SIGTSTP) enqueued at the same time. And there is something in the prepare_signal code about parent notifications as well.
I will look up the fine points of SIGCONT and SIGSTOP interaction and see what we should be doing here.
Sigh. I thought this was going to be as simple as the sequence counter but this looks a tad more complicated.
Are the earlier patches looking ok to you?
Eric
| |