Re: mm: Question: pte SMP data race in do_anomyous_page()?

[Alistair Popple <apopple@nvidia.com>]

David Hildenbrand <david@redhat.com> writes:

> On 26.05.23 11:07, Karim Manaouil wrote:
>> On Thu, May 25, 2023 at 02:55:30PM +0200, David Hildenbrand wrote:
>>> On 25.05.23 12:06, Karim Manaouil wrote:
>>>> Hi,
>>>>
>>>> In do_anonymous_page(), a new page is allocated and zeroed, and the
>>>> corresponding page struct is initialised (setting flags PageUptodate,
>>>> PageSwapBacked, etc. and initialising the various counters).
>>>>
>>>> Then, set_pte_at() is called directly without calling smp_wmb() to make
>>>> the updates above visible on other CPUs.
>>>>
>>>> This could race with a page table walker. The walker can read the new pte
>>>> and try to access the page struct or the page content before the changes
>>>> above were made visible.
>>>
>>> Only after acquiring the page table lock (which the writer first has to
>>> release), right?
>> In many cases, the walkers don't take the page table locks (e.g.
>> mm/hmm.c).
>
> Looks like we really should be locking the page table in
> hmm_vma_walk_pmd() instead of only doing a pte_offset_map().
>
> It's all very racy without that ...
>
> Even the !pte_present(pte) check is racy ...

hmm_range_fault() on it's own is racy, but it's supposed to be used with
mmu interval notifiers which provide a sequence number and a driver
mutex to synchronise against pte changes. See for example
dmirror_range_snapshot() in lib/test_hmm.c.