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@vger.kernel.org, Wen Yang <wen.yang99@zte.com.cn>,
        majiang <ma.jiang@zte.com.cn>
References: <877em2jxyr.fsf_-_@xmission.com>
        <20180711024459.10654-11-ebiederm@xmission.com>
        <20180711141456.GA6636@redhat.com> <87h8l5g3qi.fsf@xmission.com>
        <20180712134220.GA15075@redhat.com>
Date:   Thu, 12 Jul 2018 12:11:53 -0500
In-Reply-To: <20180712134220.GA15075@redhat.com> (Oleg Nesterov's message of
        "Thu, 12 Jul 2018 15:42:20 +0200")
Message-ID: <87601k5qg6.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: [RFC][PATCH 11/11] signal: Ignore all but multi-process signals that come in during fork.
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Oleg Nesterov <oleg@redhat.com> writes:

> On 07/11, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> >> -	recalc_sigpending();
>> >> -	if (signal_pending(current)) {
>> >> +	if (read_seqcount_retry(&current->signal->multi_process_seq, seq) ||
>> >> +	    fatal_signal_pending(current)) {
>> >>  		retval = -ERESTARTNOINTR;
>> >>  		goto bad_fork_cancel_cgroup;
>> >
>> > So once again, I think this is not right, see the discussion on
>> > bugzilla.
>>
>> I am trying to dig through and understand your concerns.  I am having
>> difficulty understanding your concerns.
>>
>> Do the previous patches look good to you?
>
> Yes, yes, personally I like 1-10 after a quick glance. I'll try to read this
> series carefully later, but I don't think I will find something really
> wrong.

Good.  Then I will consider those acked by both you and Linus.

Oleg do you mind if I add:
Acked-by: Oleg Nesterov <oleg@redhat.com>

To those patches?

>> If I understand you correctly.  Your concern is that since we added the:
>>
>> 	recalc_sigpending();
>>         if (signal_pending(current))
>>         	return -ERESTARTNOINTR;
>>
>> Other (non-signal) code such as the freezer has come to depend upon that
>> test.  Changing the test in the proposed way will allow the new child to
>> escape the freezer, as it is not guaranteed the new child will be
>> frozen.
>
> Yes.


>> It seems reasonable to look at other things that set TIF_SIGPENDING and
>> see if any of them are broken by the fork changes.
>
> Again, please look at do_signal_stop(). If it was the source of signal_pending(),
> copy_process() should fail. Or we should update the new thread to participate in
> group-stop, but then we need to set TIF_SIGPENDING, copy the relevant part of
> current->jobctl, and increment ->group_stop_count at least.

Hmm.  That is an interesting twist.

In general for do_signal_stop is fine as long as we have the
recalc_sigpending at the start of fork.

But yes.  What happens when it isn't a fork but it is a clone.  Signals
that affect the entire thread group (STOP CLONE) are very interesting
from this perspective.

Same issue as with fork, but different scope.

Eric