Re: [GIT PULL] overlay filesystem v25

[Al Viro <viro@ZenIV.linux.org.uk>]

On Mon, Oct 27, 2014 at 08:59:01AM -0700, Paul E. McKenney wrote:

> Indeed, life is hard here.  Keep in mind that lock acquisition is not
> guaranteed to prevent prior operations from being reordered into the
> critical section, possibly as follows:
> 
> 	CPU1:
> 	  grab lock
> 	  if (!global)
> 	      global = p;
> 	  /* Assume all of CPU2's accesses happen here. */
> 	  p->foo = 1;

A bit of context: p is a local pointer to struct file here and alloc is
opening it...

> This clearly allows CPU2 to execute as follows:
> 
> 	CPU2:
> 	  p = global; /* gets p */
> 	  if (p) /* non-NULL */
> 	  	q = p->foo; /* might not be 1 */
> 
> Not only that, on DEC Alpha, even if CPU1's accesses are ordered, CPU2's
> accesses can be misordered.  You need rcu_dereference() or the combination
> of ACCESS_ONCE() and smp_read_barrier_depends() to avoid this issue.
> As always, see http://www.openvms.compaq.com/wizard/wiz_2637.html for
> more info.
> 
> So no, there is no guarantee.  I am assuming that the lock grabbed by
> CPU1 guards all assignments to "global", otherwise the code needs further
> help.  I am further assuming that the memory pointed to by CPU1's "p"
> is inaccessible to any other CPU, as in CPU1 just allocated the memory.
> Otherwise, the assignment "p->foo = 1" is questionable.  And finally,
> I am assuming that p->foo stays constant once it has been made
> accessible to readers.
> 
> But the following should work:
> 
> 	CPU1:
> 	  p->foo = 1;  /* Assumes p is local. */
> 	  smp_mb__before_spinlock();
> 	  grab lock
> 	  if (!global)  /* Assumes lock protects all assignments to global. */
> 	      global = p;
> 
> 	CPU2:
> 	  p = rcu_dereference(global);
> 	  if (p)
> 	     q = p->foo; /* Assumes p->foo constant once visible to readers. */
> 	     		 /* Also assumes p and q are local. */
> 
> If the assumptions called out in the comments do not hold, you at least
> need ACCESS_ONCE(), and perhaps even more synchronization.  For more info
> on ACCESS_ONCE(), Jon's LWN article is at http://lwn.net/Articles/508991/.

First of all, this "p->foo = 1" is a shorthand for initialization of
struct file done by opening a file.  What you are saying is that it
can leak past the point where we stick a pointer to freshly opened
file into a place where another CPU can see it, but not past the
barrier in dropping the lock, right?

And you are suggesting rcu_dereference() as a way to bring the required
barriers in.  Ouch...  The names are really bad, but there's another
fun issue - rcu_dereference brings in sparse noise.  Wouldn't direct use
of smp_read_barrier_depends() be cleaner, anyway?