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
