Re: ptep_get_lockless() on 32-bit x86/mips/sh looks wrong

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Oct 06, 2022 at 05:55:54PM +0200, Jann Horn wrote:

> > I think the argument here has nothing to do with IPIs, but is more a
> > statement on memory ordering.
> 
> The comment above the definition of ptep_get_lockless() claims: "it
> will not switch to a completely different present page without a TLB
> flush in between; something that we are blocking by holding interrupts
> off."

I was always skeptical of that argument..

> > So, it seems plausible this could be OK based only on atomics (I did
> > not check that the present bit is properly placed in the right
> > low/high). Do you see a way the atomics don't work out?
> 
> The race would be something like this, where A is one thread doing
> ptep_get_lockless() and B, C and D are other threads:
> 
> <PTE initially points to address 0x0001000100010000>
> A: read ptep->pte_low, sees low address half 0x00010000
> B: begins MADV_DONTNEED, removes the PTE but doesn't flush TLB yet
> C: page fault installs a new PTE pointing to address 0x0001000200020000
> A: read ptep->pte_high, sees high address half 0x00010002
> C: begins MADV_DONTNEED, removes the PTE but doesn't flush TLB yet
> D: page fault installs a new PTE pointing to address 0x0001000300010000
> A: re-read ptep->pte_low, sees low address half 0x00010000 matching
> the first one
> A: returns physical address 0x000100020x00010000, which was never
> actually in the PTE

Okay, that does seem plausible :(

> So it's not a problem with the memory ordering, it's just that it's
> not possible to atomically read a 64-bit PTE with 32-bit reads when
> the PTE can completely change under you - and ptep_get_lockless() was
> written under the assumption that this can't happen because of TLB
> flush IPIs.

It does seem broken then.

If the arch can't provide an atomci load, I suppose the "easy" way to
fix it would be to use a seqlock to protect it..

Though in practice the chances the PTE changes multiple times between
two loads might be sufficiently rare on already rare 32 bit high mem
systems, perhaps it isn't worth fixing..

Thanks,
Jason