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

[Alan Stern]

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.

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

Alan Stern