| 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
| ||
|
Message-ID: <683CC7EC-3FAB-4C63-92F2-636C4DBE7729@nvidia.com>
Date: Fri, 26 Apr 2024 10:33:30 -0400
From: Zi Yan <ziy@...dia.com>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: Ryan Roberts <ryan.roberts@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
"Aneesh Kumar K.V" <aneesh.kumar@...nel.org>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Huacai Chen <chenhuacai@...nel.org>
Subject: Re: [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and
GUP-fast
--
Best Regards,
Yan, Zi
On 26 Apr 2024, at 0:50, Anshuman Khandual wrote:
> On 4/26/24 00:28, Zi Yan wrote:
>> +Anshuman, who changed pmd_present() semantics. See:
>> https://lore.kernel.org/all/1599627183-14453-2-git-send-email-anshuman.khandual@arm.com/ and commit b65399f6111b ("arm64/mm: Change
>> THP helpers to comply with generic MM semantics")
>>
>> On 25 Apr 2024, at 13:07, 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
>>
>> But pmd_mkinvalid() on x86 does not behave so. Maybe we should fix
>> pmd_mkinvalid() on arm64 by not setting PMD_PRESENT_INVALID when the
>> entry is invalid already. And add a test in mm/debug_vm_pgtable.c.
>>
>> I notice that x86, risc-v, mips behave the same. loongarch also
>> has _PAGE_PRESENT_INVALID bit set during pmd_mkinvalid(), but its
>> pmd_present() makes sure _PAGE_HUEG is set before checks _PAGE_PRESENT_INVALID.
>> So it is not a problem for loongarch. Add Huacai to confirm this.
>>
>> Maybe pmd_present() on arm64 can do that too?
>
> pmd_present() should return true even for a splitting PMD which is not
> currently mapped. IIRC in all other architectures, there is a distinct
> identification bit for huge page which stays back, even when the entry
> becomes unmapped. That bit helps pmd_present() return true, during PMD
> splitting process.
>
> But on arm64 platform
>
> #define PTE_VALID (_AT(pteval_t, 1) << 0)
> #define PMD_SECT_VALID (_AT(pmdval_t, 1) << 0)
> #define PMD_TABLE_BIT (_AT(pmdval_t, 1) << 1)
>
> When the PMD entry becomes unmapped, PTE_VALID gets cleared, so does the
> PMD_SECT_VALID, thus erasing its identity as a huge mapping. A software
> bit PMD_PRESENT_INVALID was added which helps in preserving that cleared
> huge page mapping identity once it becomes unmapped.
OK. PMD_SECT_VALID is just a different name of PTE_VALID. I wonder
if ~PMD_TABLE_BIT can be used as _PAGE_HUGE to indicate it is a huge page
PMD, since PMD_TABLE_BIT is unset for PMD huge page already, for swap
entry, since PMD_SECT_VALID is unset, PMD_TABLE_BIT is ignored. But it
will require PTE and PMD have different swap entry encoding on arm64.
It might not be worth the effort.
>>
>>> 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
>>> good measure let's add a warning to the generic implementation of
>>> pmdp_invalidate(). I've manually reviewed all other
>>> pmdp_invalidate[_ad]() call sites and believe all others to be
>>> conformant.
>>>
>>> This is a theoretical bug found during code review. I don't have any
>>> test case to trigger it in practice.
>>>
>>> Fixes: 84c3fc4e9c56 ("mm: thp: check pmd migration entry in common path")
>>> Signed-off-by: Ryan Roberts <ryan.roberts@....com>
>>> ---
>>>
>>> Applies on top of v6.9-rc5. Passes all the mm selftests on arm64.
>>>
>>> Thanks,
>>> Ryan
>>>
>>>
>>> mm/huge_memory.c | 5 +++--
>>> mm/pgtable-generic.c | 2 ++
>>> 2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 89f58c7603b2..80939ad00718 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -2513,12 +2513,12 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>> * for this pmd), then we flush the SMP TLB and finally we write the
>>> * non-huge version of the pmd entry with pmd_populate.
>>> */
>>> - old_pmd = pmdp_invalidate(vma, haddr, pmd);
>>>
>>> - pmd_migration = is_pmd_migration_entry(old_pmd);
>>> + pmd_migration = is_pmd_migration_entry(*pmd);
>>> if (unlikely(pmd_migration)) {
>>> swp_entry_t entry;
>>>
>>> + old_pmd = *pmd;
>>> entry = pmd_to_swp_entry(old_pmd);
>>> page = pfn_swap_entry_to_page(entry);
>>> write = is_writable_migration_entry(entry);
>>> @@ -2529,6 +2529,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>> soft_dirty = pmd_swp_soft_dirty(old_pmd);
>>> uffd_wp = pmd_swp_uffd_wp(old_pmd);
>>> } else {
>>> + old_pmd = pmdp_invalidate(vma, haddr, pmd);
>>> page = pmd_page(old_pmd);
>>> folio = page_folio(page);
>>> if (pmd_dirty(old_pmd)) {
>>> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
>>> index 4fcd959dcc4d..74e34ea90656 100644
>>> --- a/mm/pgtable-generic.c
>>> +++ b/mm/pgtable-generic.c
>>> @@ -198,6 +198,7 @@ pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp)
>>> pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
>>> pmd_t *pmdp)
>>> {
>>> + VM_WARN_ON(!pmd_present(*pmdp));
>>> pmd_t old = pmdp_establish(vma, address, pmdp, pmd_mkinvalid(*pmdp));
>>> flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
>>> return old;
>>> @@ -208,6 +209,7 @@ pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
>>> pmd_t pmdp_invalidate_ad(struct vm_area_struct *vma, unsigned long address,
>>> pmd_t *pmdp)
>>> {
>>> + VM_WARN_ON(!pmd_present(*pmdp)); return pmdp_invalidate(vma, address, pmdp);
>>> }
>>> #endif
>>> --
>>> 2.25.1
>>
>>
>> --
>> Best Regards,
>> Yan, Zi
Download attachment "signature.asc" of type "application/pgp-signature" (855 bytes)
Powered by blists - more mailing lists