Re: [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and GUP-fast

[Zi Yan]

On 29 Apr 2024, at 2:17, Anshuman Khandual wrote:

> On 4/28/24 02:18, Zi Yan wrote:
>> On 27 Apr 2024, at 16:45, Zi Yan wrote:
>>
>>> On 27 Apr 2024, at 15:11, John Hubbard wrote:
>>>
>>>> On 4/27/24 8:14 AM, Zi Yan wrote:
>>>>> On 27 Apr 2024, at 0:41, John Hubbard wrote:
>>>>>> On 4/25/24 10:07 AM, Ryan Roberts wrote:
>>>>>>> __split_huge_pmd_locked() can be called for a present THP, devmap or
>>>>>>> (non-present) migration entry. It calls pmdp_invalidate()
>>>>>>> unconditionally on the pmdp and only determines if it is present or not
>>>>>>> based on the returned old pmd. This is a problem for the migration entry
>>>>>>> case because pmd_mkinvalid(), called by pmdp_invalidate() must only be
>>>>>>> called for a present pmd.
>>>>>>>
>>>>>>> On arm64 at least, pmd_mkinvalid() will mark the pmd such that any
>>>>>>> future call to pmd_present() will return true. And therefore any
>>>>>>> lockless pgtable walker could see the migration entry pmd in this state
>>>>>>> and start interpretting the fields as if it were present, leading to
>>>>>>> BadThings (TM). GUP-fast appears to be one such lockless pgtable walker.
>>>>>>> I suspect the same is possible on other architectures.
>>>>>>>
>>>>>>> Fix this by only calling pmdp_invalidate() for a present pmd. And for
>>>>>> Yes, this seems like a good design decision (after reading through the
>>>>>> discussion that you all had in the other threads).
>>>>> This will only be good for arm64 and does not prevent other arch developers
>>>>> to write code breaking arm64, since only arm64's pmd_mkinvalid() can turn
>>>>> a swap entry to a pmd_present() entry.
>>>> Well, let's characterize it in a bit more detail, then:
>>>>
>>>> 1) This patch will make things better for arm64. That's important!
>>>>
>>>> 2) Equally important, this patch does not make anything worse for
>>>>    other CPU arches.
>>>>
>>>> 3) This patch represents a new design constraint on the CPU arch
>>>>    layer, and thus requires documentation and whatever enforcement
>>>>    we can provide, in order to keep future code out of trouble.
>>>>
>>>> 3.a) See the VM_WARN_ON() hunks below.
>>>>
>>>> 3.b) I like the new design constraint, because it is reasonable and
>>>>      clearly understandable: don't invalidate a non-present page
>>>>      table entry.
>>>>
>>>> I do wonder if there is somewhere else that this should be documented?
>> In terms of documentation, at least in Documentation/mm/arch_pgtable_helpers.rst,
>> pmd_mkinvalid() entry needs to add "do not call on an invalid entry as
>> it breaks arm64"
>
> s/invalid/non-present ?					^^^^^^^^^^^^^
>
> But validation via mm/debug_vm_pgtable.c would require a predictable return
> value from pmd_mkinvalid() e.g return old pmd when the entry is not present.
>
> 	ASSERT(pmd = pmd_mkinvalid(pmd)) - when pmd is not present
>
> Otherwise, wondering how the semantics could be validated in the test.

I thought about checking this in mm/debug_vm_pgtable.c but concluded it is
impossible. We want to make sure no one use pmd_mkinvalid() on
!pmd_present() entries but that requires pmd_mkinvalid() on input entries'
at code writing time. A runtime test like mm/debug_vm_pgtable.c does not help.

I also even thought about changing pmd_mkinvalid() input parameter type to
a new pmd_invalid_t, so the type system can enforce it, but when we read
from a PMD entry, unless we inspect the bits, there is no way of determining
it is valid or not statically.

To me, the most future proof method is to make arm64 pmd_mkinvalid() to return
without changing anything if the input entry is !pmd_present(). This aligns
arm64 pmd_mkinvalid() with other arches pmd_mkinvalid() semantics, so that
if someone writes code using pmd_mkinvalid(), which runs on arches other than
arm64, the code would also work on arm64. But I am not going to insist on this
and let Ryan to make the call.


--
Best Regards,
Yan, Zi

Attachment: signature.asc
Description: OpenPGP digital signature