Re: [PATCH] mm/mmzone.c: fix page_cpupid_xchg_last() to READ_ONCE() the page flags

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Jan 19, 2022 at 03:28:32PM -0800, Peter Collingbourne wrote:
> On Wed, Jan 19, 2022 at 2:03 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Tue, Jan 18, 2022 at 03:05:39PM -0800, Peter Collingbourne wrote:
> > > After submitting a patch with a compare-exchange loop similar to this
> > > one to set the KASAN tag in the page flags, Andrey Konovalov pointed
> > > out that we should be using READ_ONCE() to read the page flags. Fix
> > > it here.
> >
> > What does it actually fix? If it manages to split the read and read
> > garbage the cmpxchg will fail and we go another round, no harm done.
> 
> What I wasn't sure about was whether the compiler would be allowed to
> break this code by hoisting the read of page->flags out of the loop
> (because nothing in the loop actually writes to page->flags aside from
> the compare-exchange, and if that succeeds we're *leaving* the loop).

The cmpxchg is a barrier() and as such I don't think it's allowed to
hoist anything out of the loop. Except perhaps since it's do-while, it
could try and unroll the first iteration and wreck that something
fierce.

The bigger problem is I think that page_cpuid_last() usage which does a
second load of page->flags, and given sufficient races that could
actually load a different value and then things would be screwy. But
that's not actually fixed.

> > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > Link: https://linux-review.googlesource.com/id/I2e1f5b5b080ac9c4e0eb7f98768dba6fd7821693
> >
> > That's that doing here?
> 
> I upload my changes to Gerrit and link to them here so that I (and
> others) can see the progression of the patch via the web UI.

What's the life-time guarantee for that URL existing? Because if it
becomes part of the git commit, it had better stay around 'forever'
etc..