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

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

[Karim Manaouil]

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.

The reason I thought about this is because of the comment in
pmd_install(), which describes exactly the same situation, so I
wondered why the same thing is not considered in do_anonymous_page().

I might probably be missing something, but I would love to hear your
comments.

Cheers
Karim

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

[David Hildenbrand]

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?

-- 
Thanks,

David / dhildenb

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

[Matthew Wilcox]

On Thu, May 25, 2023 at 11:06:40AM +0100, 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.

How are you able to read the code without reading the comment
that explains this?

        /*
         * The memory barrier inside __SetPageUptodate makes sure that
         * preceding stores to the page contents become visible before
         * the set_pte_at() write.
         */
        __SetPageUptodate(page);

        entry = mk_pte(page, vma->vm_page_prot);

That comment has been there since 2013.

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

[David Hildenbrand]

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 ...

-- 
Thanks,

David / dhildenb

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

[Alistair Popple]

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.

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

[David Hildenbrand]

On 31.05.23 06:55, Alistair Popple wrote:
> 
> 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.
> 

How is this supposed to work with racing do_swap_page() that converts 
!pte_present() -> pte_present() without triggering any mmu notifier AFAIKs?

-- 
Thanks,

David / dhildenb

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

[Alistair Popple]

David Hildenbrand <david@redhat.com> writes:

> On 31.05.23 06:55, Alistair Popple wrote:
>> 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.
>> 
>
> How is this supposed to work with racing do_swap_page() that converts
> !pte_present() -> pte_present() without triggering any mmu notifier
> AFAIKs?

It depends what the caller asks for. If the caller has set
HMM_PFN_REQ_FAULT hmm_range_fault() will call handle_mm_fault() and then
retry, presumably finding the now present PTE.

If the caller hasn't asked for it to be faulted it will be returned as a
non-present PTE. But the caller has to be prepared to deal with that -
even if we took the PTL it's just a matter of timing.

In practice hmm_range_fault() is used to mirror PTEs to a secondary MMU
in response to a fault. So it doesn't matter if a transient condition
causes no PTE to be returned, the secondary MMU will just retry the
fault. What is critically important is that the secondary MMU doesn't
contain more permissive PTEs, hence the need to synchronise using mmu
notifiers to ensure mappings and write permissions get revoked.