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). 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 pcc@google.com Link: https://linux-review.googlesource.com/id/I2e1f5b5b080ac9c4e0eb7f98768dba6fd7...
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: stable@vger.kernel.org
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