Re: [PATCH] mm/hugetlb: avoid recursive i_mmap_rwsem in PMD sharing

[David Hildenbrand (Arm)]

On 10/16/26 10:56, Li Zhe wrote:
> On 10/16/26 3:33 AM, David Hildenbrand (Arm) wrote:
>> On 10/15/26 04:40, Li Zhe wrote:
>>> Yes, mostly.
>>>
>>> If the source PMD table is shared, huge_pmd_unshare() already drops that
>>> shared PMD table from the source side and we skip moving individual PTEs
>>> from that table.
>>>
>>> If the source PTE is not in a shared PMD table, but the destination PTE
>>> is in a shared PMD table, then I think we should not install the source
>>> PTE into the destination page table. The destination mapping is already
>>> represented by the shared PMD page table.
>>>
>>> However, move_huge_pte() still has to handle the source side as an
>>> unmap: clear the source PTE, record the TLB removal, and update the
>>> source-side hugetlb accounting/rmap state. Otherwise the source mapping
>>> is removed from the page table without the matching source-side
>>> accounting. I was able to reproduce this as:
>>>
>>>     BUG: Bad page cache in process init
>>>     page dumped because: still mapped when deleted
>>>
>>> when keeping the preallocation approach but dropping the special handling
>>> for a destination PMD-shared page table.
>>>
>>> So my current understanding is:
>>>
>>>     1. source PMD shared: unshare it and skip the PMD-sized range;
>>>     2. destination PMD shared: do not modify the destination PTE, but
>>>        still clear and account the source PTE removal.
>> Yes, I think this makes a lot of sense. Given that this issue has been
>> in the works for more than one month without a fix I tried to see how to do
>> it cleaner than your version:
>>
>> This is what I came up with quickly, using existing helpers by refactoring them.
>>
>> Is this what you had in mind? I didn't actually test this yet. If it makes sense,
>> could you test it and possibly clean it up further?
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index fb51f7d02cebf..0c3aebea7a2f8 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -124,6 +124,9 @@ static int __huge_pmd_unshare(struct mmu_gather *tlb,
>>   		bool check_locks);
>>   static void hugetlb_unshare_pmds(struct vm_area_struct *vma,
>>   		unsigned long start, unsigned long end, bool take_locks);
>> +static void hugetlb_unmap_range(struct mmu_gather *tlb,
>> +		struct vm_area_struct *vma, unsigned long start, unsigned long end,
>> +		struct folio *folio, zap_flags_t zap_flags);
>>   static struct resv_map *vma_resv_map(struct vm_area_struct *vma);
>>   
>>   static inline bool subpool_is_free(struct hugepage_subpool *spool)
>> @@ -5172,6 +5175,8 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
>>   	for (; old_addr < old_end; old_addr += sz, new_addr += sz) {
>>   		const unsigned long offset_to_last_entry =
>>   			(old_addr | last_addr_mask) - old_addr;
>> +		const unsigned long offset_to_last_dst_entry =
>> +			(new_addr | last_addr_mask) - new_addr;
>>   
>>   		src_pte = hugetlb_walk(vma, old_addr, sz);
>>   		if (!src_pte) {
>> @@ -5188,6 +5193,23 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
>>   			continue;
>>   		}
>>   
>> +		/*
>> +		 * Don't try to install a shared PMD table and move stuff into
>> +		 * them. Instead, just zap the source PMDs and share the PMD
>> +		 * table later lazily.
>> +		 */
>> +		if (sz == PMD_SIZE && want_pmd_share(new_vma, new_addr)) {
>> +			const unsigned long unmap_size = min(old_end - old_addr,
>> +							     offset_to_last_dst_entry + sz);
>> +
>> +			hugetlb_unmap_range(&tlb, vma, old_addr,
>> +					    old_addr + unmap_size, NULL,
>> +					    ZAP_FLAG_DROP_MARKER);
>> +			old_addr += unmap_size - sz;
>> +			new_addr += unmap_size - sz;
>> +			continue;
>> +		}
>> +
>>   		dst_pte = huge_pte_alloc(mm, new_vma, new_addr, sz);
>>   		if (!dst_pte)
>>   			break;
>> @@ -5207,9 +5229,9 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
>>   	return len + old_addr - old_end;
>>   }
>>   
>> -void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
>> -			    unsigned long start, unsigned long end,
>> -			    struct folio *folio, zap_flags_t zap_flags)
>> +static void hugetlb_unmap_range(struct mmu_gather *tlb,
>> +		struct vm_area_struct *vma, unsigned long start, unsigned long end,
>> +		struct folio *folio, zap_flags_t zap_flags)
>>   {
>>   	struct mm_struct *mm = vma->vm_mm;
>>   	const bool folio_provided = !!folio;
>> @@ -5227,13 +5249,6 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
>>   	BUG_ON(start & ~huge_page_mask(h));
>>   	BUG_ON(end & ~huge_page_mask(h));
>>   
>> -	/*
>> -	 * This is a hugetlb vma, all the pte entries should point
>> -	 * to huge page.
>> -	 */
>> -	tlb_change_page_size(tlb, sz);
>> -	tlb_start_vma(tlb, vma);
>> -
>>   	last_addr_mask = hugetlb_mask_last_page(h);
>>   	address = start;
>>   	for (; address < end; address += sz) {
>> @@ -5362,6 +5377,15 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
>>   		if (folio_provided)
>>   			break;
>>   	}
>> +}
>> +
>> +void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
>> +			    unsigned long start, unsigned long end,
>> +			    struct folio *folio, zap_flags_t zap_flags)
>> +{
>> +	tlb_change_page_size(tlb, huge_page_size(hstate_vma(vma)));
>> +	tlb_start_vma(tlb, vma);
>> +	hugetlb_unmap_range(tlb, vma, start, end, folio, zap_flags);
>>   	tlb_end_vma(tlb, vma);
>>   
>>   	huge_pmd_unshare_flush(tlb, vma);
> 
> 
> Hi David,
> 
> Your patch looks good to me.
> 
> Sashiko pointed out one corner case around UFFDIO_POISON.  In the
> destination-PMD-sharing path, the patch zaps the source range and lets
> the destination fault path populate or share the PMD lazily.  That works
> for normal hugetlb mappings, but if the source PTE contains
> PTE_MARKER_POISONED, the marker is not copied to the destination. 

Any UFFD activity must necessarily disable PMD sharing.

That's why want_pmd_share() checks uffd_disable_huge_pmd_share().

We cannot possibly move a UFFD marker into shared page table, that would be
completely broken already.


> The
> source zap uses ZAP_FLAG_DROP_MARKER, so the marker can be dropped, and a
> later fault at the new address would see an empty entry instead of the
> poisoned marker.
> 
> One possible way to avoid this is to make VM_UFFD_MISSING participate in
> the existing uffd_disable_huge_pmd_share() logic, because UFFDIO_POISON
> can install PTE_MARKER_POISONED and the poison state is stored in the
> PTE, similar to the existing UFFD-WP/RWP per-PTE state.
> 
> For example:
> 
> ---
> diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> index a4351cffc60c..9a057d7531c3 100644
> --- a/include/linux/userfaultfd_k.h
> +++ b/include/linux/userfaultfd_k.h
> @@ -183,8 +187,8 @@ static inline bool 
> is_mergeable_vm_userfaultfd_ctx(struct
> vm_area_struct *vma,
>   static inline bool uffd_disable_huge_pmd_share(struct vm_area_struct *vma)
>   {
>       return vma_test_any_mask(vma,
> -        mk_vma_flags_from_masks(VMA_UFFD_WP, VMA_UFFD_RWP,
> -                    VMA_UFFD_MINOR));
> +        mk_vma_flags_from_masks(VMA_UFFD_MISSING, VMA_UFFD_WP,
> +                    VMA_UFFD_RWP, VMA_UFFD_MINOR));


I think we can currently install POISON markers into shared PMD tables. What a
mess. Let me dig, that all needs a proper fix.

-- 
Cheers,

David