Messages in this thread | | | From | (Eric W. Biederman) | Date | Tue, 10 Jul 2018 11:00:55 -0500 | Subject | Re: [Bug 200447] infinite loop in fork syscall |
| |
Oleg Nesterov <oleg@redhat.com> writes:
> On 07/09, Linus Torvalds wrote: >> >> But the patch was written for testing and feedback more than anything >> else. Comments? > > see my reply on bugzilla. Can't we add lkml? Done.
> Perhaps we can do another change? Not sure it is actually better, but I think > it is always good to discuss the alternatives. > > 1. Once again, we turn "int group" argument into thread/process/pgid enum, and > change kill_pgrp/tty_signal_session_leader to pass "group = pgid", imo this > makes sense in any case.
I agree. There are a lot more multi-process signals cases than handled in Linus's patch. I believe this will clean that up.
> 2. To simplify, lets suppose we add the new PF_INFORK flag. Yes, this is bad, > we can do better. I think we can simply add "struct hlist_head forking_threads" > into signal_struct, so complete_signal() can just do hlist_for_each_entry() > rather than for_each_thread() + PF_INFORK check. We don't even need a new > member in task_struct.
We still need the distinction between multi-process signals and single process signals (which is the hard part). For good performance of signal delivery to multi-threaded tasks we still need a new member in signal_struct. Plus it is a bit more work to update the list or even walk the list than a sequence counter.
So I think adding a sequence counter to let us know about multiprocess signals is the local optimum.
If we ever need to remove the restart case entirely from fork and queue all new multi-process signals for the newly created task. Going through the list of forking
> > 3. copy_process() can simply block/unblock all signals (except KILL/STOP), see > the "patch" below.
All signals are effectively blocked for the duration of the fork for the calling task. Where we get into trouble and where we need a fix for correctness is that another thread can dequeue the signal. Blocking signals of the forking task does not change that.
I think that reveals another bug in our current logic. For blocked multi-process signals we don't ensure they get delivered to both the parent and the child if the signal logically comes in after the fork.
Multi-threaded b0rked ness and blocked signal b0rkenness, plus periodic timers making for take forever for no good reason. This business of ensuring a signal is logically delvered before or after a fork is tricky.
Eric
> diff --git a/kernel/fork.c b/kernel/fork.c > index 9440d61..652ef09 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1591,6 +1591,23 @@ static __latent_entropy struct task_struct *copy_process( > int retval; > struct task_struct *p; > > + spin_lock_irq(¤t->sighand->siglock); > + recalc_sigpending_tsk(current); > + if (signal_pending(current)) { > + retval = -ERESTARTNOINTR; > + } else { > + // Yes, yes, this is wrong, just to explain the idea. > + // We should not block SIGKILL, we need to clear PF_INFORK > + // and restore ->blocked in error paths, we need helper(s). > + retval = 0; > + current->flags |= PF_INFORK; > + current->saved_sigmask = current->blocked; > + sigfillset(¤t->blocked); > + } > + spin_unlock_irq(¤t->sighand->siglock); > + if (retval) > + return retval; > + > /* > * Don't allow sharing the root directory with processes in a different > * namespace > @@ -1918,8 +1935,14 @@ static __latent_entropy struct task_struct *copy_process( > * A fatal signal pending means that current will exit, so the new > * thread can't slip out of an OOM kill (or normal SIGKILL). > */ > - recalc_sigpending(); > - if (signal_pending(current)) { > + // check signal_pending() before __set_task_blocked() which does > + // recalc_sigpending(). Again, this is just to explain the idea, > + // __set_task_blocked() is not exported, it is suboptimal, we can > + // do better. > + bool eintr = signal_pending(); > + current->flags &= ~PF_INFORK; > + __set_task_blocked(current, ¤t->saved_sigmask); > + if (eintr) { > retval = -ERESTARTNOINTR; > goto bad_fork_cancel_cgroup; > } > diff --git a/kernel/signal.c b/kernel/signal.c > index 8d8a940..3433e66 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -900,6 +900,14 @@ static void complete_signal(int sig, struct task_struct *p, int group) > struct signal_struct *signal = p->signal; > struct task_struct *t; > > + // kill_pgrp/etc. > + if (group == 2) { > + for_each_thread(p, t) { > + if (p->flags & PF_INFORK && !sigismember(&p->saved_sigmask, sig)) > + signal_wake_up(t, 0); > + } > + } > + > /* > * Now find a thread we can wake up to take the signal off the queue. > *
| |