Re: [GIT PULL] overlay filesystem v25

From: Paul E. McKenney
Date: Mon Oct 27 2014 - 13:40:21 EST


On Mon, Oct 27, 2014 at 05:28:01PM +0000, Al Viro wrote:
> 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...

OK, good to know. ;-)

> > 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?

Exactly!

I should also add that smp_mb__before_spinlock() implies smp_wmb(), but
nothing more. But that is OK because smp_wmb() suffices in this case.

> 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?

Code making direct use of smp_read_barrier_depends() is harder to read,
in my experience, but good point on the sparse noise. Maybe a new
lockless_dereference() primitive? Maybe something like the following?
(Untested, probably does not even build.)

#define lockless_dereference(p) \
({ \
typeof(*p) *_________p1 = ACCESS_ONCE(p); \
smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
_________p1; \
})

Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/