linux-kernel - Re: [RFC][PATCH] mips: Fix arch_spin

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]

Date:	Mon, 1 Feb 2016 22:44:33 -0800
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Boqun Feng <boqun.feng@...il.com>
Cc:	Will Deacon <will.deacon@....com>,
	Peter Zijlstra <peterz@...radead.org>,
	"Maciej W. Rozycki" <macro@...tec.com>,
	David Daney <ddaney@...iumnetworks.com>,
	Måns Rullgård <mans@...sr.com>,
	Ralf Baechle <ralf@...ux-mips.org>,
	linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org
Subject: Re: [RFC][PATCH] mips: Fix arch_spin_unlock()

On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> Hi Paul,
> 
> On Mon, Feb 01, 2016 at 07:54:58PM -0800, Paul E. McKenney wrote:
> > On Mon, Feb 01, 2016 at 01:56:22PM +0000, Will Deacon wrote:
> > > On Fri, Jan 29, 2016 at 02:22:53AM -0800, Paul E. McKenney wrote:
> > > > On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> > > > > On Thu, Jan 28, 2016 at 02:31:31PM -0800, Paul E. McKenney wrote:
> > > > 
> > > > [ . . . ]
> > > > 
> > > > > > For Linux in general, this is a question: How strict do we want to be
> > > > > > about matching the type of write with the corresponding read?  My
> > > > > > default approach is to initially be quite strict and loosen as needed.
> > > > > > Here "quite strict" might mean requiring an rcu_assign_pointer() for
> > > > > > the write and rcu_dereference() for the read, as opposed to (say)
> > > > > > ACCESS_ONCE() for the read.  (I am guessing that this would be too
> > > > > > tight, but it makes a good example.)
> > > > > > 
> > > > > > Thoughts?
> > > > > 
> > > > > That sounds broadly sensible to me and allows rcu_assign_pointer and
> > > > > rcu_dereference to be used as drop-in replacements for release/acquire
> > > > > where local transitivity isn't required. However, I don't think we can
> > > > > rule out READ_ONCE/WRITE_ONCE interactions as they seem to be used
> > > > > already in things like the osq_lock (albeit without the address
> > > > > dependency).
> > > > 
> > > > Agreed.  So in the most strict case that I can imagine anyone putting
> > > > up with, we have the following pairings:
> > > 
> > > I think we can group these up:
> > > 
> > > Locally transitive:
> > > 
> > > > o	smp_store_release() -> smp_load_acquire() (locally transitive)
> > > 
> > > Locally transitive chain termination:
> > > 
> > > (i.e. these can't be used to extend a chain)
> > 
> > Agreed.
> > 
> > > > o	smp_store_release() -> lockless_dereference() (???)
> > > > o	rcu_assign_pointer() -> rcu_dereference()
> > > > o	smp_store_release() -> READ_ONCE(); if
> 
> Just want to make sure, this one is actually:
> 
> o	smp_store_release() -> READ_ONCE(); if ;<WRITE_ONCE()>
> 
> right? Because control dependency only orders READ->WRITE.

Yep!

> If so, do we also need to take the following pairing into consideration?
> 
> o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>

It looks like we will be restricing smp_rmb() and smp_wmb() to pairwise
scenarios only.  So no transitivity in any scenarios involving these
two primitives.

> > I am OK with the first and last, but I believe that the middle one
> > has real use cases.  So the rcu_assign_pointer() -> rcu_dereference()
> > case needs to be locally transitive.
> 
> Hmm... I don't think we should differ rcu_dereference() and
> lockless_dereference(). One reason: list_for_each_entry_rcu() are using
> lockless_dereference() right now, which means we used to think
> rcu_dereference() and lockless_dereference() are interchangeable, right?

They use the same instructions, if that is what you mean, so intuitively
they should behave the same.  I don't feel all that strongly either way.
But where there is uncertainty, we should -not- assume ordering.  It is
easy to tighten the rules later, but hard to loosen them.

That might change if tools that automatically analyze usage patterns
in raw code, but we do not yet have such tools.

							Thanx, Paul

> Besides, Will, what's the reason of having a locally transitive chain
> termination? Because on some architectures RELEASE->DEPENDENCY pairs may
> not be locally transitive?
> 
> Regards,
> Boqun
> 
> > > Globally transitive:
> > > 
> > > > o	smp_mb(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)
> > > > o	synchronize_rcu(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)
> > > 
> > > RCU:
> > > 
> > > > o	synchronize_rcu(); WRITE_ONCE() -> rcu_read_lock(); READ_ONCE()
> > > > 		(strange and wonderful properties)
> > 
> > Agreed.
> > 
> > > > Seem reasonable, or am I missing some?
> > > 
> > > Looks alright to me.
> > 
> > So I have some litmus tests to generate.  ;-)
> > 
> > 							Thnax, Paul
> >