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

[Joel Fernandes]

On Fri, Jul 28, 2023 at 8:44 AM Will Deacon <will@xxxxxxxxxx> wrote:
>
> On Thu, Jul 27, 2023 at 12:34:44PM -0400, Joel Fernandes wrote:
> > > On Jul 27, 2023, at 10:57 AM, Will Deacon <will@xxxxxxxxxx> wrote:
> > > On Thu, Jul 27, 2023 at 04:39:34PM +0200, Jann Horn wrote:
> > >> 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;
> > >> }
> > >>
> > >>
> > >> 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.
> >
> > Will, Unless I am missing something fundamental, this case is different though.
> > This case does not care about fewer reads. As long as the first read is volatile, the subsequent loads (even plain)
> > should work fine, no?
> > I am not seeing how the compiler can screw that up, so please do enlighten :).
>
> I guess the thing I'm worried about is if there is some previous read of
> 'vma->anon_vma' which didn't use READ_ONCE() and the compiler kept the
> result around in a register. In that case, 'a' could be NULL, even if
> the READ_ONCE(vma->anon_vma) returned non-NULL.

If I can be a bit brave enough to say -- that appears to be a compiler
bug to me. It seems that the compiler in such an instance violates the
"Sequential Consistency Per Variable" rule? I mean if it can't even
keep SCPV true for a same memory-location load (plain or not) for a
sequence of code, how can it expect the hardware to.

In other words, with that kind of caching, the value of the variable
goes back in time which will be tough luck for even a fully ordered
sequentially-consistent processor!!!

> The crux of the issue is that the compiler can break read-after-read
> ordering if you don't use READ_ONCE() consistently. Sadly, judging by
> the other part of the thread from Nadav, it's fiddly to fix this without
> wrecking the codegen.

Right. Thanks to you and others for sharing your informative
perspective as always,

 - Joel