linux-kernel - Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected

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: <alpine.DEB.2.11.1601141812270.3575@nanos>
Date:	Thu, 14 Jan 2016 18:43:16 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
cc:	Sasha Levin <sasha.levin@...cle.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected

On Wed, 13 Jan 2016, Paul E. McKenney wrote:
> On Wed, Jan 13, 2016 at 10:05:49AM +0100, Thomas Gleixner wrote:
> > We can fix that particular issue in the posix-timer code by making the
> > locking symetric:
> > 
> > 	rcu_read_lock();
> > 	spin_lock_irq(timer->lock);
> > 
> > ...
> > 
> > 	spin_unlock_irq(timer->lock);
> > 	rcu_read_unlock();
> > 
> > instead of:
> > 
> > 	rcu_read_lock();
> > 	spin_lock_irq(timer->lock);
> > 	rcu_read_unlock();
> > 
> > ...
> > 
> > 	spin_unlock_irq(timer->lock);
> > 
> > But the question is, whether this is the only offending code path in tree. We
> > can avoid the hassle by making rtmutex->wait_lock irq safe.
> > 
> > Thoughts?
> 
> Given that the lock is disabling irq, I don't see a problem with
> extending the RCU read-side critical section to cover the entire
> irq-disabled region.

I cannot follow here. What would be different if the lock would not disable
irqs? I mean you can get preempted right after rcu_read_lock() before
acquiring the spinlock.

> Your point about the hassle of finding and fixing all the other instances of
> this sort is well taken, however.

Right. We have the pattern 

     rcu_read_lock();
     x = lookup();
     if (x)
	   keep_hold(x)
     rcu_read_unlock();
     return x;

all over the place. Now that keep_hold() can be everything from a refcount to
a spinlock and I'm not sure that we can force stuff depending on the mechanism
to be completely symetric. So we are probably better off by making that rcu
unlock machinery more robust.

Thanks,

	tglx