linux-kernel - Re: [PATCH v5 3/6] signal: Add calculate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <87lg9fv2op.fsf@xmission.com>
Date:   Thu, 09 Aug 2018 13:02:14 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     <wen.yang99@....com.cn>
Cc:     <oleg@...hat.com>, <akpm@...ux-foundation.org>,
        <linux-kernel@...r.kernel.org>, <ma.jiang@....com.cn>,
        <torvalds@...ux-foundation.org>, <cheng.shengyu@....com.cn>,
        <zhong.weidong@....com.cn>
Subject: Re: [PATCH v5 3/6] signal: Add calculate_sigpending()

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

> EricW.Biederman <ebiederm@...ssion.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