Messages in this thread | | | From | Linus Torvalds <> | Date | Tue, 24 Jul 2018 10:27:58 -0700 | Subject | Re: [PATCH 20/20] signal: Don't restart fork when signals come in. |
| |
This is completely broken.
On Mon, Jul 23, 2018 at 8:27 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > diff --git a/kernel/fork.c b/kernel/fork.c > index 6c358846a8b8..6ee5822f0085 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1602,6 +1603,24 @@ static __latent_entropy struct task_struct *copy_process( > { > int retval; > struct task_struct *p; > + struct multiprocess_signals delayed; > + > + /* > + * Force any signals received before this point to be delivered > + * before the fork happens. Collect up signals sent to multiple > + * processes that happen during the fork and delay them so that > + * they appear to happen after the fork. > + */ > + sigemptyset(&delayed.signal); > + INIT_HLIST_NODE(&delayed.node); > + > + spin_lock_irq(¤t->sighand->siglock); > + if (!(clone_flags & CLONE_THREAD)) > + hlist_add_head(&delayed.node, ¤t->signal->multiprocess);
Here you add the entry to the multiprocess list.
> + recalc_sigpending(); > + spin_unlock_irq(¤t->sighand->siglock); > + if (signal_pending(current)) > + return ERR_PTR(restart_syscall());
.. and here you return with the list entry still there, pointing to the stack that you now no longer use.
The same is true of *all* the error cases, because the only point you remove it is for the success case:
> @@ -1979,6 +1982,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; > + hlist_del(&delayed.node);
So for all the error cases, you leave a dangling pointer to the current stack in that signal handler, and then return an error.
Guaranteed stack and list corruption.
Linus
| |