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:     <wen.yang99@zte.com.cn>
Cc:     <oleg@redhat.com>, <akpm@linux-foundation.org>,
        <linux-kernel@vger.kernel.org>, <ma.jiang@zte.com.cn>,
        <torvalds@linux-foundation.org>, <cheng.shengyu@zte.com.cn>,
        <zhong.weidong@zte.com.cn>
References: <201808091624383651898@zte.com.cn>
Date:   Thu, 09 Aug 2018 13:02:14 -0500
In-Reply-To: <201808091624383651898@zte.com.cn> (wen's message of "Thu, 9 Aug
        2018 16:24:38 +0800 (CST)")
Message-ID: <87lg9fv2op.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 v5 3/6] signal: Add calculate_sigpending()
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

<wen.yang99@zte.com.cn> writes:

> EricW.Biederman <ebiederm@xmission.com> wrote:
>> Add a function calculate_sigpending to test to see if any signals are
>> pending for a new task immediately following fork. Signals have to
>> happen either before or after fork. Today our practice is to push
>> all of the signals to before the fork, but that has the downside that
>> frequent or periodic signals can make fork take much much longer than
>> normal or prevent fork from completing entirely.
>> 
>
>> + calculate_sigpending();
>> }
>> /*
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index dddbea558455..1e06f1eba363 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -172,6 +172,17 @@ void recalc_sigpending(void)
>> }
>> +void calculate_sigpending(void)
>> +{
>> + /* Have any signals or users of TIF_SIGPENDING been delayed
>> + * until after fork?
>> + */
>> + spin_lock_irq(&current->sighand->siglock);
>> + set_tsk_thread_flag(current, TIF_SIGPENDING);
>> + recalc_sigpending();
>> + spin_unlock_irq(&current->sighand->siglock);
>> +}
>> +
>
> The new function calculate_sigpending is similar to  recalc_sigpending,
> but recalc_sigpending has no spin_lock_irq(&current->sighand->siglock) in it.
> This gives recalc_sigpending more flexibility, 
> we may use spin_lock_irq or spin_lock_irqsave before recalc_sigpending .
> eg: 
>
> static int autofs4_write(struct autofs_sb_info *sbi,
>              struct file *file, const void *addr, int bytes)
> {
>   ...
>         spin_lock_irqsave(&current->sighand->siglock, flags);
>         sigdelset(&current->pending.signal, SIGPIPE);
>         recalc_sigpending();
>         spin_unlock_irqrestore(&current->sighand->siglock, flags);
>   ... 
> }
>
> or:
> void kernel_sigaction(int sig, __sighandler_t action)
> {
>     spin_lock_irq(&current->sighand->siglock);
> ...
>         recalc_sigpending();
> ...   
>     spin_unlock_irq(&current->sighand->siglock);
> }
>
>
> But calculate_sigpending is currently hardwired to call spin_lock_irq.

calculate_sigpending really only exists to keep the code comprehensible.

It is only ever expected to be called in exactly one place so the lack
of flexibility should not be a problem.  Further the use of irqsave
is discouraged unless it is necessary.

The irqsave in autofs_write actually looks like a misfeature.  We take
a mutex a few lines earlier, so we know that irqs are enabled.  Saving
and restoring them is uncessary work.  Further unless I am missing
something that code path should be calling kernel_dequeue_signal, to
ensure that any siginfo associated with that SIGPIPE gets dequeued.

Eric