On 4/26/24 20:03, Zi Yan wrote: > > > -- > 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@xxxxxxx/ 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 Non PTE Level descriptor BIT[1:0] = X0 Invalid entry = 01 Block entries (Huge Pages) = 11 Table entries PTE Level descriptor BIT[1:0] = X0 Invalid entry = 01 Reserved invalid entry = 11 Page entries Although PTE_VALID and PMD_SECT_VALID share the same bit position, the huge page also requires the table bit to be cleared. > 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. Right and also depending on just clearing of table bit for huge page mapping might be problematic in subtle ways. > >>> >>>> 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@xxxxxxx> >>>> --- >>>> >>>> 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
