Re: [PATCH v3 2/3] mm/hugetlb: disable PMD sharing for UFFD missing mode

["Li Zhe" <lizhe.67@bytedance.com>]

On 10/23/26 12:36 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mm/hugetlb: uffd_disable_huge_pmd_share() leaks poison markers on unregister
> --
>
> commit 5da87cbb6b8f2271fb75acc59e14e2d5860cb54c
> Author: David Hildenbrand (Arm) <david@kernel.org>
>
> mm/hugetlb: disable PMD sharing for UFFD missing mode
>
> This commit disables hugetlb pmd page table sharing for all userfaultfd
> registered vmas by checking vma->vm_userfaultfd_ctx.ctx in
> uffd_disable_huge_pmd_share(), preventing poison markers installed under
> missing mode from affecting shared pmd tables. It also adds a debug check
> in hugetlb_mfill_atomic_pte() when installing a poison marker.
>
>> diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
>> index a14b8a9ffb7b1..ebd86677b052d 100644
>> --- a/include/linux/userfaultfd_k.h
>> +++ b/include/linux/userfaultfd_k.h
>> @@ -168,22 +168,12 @@ static inline bool is_mergeable_vm_userfaultfd_ctx(struct vm_area_struct *vma,
>>   }
>>   
>>   /*
>> - * Never enable huge pmd sharing on some uffd registered vmas:
>> - *
>> - * - VM_UFFD_WP and VM_UFFD_RWP VMAs, because the write protect / access
>> - *   tracking information is per pgtable entry.
>> - *
>> - * - VM_UFFD_MINOR VMAs, because otherwise we would never get minor faults for
>> - *   VMAs which share huge pmds. (If you have two mappings to the same
>> - *   underlying pages, and fault in the non-UFFD-registered one with a write,
>> - *   with huge pmd sharing this would *also* setup the second UFFD-registered
>> - *   mapping, and we'd not get minor faults.)
>> + * Never enable huge pmd sharing on userfaultfd registered VMAs, because
>> + * userfaultfd state can be tracked per VMA or per page table entry.
>>    */
>>   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));
>> +	return vma->vm_userfaultfd_ctx.ctx;
>>   }
>>   
>>   /*
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 966d249469454..7566f2eed50b0 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -6399,6 +6399,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);
>>   
> [Severity: High]
> Can a pmd table containing a poison marker still become shared across
> processes after the vma is unregistered from userfaultfd?
>
> When a process registers a shared hugetlbfs vma in missing mode and installs
> a poison marker via hugetlb_mfill_atomic_pte(), the marker is written into the
> process's private pmd table.
>
> If the process later unregisters the vma or closes the userfaultfd descriptor
> (via userfaultfd_unregister() -> userfaultfd_clear_vma() or
> userfaultfd_release_all() -> userfaultfd_clear_vma(), or remaps via
> mremap_userfaultfd_prep()), the userfaultfd flags and context are cleared
> while the poison marker remains in the page table:
>
> mm/userfaultfd.c:userfaultfd_clear_vma() {
> 	...
> 	ret = vma_modify_flags_uffd(vmi, prev, vma, start, end,
> 				    &new_vma_flags, NULL_VM_UFFD_CTX,
> 				    give_up_on_oom);
>
> 	if (!IS_ERR(ret))
> 		userfaultfd_reset_ctx(ret);
> 	...
> }
>
> Unlike write-protect markers, which are resolved by change_protection() on
> unregistration, poison markers intentionally persist after the userfaultfd
> context is reset.
>
> Once vma->vm_userfaultfd_ctx.ctx is cleared and the userfaultfd flags are
> removed from vma->vm_flags, uffd_disable_huge_pmd_share() returns false and
> page_table_shareable() in mm/hugetlb.c considers the vma eligible for sharing
> again:
>
> mm/hugetlb.c:huge_pmd_share() {
> 	...
> 	mapping_rmap_tree_foreach(svma, mapping, idx, idx) {
> 		if (svma == vma)
> 			continue;
>
> 		saddr = page_table_shareable(svma, vma, addr, idx);
> 		if (saddr) {
> 			spte = hugetlb_walk(svma, saddr,
> 					    vma_mmu_pagesize(svma));
> 			if (spte) {
> 				ptdesc_pmd_pts_inc(virt_to_ptdesc(spte));
> 				break;
> 			}
> 		}
> 	}
> 	...
> }
>
> When another process mapping the same shared hugetlbfs file faults in that
> range (hugetlb_fault() -> huge_pte_alloc() -> want_pmd_share() ->
> huge_pmd_share()), huge_pmd_share() walks the first process's vma, finds its
> populated pmd table containing the poison marker, and shares it into the
> second process.
>
> That exposes the second process to the first process's poison marker. In
> addition, if the first process subsequently unshares the pmd table (for
> example via mprotect(), hugetlb_split(), or re-registering userfaultfd),
> __huge_pmd_unshare() clears the pud entry in the first process, causing the
> originating process to lose its own poison marker while leaving it installed
> in the second process.
>
> Should the vma or ptdesc record that a poison marker was installed so pmd
> sharing remains disabled for it even after userfaultfd unregistration?

Thanks for the report.

This looks like a valid concern, but I think it is not introduced by
this patch. Before this patch, VM_UFFD_MISSING did not prevent hugetlb
PMD sharing, so UFFDIO_POISON could already install poison markers into
page tables that could later become shared.

This patch addresses the registered-VMA side of that problem by making
hugetlb PMD sharing disabled for userfaultfd-registered VMAs. So new
poison markers should no longer be installed into shared PMD page tables
while the VMA is registered.

The unregister case seems to be a remaining pre-existing lifecycle issue
around PTE_MARKER_POISONED and hugetlb PMD sharing. David previously
mentioned broader poison-marker issues and prototype fixes here:

https://lore.kernel.org/all/bb72cc2c-a46a-44f8-99a2-b036abfdbd26@kernel.org/

So I think we should wait for David's thoughts on how to handle that
remaining case.

Thanks,
Zhe