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:     Oleg Nesterov <oleg@redhat.com>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        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: <20180724032419.20231-20-ebiederm@xmission.com>
        <CA+55aFxNijeenFd6o7fg3iJFqAjdTZLaFV2=Eaqbg1fDOzx26A@mail.gmail.com>
        <874lgo5xdg.fsf@xmission.com>
        <CA+55aFw_CTTRTZfUf1e9yg04X_djG3q7HrCG-XqnyQorpKjaTQ@mail.gmail.com>
        <87fu084cxj.fsf@xmission.com>
        <CA+55aFxUc3ttEAuRS7GKs9aTjTFw1poE80DASFSXudVQCuWpHg@mail.gmail.com>
        <87a7qg4bb3.fsf_-_@xmission.com>
        <CA+55aFxe+Ku78ZEvd_F2TD_3gTmLXw0kzo4HDE-ngkYfSHQfPw@mail.gmail.com>
        <87pnzc2upf.fsf@xmission.com> <87k1pk2cj9.fsf_-_@xmission.com>
        <20180726134143.GB32718@redhat.com>
Date:   Thu, 26 Jul 2018 09:42:13 -0500
In-Reply-To: <20180726134143.GB32718@redhat.com> (Oleg Nesterov's message of
        "Thu, 26 Jul 2018 15:41:43 +0200")
Message-ID: <87pnza6ou2.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 v3 20/20] signal: Don't restart fork when signals come in.
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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