linux-kernel - Re: memory barrier question

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, 20 Sep 2010 11:57:32 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	James.Bottomley@...senPartnership.com, benh@...nel.crashing.org,
	dhowells@...hat.com, linux-kernel@...r.kernel.org,
	linux-arch@...r.kernel.org
Subject: Re: memory barrier question

On Mon, Sep 20, 2010 at 11:25:04AM -0700, Paul E. McKenney wrote:
> On Mon, Sep 20, 2010 at 06:01:58PM +0200, Miklos Szeredi wrote:
> > On Sun, 19 Sep 2010, Paul E. McKenney wrote:
> > > > > Again, please put at least an ACCESS_ONCE() in.  Trivial to do now,
> > > > > possibly saving much pain and headache later on.
> > > > 
> > > > OK, lost you here.  ACCESS_ONCE() is only needed in certain situations
> > > > (like list traversal) because some compilers can reload cached values
> > > > across an explicit barrier (which isn't here).
> > > 
> > > ACCESS_ONCE() also tells the compiler not to try to guess.
> > 
> > If the code is written like this:
> > 
> > 	if (ACCESS_ONCE(dentry->d_inode)) {
> > 		blah = dentry->d_inode->i_some_field
> > 		...
> > 	}
> > 
> > does the compiler guarantee anything or does it need a full compiler
> > barrier to prevent reordering?
> 
> From what I understand, this could do strange things.  The compiler
> is forced to access dentry->d_inode for the "if" check, but would be
> free to use some previously fetched value for the assignment to "blah".
> Unless of course this code was under a lock that prevented any
> change to dentry->d_inode.
> 
> If the code is to execute in a lockless manner, I would instead suggest
> something like the following:
> 
> 	p = ACCESS_ONCE(dentry->d_inode);
> 	if (p) {
> 		blah = p->i_some_field
> 		...
> 	}
> 
> This would force the compiler to actually fetch dentry->d_inode
> and only then dereference it.

Are the initial check and the assignment to "blah" in different
functions or something?  If so, the following might be easier to
deal with:

	if (ACCESS_ONCE(dentry->d_inode)) {
		blah = ACCESS_ONCE(dentry->d_inode)->i_some_field
		...
	}

The compiler is forbidden to reorder volatile accesses (at least assuming
that they have a well-defined order, which they do in this case).  The
CPU is required to make successive accesses to the same memory location
be in order.

So as long as the only change to dentry->d_inode is a NULL-to-non-NULL
transition, the above should work, other than on DEC Alpha.

						Thanx, Paul

> This would -not- constrain the CPU in any way, but the only CPU that
> I know of that misbehaves in this case is DEC Alpha.
> 
> So my version of the above code would do what you expect on most CPUs,
> but really could fail on DEC Alpha.  If you don't believe me, please feel
> free to take a look at http://www.openvms.compaq.com/wizard/wiz_2637.html.
> 
> But do we really care about Alpha anymore?  (I can see it now... The
> Alpha portion of the kernel tree moves to staging...)
> 
> > Because that pattern is, again, pretty much all over the place.  Yeah
> > it can be rewritten but that's not always feasable since it's
> > difficult to audit, would possibly need extra function arguments,
> > etc...
> 
> Again, the pattern is OK if you are preventing the pointer from changing.
> 
> 							Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/