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.
Fixes: 75980e97dacc ("mm: fold page->_last_nid into page->flags where possible")
Signed-off-by: Peter Collingbourne <[email protected]>
Link: https://linux-review.googlesource.com/id/I2e1f5b5b080ac9c4e0eb7f98768dba6fd7821693
Cc: [email protected]
---
mm/mmzone.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/mmzone.c b/mm/mmzone.c
index eb89d6e018e2..f84b84b0d3fc 100644
--- a/mm/mmzone.c
+++ b/mm/mmzone.c
@@ -90,7 +90,7 @@ int page_cpupid_xchg_last(struct page *page, int cpupid)
int last_cpupid;
do {
- old_flags = flags = page->flags;
+ old_flags = flags = READ_ONCE(page->flags);
last_cpupid = page_cpupid_last(page);
flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);
--
2.34.1.703.g22d0c6ccf7-goog
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.
> Fixes: 75980e97dacc ("mm: fold page->_last_nid into page->flags where possible")
As per the above argument, I don't think this rates a Fixes tag, there
is no actual fix.
> Signed-off-by: Peter Collingbourne <[email protected]>
> Link: https://linux-review.googlesource.com/id/I2e1f5b5b080ac9c4e0eb7f98768dba6fd7821693
That's that doing here?
> Cc: [email protected]
That's massively over-selling things.
> ---
> mm/mmzone.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mmzone.c b/mm/mmzone.c
> index eb89d6e018e2..f84b84b0d3fc 100644
> --- a/mm/mmzone.c
> +++ b/mm/mmzone.c
> @@ -90,7 +90,7 @@ int page_cpupid_xchg_last(struct page *page, int cpupid)
> int last_cpupid;
>
> do {
> - old_flags = flags = page->flags;
> + old_flags = flags = READ_ONCE(page->flags);
> last_cpupid = page_cpupid_last(page);
>
> flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);
I think that if you want to touch that code, something like the below
makes more sense...
diff --git a/mm/mmzone.c b/mm/mmzone.c
index eb89d6e018e2..ed9f4bcdc9ee 100644
--- a/mm/mmzone.c
+++ b/mm/mmzone.c
@@ -89,13 +89,14 @@ int page_cpupid_xchg_last(struct page *page, int cpupid)
unsigned long old_flags, flags;
int last_cpupid;
+ old_flags = READ_ONCE(page->flags);
do {
- old_flags = flags = page->flags;
- last_cpupid = page_cpupid_last(page);
+ flags = old_flags;
+ last_cpupid = (flags >> LAST_CPUPID_PGSHIFT) & LAST_CPUPID_MASK;
flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);
flags |= (cpupid & LAST_CPUPID_MASK) << LAST_CPUPID_PGSHIFT;
- } while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags));
+ } while (unlikely(!try_cmpxchg(&page->flags, &old_flags, flags)));
return last_cpupid;
}
On Wed, Jan 19, 2022 at 2:03 AM Peter Zijlstra <[email protected]> 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).
That could potentially result in a loop that never terminates if the
first compare-exchange fails. This is largely a theoretical problem as
far as I know; the assembly produced by clang and gcc on x86_64 and
arm64 appears to be doing the expected thing for now, and we're using
inline asm for compare-exchange instead of the compiler builtins on
those architectures (and on all other architectures it seems? no
matches for __atomic_compare_exchange outside of kcsan and the
selftests) so the compiler wouldn't be able to look inside it anyway.
> > Fixes: 75980e97dacc ("mm: fold page->_last_nid into page->flags where possible")
>
> As per the above argument, I don't think this rates a Fixes tag, there
> is no actual fix.
Okay, I'll remove it unless you find the above convincing.
> > Signed-off-by: Peter Collingbourne <[email protected]>
> > 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.
> > Cc: [email protected]
>
> That's massively over-selling things.
Fair enough since it isn't causing an actual problem, I'll remove this tag.
> > ---
> > mm/mmzone.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/mmzone.c b/mm/mmzone.c
> > index eb89d6e018e2..f84b84b0d3fc 100644
> > --- a/mm/mmzone.c
> > +++ b/mm/mmzone.c
> > @@ -90,7 +90,7 @@ int page_cpupid_xchg_last(struct page *page, int cpupid)
> > int last_cpupid;
> >
> > do {
> > - old_flags = flags = page->flags;
> > + old_flags = flags = READ_ONCE(page->flags);
> > last_cpupid = page_cpupid_last(page);
> >
> > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);
>
> I think that if you want to touch that code, something like the below
> makes more sense...
Yeah, that looks a bit nicer. I'll send a v2 and update the other patch as well.
Peter
On Wed, Jan 19, 2022 at 03:28:32PM -0800, Peter Collingbourne wrote:
> On Wed, Jan 19, 2022 at 2:03 AM Peter Zijlstra <[email protected]> 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 <[email protected]>
> > > 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..
On Thu, Jan 20, 2022 at 2:43 AM Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Jan 19, 2022 at 03:28:32PM -0800, Peter Collingbourne wrote:
> > On Wed, Jan 19, 2022 at 2:03 AM Peter Zijlstra <[email protected]> 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.
Yes it looks like it's at least as powerful as a barrier() because the
implementations I looked at (arm64 and x86) use inline asm with memory
operand (i.e. same as barrier()).
> 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.
Right, the patch you provided (which I posted as v2) inlines the
page_cpuid_last() and should resolve this.
> > > > Signed-off-by: Peter Collingbourne <[email protected]>
> > > > 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..
I'd be surprised if it went away any time soon, the same hosting is
used for projects like Android and Chromium which have been using it
for years and have a long track record of stability.
Also note that the link is useful independent of the host continuing
to be up, it means that you can also do a search of the mailing list
archive like so:
https://lore.kernel.org/all/?q=I2e1f5b5b080ac9c4e0eb7f98768dba6fd7821693
(or the equivalent link on a different mailing list archive if
lore.kernel.org ever gets shut down) and find the progression of the
patch that way. This is particularly useful if (as in this case) the
subject line of the patch changes.
Peter