Re: [PATCH 0/2] fix vma->anon_vma check for per-VMA locking; fix anon_vma memory ordering

[Paul E. McKenney]

On Thu, Jul 27, 2023 at 11:44:02AM -0400, Alan Stern wrote:
> On Thu, Jul 27, 2023 at 03:57:47PM +0100, Will Deacon wrote:
> > On Thu, Jul 27, 2023 at 04:39:34PM +0200, Jann Horn wrote:
> 
> > > Assume that we are holding some kind of lock that ensures that the
> > > only possible concurrent update to "vma->anon_vma" is that it changes
> > > from a NULL pointer to a non-NULL pointer (using smp_store_release()).
> > > 
> > > 
> > > if (READ_ONCE(vma->anon_vma) != NULL) {
> > >   // we now know that vma->anon_vma cannot change anymore
> > > 
> > >   // access the same memory location again with a plain load
> > >   struct anon_vma *a = vma->anon_vma;
> > > 
> > >   // this needs to be address-dependency-ordered against one of
> > >   // the loads from vma->anon_vma
> > >   struct anon_vma *root = a->root;
> > > }
> 
> This reads a little oddly, perhaps because it's a fragment from a larger 
> piece of code.  Still, if I were doing something like this, I'd write it 
> as:
> 
> struct anon_vma *a;
> 
> a = READ_ONCE(vma->anon_vma);
> if (a != NULL) {
> 	struct anon_vma *root = a->root;
> 	...
> 
> thus eliminating the possibility of confusion from multiple reads of the 
> same address.
> 
> In this situation, the ordering of the two reads is guaranteed by the 
> address dependency.  And people shouldn't worry too much about using 
> that sort of ordering; RCU relies on it critically, all the time.

Agreed.  In contrast, control dependencies require quite a bit more care
and feeding, and are usually best avoided.

But even with the normal RCU address/data dependencies, it is possible
to get in trouble.  For but one example, comparing a pointer obtained
from rcu_dereference() to the address of a static structure is a good
way to break your address dependency.  (Just yesterday evening I talked
to someone who had spent quite a bit of time chasing one of these down,
so yes, this is quite real.)

> > > Is this fine? If it is not fine just because the compiler might
> > > reorder the plain load of vma->anon_vma before the READ_ONCE() load,
> > > would it be fine after adding a barrier() directly after the
> > > READ_ONCE()?
> > 
> > I'm _very_ wary of mixing READ_ONCE() and plain loads to the same variable,
> > as I've run into cases where you have sequences such as:
> > 
> > 	// Assume *ptr is initially 0 and somebody else writes it to 1
> > 	// concurrently
> > 
> > 	foo = *ptr;
> > 	bar = READ_ONCE(*ptr);
> > 	baz = *ptr;
> > 
> > and you can get foo == baz == 0 but bar == 1 because the compiler only
> > ends up reading from memory twice.
> > 
> > That was the root cause behind f069faba6887 ("arm64: mm: Use READ_ONCE
> > when dereferencing pointer to pte table"), which was very unpleasant to
> > debug.
> 
> Indeed, that's the sort of thing that can happen when plain accesses are 
> involved in a race.

Agreed.  Furthermore, it is more important to comment plain C-language
accesses to shared variables than to comment the likes of READ_ONCE().
"OK, tell me again exactly why you think the compiler cannot mess you
up here?"

							Thanx, Paul