linux-kernel - Re: [PATCH] mm/mmzone.c: fix page_cpupid_xchg_last() to READ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 Jan 2022 11:43:32 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Peter Collingbourne <pcc@...gle.com>
Cc:     Andrey Konovalov <andreyknvl@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mel Gorman <mgorman@...e.de>,
        "# 3.4.x" <stable@...r.kernel.org>
Subject: Re: [PATCH] mm/mmzone.c: fix page_cpupid_xchg_last() to READ_ONCE()
 the page flags

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@...radead.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@...gle.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..