linux-kernel - Re: [PATCH] kernel/signal: Remove no longer required irqsave/restore

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87sh77b5w7.fsf@xmission.com>
Date:   Fri, 04 May 2018 14:03:04 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-kernel@...r.kernel.org, tglx@...utronix.de,
        Anna-Maria Gleixner <anna-maria@...utronix.de>
Subject: Re: [PATCH] kernel/signal: Remove no longer required irqsave/restore

"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> writes:

> On Fri, May 04, 2018 at 12:17:20PM -0500, Eric W. Biederman wrote:
>> Sebastian Andrzej Siewior <bigeasy@...utronix.de> writes:
>> 
>> > On 2018-05-04 11:59:08 [-0500], Eric W. Biederman wrote:
>> >> Sebastian Andrzej Siewior <bigeasy@...utronix.de> writes:
>> >> > From: Anna-Maria Gleixner <anna-maria@...utronix.de>
>> > …
>> >> > This long-term fix has been made in commit 4abf91047cf ("rtmutex: Make >
>> >> > wait_lock irq safe") for different reason.
>> >> 
>> >> Which tree has this change been made in?  I am not finding the commit
>> >> you mention above in Linus's tree.
>> >
>> > I'm sorry, it should have been commit b4abf91047cf ("rtmutex: Make
>> > wait_lock irq safe").
>> 
>> Can you fix that in your patch description and can you also up the
>> description of rcu_read_unlock?
>> 
>> If we don't need to jump through hoops it looks very reasonable to
>> remove this unnecessary logic.  But we should fix the description
>> in rcu_read_unlock that still says we need these hoops.
>
> The hoops are still required for rcu_read_lock(), otherwise you
> get deadlocks between the scheduler and RCU in PREEMPT=y kernels.
> What happens with this patch (if I understand it correctly) is that the
> signal code now uses a different way of jumping through the hoops.
> But the hoops are still jumped through.

The patch changes:

local_irq_disable();
rcu_read_lock();
spin_lock();
rcu_read_unlock();

to:

rcu_read_lock();
spin_lock_irq();
rcu_read_unlock();

Now that I have a chance to relfect on it the fact that the patern
that is being restored does not work is scary.  As the failure has
nothing to do with lock ordering and people won't realize what is going
on.  Especially since the common rcu modes won't care.

So is it true that taking spin_lock_irq before calling rcu_read_unlock
is a problem because of rt_mutex_unlock()?  Or has b4abf91047cf ("rtmutex: Make
wait_lock irq safe") actually fixed that and we can correct the
documentation of rcu_read_unlock() ?  And fix __lock_task_sighand?

Eric