On 6/1/26 11:08, Lance Yang wrote:
>
>
> On 2026/6/1 14:54, David Hildenbrand (Arm) wrote:
>> On 6/1/26 05:28, Lance Yang wrote:
>>>
>>>
>>> Ah, fair point.
>>>
>>> I was mostly worried about arch hooks that walk vma->vm_mm again, rather
>>> than only using the pte pointer passed in. For example, mips does:
>>
>> Right, a re-walk would be the real problem.
>>
>>>
>>> update_mmu_cache_range()
>>> -> __update_tlb()
>>> -> pgd_offset(vma->vm_mm, address)
>>> -> pte_offset_map(...)
>>>
>>> and __update_tlb() has this assumption:
>>>
>>> /*
>>> * update_mmu_cache() is called between pte_offset_map_lock()
>>> * and pte_unmap_unlock(), so we can assume that ptep is not
>>> * NULL here: and what should be done below if it were NULL?
>>> */
>>>
>>> So if khugepaged happens to run with current->active_mm == vma->vm_mm
>>> here, could __update_tlb() hit the none PMD, get NULL from
>>> pte_offset_map(), and then dereference it?
>>
>> Likely yes -- that MIPS code is horrible. And the comment in MIPS code
>> even spells that out. :(
>>
>> Do you know about other code like that, or is MIPS the only one doing a
>> re-walk and crossing fingers?
>>
>>>
>>> Just wanted to raise it since some arch code may still have assumptions
>>> like this, and the always-enable-mTHP work is getting closer ...
>>
>> Right. I assume set_pte_at() couldn't trigger something similar (re-walk) in
>> arch code,
>> because we simply provide the ptep. update_mmu_cache_range() only consumes
>> the
>> pte.
>>
>>>
>>> Probably very very very hard to hit, though :)
>>
>> Delaying update_mmu_cache_range() is nasty, as we'd have to make sure that
>> nobody can interfere in the meantime ... and the PMD lock will not be
>> sufficient.
>>
>> Maybe we could reinstall the page table with the cleared (none) entries while
>> still holding the PTL?
>>
>> Thinking out loud:
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 5ba298d420b7..e39b750b1e6f 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -1413,13 +1413,17 @@ static enum scan_result collapse_huge_page(struct
>> mm_struct *mm, unsigned long s
>> map_anon_folio_pmd_nopf(folio, pmd, vma, pmd_addr);
>> } else {
>> /*
>> - * set_ptes is called in map_anon_folio_pte_nopf with the
>> - * pmd_ptl lock still held; this is safe as the PMD is
>> expected
>> - * to be none. The pmd entry is then repopulated below.
>> + * Re-insert the page table with the cleared entries, but
>> + * hold the PTL, such that no one can mess with the
>> re-installed
>> + * page table until we updated the temporarily-cleared
>> entries
>> + * through map_anon_folio_pte_nopf().
>> */
>> - map_anon_folio_pte_nopf(folio, pte, vma, start_addr, /
>> *uffd_wp=*/ false);
>> - smp_wmb(); /* make PTEs visible before PMD. See
>> pmd_install() */
>
> One small thing, I think we should probably keep the smp_wmb(), and just
> move it before the earlier pmd_populate().
>
> IIUC, the ordering we want is still:
>
> clear old PTEs
> smp_wmb()
> pmd_populate()
>
> so another CPU cannot walk through the re-installed PMD and still observe
> the old PTEs, right?
There is a smp_wmb() in __folio_mark_uptodate(), that should be sufficient?
--
Cheers,
David
