Re: [PATCH v5] mm/userfaultfd: detect VMA type change after copy retry in mfill_copy_folio_retry()

[Mike Rapoport <rppt@kernel.org>]

Hi Usama,

On Fri, Apr 10, 2026 at 04:48:08AM -0700, Usama Arif wrote:
> On Thu,  9 Apr 2026 13:06:53 +0100 David Carlier <devnexen@gmail.com> wrote:
> 
> > mfill_copy_folio_retry() drops mmap_lock for the copy_from_user() call.
> > During this window, the VMA can be replaced with a different type (e.g.
> > hugetlb), making the caller's ops pointer stale. Subsequent use of the
> > stale ops can lead to incorrect folio handling or a kernel crash.
> > 
> > Pass the caller's ops into mfill_copy_folio_retry() and compare against
> > the current vma_uffd_ops() after re-acquiring the lock. Return -EAGAIN
> > if they differ so the operation can be retried.
> > 
> > Fixes: 59da5c32ffa3 ("userfaultfd: mfill_atomic(): remove retry logic")
> > Signed-off-by: David Carlier <devnexen@gmail.com>
> > ---
> >  mm/userfaultfd.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index 481ec7eb4442..214923a411c1 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -443,7 +443,9 @@ static int mfill_copy_folio_locked(struct folio *folio, unsigned long src_addr)
> >  	return ret;
> >  }
> >  
> > -static int mfill_copy_folio_retry(struct mfill_state *state, struct folio *folio)
> > +static int mfill_copy_folio_retry(struct mfill_state *state,
> > +				  const struct vm_uffd_ops *ops,
> > +				  struct folio *folio)
> >  {
> >  	unsigned long src_addr = state->src_addr;
> >  	void *kaddr;
> > @@ -465,6 +467,14 @@ static int mfill_copy_folio_retry(struct mfill_state *state, struct folio *folio
> >  	if (err)
> >  		return err;
> >  
> > +	/*
> > +	 * The VMA type may have changed while the lock was dropped
> > +	 * (e.g. replaced with a hugetlb mapping), making the caller's
> > +	 * ops pointer stale.
> > +	 */
> > +	if (vma_uffd_ops(state->vma) != ops)
> > +		return -EAGAIN;
> > +
> 
> hmm I am not sure if this is correct for shmem MAP_PRIVATE.
> 
> mfill_atomic_pte_copy() overrides ops to &anon_uffd_ops for MAP_PRIVATE
> mappings:
> 
>     if (!(state->vma->vm_flags & VM_SHARED))
>         ops = &anon_uffd_ops;
> 
> This overridden ops pointer propagates through __mfill_atomic_pte() into
> mfill_copy_folio_retry().  But the new check here calls vma_uffd_ops()
> which returns the original file-backed ops (e.g. &shmem_uffd_ops).
> For shmem MAP_PRIVATE VMAs, the comparison always fails even when
> the VMA type has not changed.

Good catch.

@Andrew, can you please drop the patch for now? 
 
> Maybe save the original (non-overridden) ops before the MAP_PRIVATE override
> and compare against that?

-- 
Sincerely yours,
Mike.