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

["David Hildenbrand (Arm)" <david@kernel.org>]

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. 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.

Okay, things escalated a bit and I feel like I ended up prototyping fixes for half the kernel ;)

So, we can as of today:

1) install POISON marker into shared PMD tables. Either using ordinary marker
   installation or using mremap(). Bad.

2) Lose POISON markers during fork(). Bad.

3) Lose POISON markers during retracting of pgtables. Bad.


I have prototype fixes for everything, but it depends on other fixes/infrastrcture that would make it all hard to backport.

So my thinking is that likely we should do what you suggest:
commit ed299abf3922678d2293072ee5d7bddc22b4e9a1 (HEAD)
Author: David Hildenbrand (Arm) <david@kernel.org>
Date:   Fri Oct 16 13:22:43 2026 +0200

    mm/hugetlb: disable PMD sharing for UFFD missing mode

    For UFFDIO_POISON to install a poison marker in a hugetlb mapping, it is
    sufficient to have the VMA registered for missing mode.

    However, UFFD missing mode does currently not block PMD table sharing, so
    installing the marker will be visible to everyone using the shared PMD
    table, which is wrong as UFFDIO_POISON must stay local to the process.

    To fix it, include VM_UFFD_MISSING in uffd_disable_huge_pmd_share().
    Add a debug check on the marker installation path.

    The existing registration path will then unshare PMD tables, and subsequent
    faults will not share them while the mapping remains registered.

    The alternative would be to unshare PMD tables selectively when
    installing poison markers, which results in way more complexity. Or
    alternatively, simply failing the marker installation, which might break
    some use cases.

    This effectively disables PMD sharing for missing-mode registrations that never
    use UFFDIO_POISON.

    Suggested-by: Li Zhe <lizhe.67@bytedance.com>
    Fixes: 8a13897fb0da ("mm: userfaultfd: support UFFDIO_POISON for hugetlbfs")
    Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>

diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index a14b8a9ffb7b1..607f012b33849 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -168,7 +168,10 @@ static inline bool is_mergeable_vm_userfaultfd_ctx(struct vm_area_struct *vma,
 }

 /*
- * Never enable huge pmd sharing on some uffd registered vmas:
+ * Never enable huge pmd sharing on the following VMAs:
+ *
+ * - VM_UFFD_MISSING VMAs, because UFFDIO_POISON may install markers and we
+ *   want PMD tables to be unshared when installing these markers already.
  *
  * - VM_UFFD_WP and VM_UFFD_RWP VMAs, because the write protect / access
  *   tracking information is per pgtable entry.
@@ -181,9 +184,10 @@ 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));
+       const vma_flags_t uffd_flags = mk_vma_flags_from_masks(VMA_UFFD_MISSING, VMA_UFFD_WP,
+                                                              VMA_UFFD_RWP, VMA_UFFD_MINOR);
+
+       return vma_test_any_mask(vma, uffd_flags);
 }

 /*
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 76f4d2a5e0206..fc42be0274603 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6376,6 +6376,7 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
                        return -EEXIST;
                }

+               VM_WARN_ON_ONCE(size == PMD_SIZE && hugetlb_pmd_shared(dst_pte));
                _dst_pte = make_pte_marker(PTE_MARKER_POISONED);
                set_huge_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte, size);


Followed by my fix here.

We will at least no longer be able to install poison markers into shared PMD tables or deadlock.

Yes, we might lose UFFD_POISON markers in some odd mremap() situations, but it's all
very broken already, and likely we should just fix that later and not backport it.

It's unlikely that people depend on it.

-- 
Cheers,

David