Re: [PATCH] mm/userfaultfd: only allocate anon_vma in CoW mappings

[Mike Rapoport]

On Wed, Oct 14, 2026 at 01:53:11PM +0200, David Hildenbrand (Arm) wrote:
> find_vma_and_prepare_anon() can end up allocating an anon_vma in VMAs that
> are not CoW mappings, which is wrong.
> 
> If we have a MAP_SHARED R/O file mapped, we lack VM_SHARED (but do have
> VM_MAYSHARE) and lack VM_MAYWRITE.
> 
> While uffd later rejects such mappings (uffd registration requires
> VM_MAYWRITE, see comment in validate_dst_vma) find_vma_and_prepare_anon()
> still wrongly allocates an anon vma before it validates the vma.
> 
> This in turn later confuses copy_page_range()->vma_needs_copy() such
> that we end up calling copy_hugetlb_page_range() with ranges that
> actually do not contain *any* per-pagetable information that would
> require copying, which is the basis for PMD-table sharing in the first
> place.
> 
> copy_hugetlb_page_range() should not have to worry about shared
> PMD tables at all: it doesn't make any sense to copy page tables that are
> shared, as they can be fully recreated later. Ending up in the function
> with shareable page tables is the real issue.
> 
> If a destination PMD table is shared in copy_hugetlb_page_range(), it
> might get concurrently unshared as we are not holding relevant
> locks, resulting in us using a page table that is no longer mapped and
> possibly freed after calling:
> 
> 	dst_pte = huge_pte_alloc(dst, dst_vma, addr, sz);
> 
> To fix it, do not allocate an anon_vma in non-CoW mappings and add a
> safety guard to copy_hugetlb_page_range(), making sure that we never
> end up messing with shared PMD tables in there.
> 
> Before commit 867a43a34ff8a ("userfaultfd: use per-vma locks in userfaultfd
> operations") we would always validate the VMA before allocating an
> anon_vma, and consequently never wrongly allocate an anon_vma.
> 
> As uffd_lock_vma() is part of the same lookup machinery and
> also contains a misleading VM_SHARED check, change that one as well
> directly for consistency.
> 
> A follow-up cleanup will stop taking the hugetlb_vma_lock_read(src_vma)
> lock in copy_hugetlb_page_range() entirely. Further, the usage of
> VM_SHARED in userfaultfd code should be cleaned up later to focus on
> CoW vs. non-CoW mappings.
> 
> Reported-by: Kyle Zeng <kylebot@xxxxxxxxxx>
> Fixes: 867a43a34ff8a ("userfaultfd: use per-vma locks in userfaultfd operations")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

Acked-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>

> ---
>  mm/hugetlb.c     | 24 ++++++++++++------------
>  mm/userfaultfd.c |  5 ++---
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 63dc836061c99..60f9b209c8db1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4984,10 +4984,11 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  		raw_write_seqcount_begin(&src->write_protect_seq);
>  	} else {
>  		/*
> -		 * For shared mappings the vma lock must be held before
> -		 * calling hugetlb_walk() in the src vma. Otherwise, the
> -		 * returned ptep could go away if part of a shared pmd and
> -		 * another thread calls huge_pmd_unshare.
> +		 * Shared mappings only need copying when we could have
> +		 * information stored in page tables (e.g., userfaultfd PTE
> +		 * bits or markers), which is incompatible with PMD sharing.
> +		 * Still take the hugetlb VMA lock for now to satisfy
> +		 * hugetlb_walk()'s locking requirements.
>  		 */
>  		hugetlb_vma_lock_read(src_vma);
>  	}
> @@ -4995,6 +4996,13 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  	last_addr_mask = hugetlb_mask_last_page(h);
>  	for (addr = src_vma->vm_start; addr < src_vma->vm_end; addr += sz) {
>  		spinlock_t *src_ptl, *dst_ptl;
> +
> +		if (WARN_ON_ONCE(want_pmd_share(src_vma, addr) ||
> +				 want_pmd_share(dst_vma, addr))) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
>  		src_pte = hugetlb_walk(src_vma, addr, sz);
>  		if (!src_pte) {
>  			addr |= last_addr_mask;
> @@ -5006,14 +5014,6 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  			break;
>  		}
>  
> -#ifdef CONFIG_HUGETLB_PMD_PAGE_TABLE_SHARING
> -		/* If the pagetables are shared, there is nothing to do */
> -		if (ptdesc_pmd_is_shared(virt_to_ptdesc(dst_pte))) {
> -			addr |= last_addr_mask;
> -			continue;
> -		}
> -#endif
> -
>  		dst_ptl = huge_pte_lock(h, dst, dst_pte);
>  		src_ptl = huge_pte_lockptr(h, src, src_pte);
>  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 0e0fe9bfef239..82b56a58fc40d 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -115,8 +115,7 @@ struct vm_area_struct *find_vma_and_prepare_anon(struct mm_struct *mm,
>  	vma = vma_lookup(mm, addr);
>  	if (!vma)
>  		vma = ERR_PTR(-ENOENT);
> -	else if (!(vma->vm_flags & VM_SHARED) &&
> -		 unlikely(anon_vma_prepare(vma)))
> +	else if (vma_is_cow_mapping(vma) && unlikely(anon_vma_prepare(vma)))
>  		vma = ERR_PTR(-ENOMEM);
>  
>  	return vma;
> @@ -145,7 +144,7 @@ static struct vm_area_struct *uffd_lock_vma(struct mm_struct *mm,
>  		 * We know we're going to need an anon rmap, so check that
>  		 * early.
>  		 */
> -		if (!(vma->vm_flags & VM_SHARED) && unlikely(!vma_has_anon_rmap(vma)))
> +		if (vma_is_cow_mapping(vma) && unlikely(!vma_has_anon_rmap(vma)))
>  			vma_end_read(vma);
>  		else
>  			return vma;
> 
> ---
> 
> base-commit: 1d0b04a60516de48250b04227dcf483c6c09aab1
> 
> change-id: 20261014-copy_hugetlb_page_range-743d71a2980e
> 
> --
> 
> Cheers,
> 
> David
> 

-- 
Sincerely yours,
Mike.