Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Oleg Nesterov <oleg@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Wen Yang <wen.yang99@zte.com.cn>, majiang <ma.jiang@zte.com.cn>
References: <87efft5ncd.fsf_-_@xmission.com>
        <20180724032419.20231-20-ebiederm@xmission.com>
        <CA+55aFxNijeenFd6o7fg3iJFqAjdTZLaFV2=Eaqbg1fDOzx26A@mail.gmail.com>
Date:   Tue, 24 Jul 2018 12:58:35 -0500
In-Reply-To: <CA+55aFxNijeenFd6o7fg3iJFqAjdTZLaFV2=Eaqbg1fDOzx26A@mail.gmail.com>
        (Linus Torvalds's message of "Tue, 24 Jul 2018 10:27:58 -0700")
Message-ID: <874lgo5xdg.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 20/20] signal: Don't restart fork when signals come in.
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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(&current->sighand->siglock);
>> +       if (!(clone_flags & CLONE_THREAD))
>> +               hlist_add_head(&delayed.node, &current->signal->multiprocess);
>
> Here you add the entry to the multiprocess list.
>
>> +       recalc_sigpending();
>> +       spin_unlock_irq(&current->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:

Yes you are quite right.   Easy enough to fix, but it definitely needs
to be fixed.

I will respin.

Eric


>> @@ -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