Re: [PATCH v2 1/3] userfaultfd: verify VMA state across UFFDIO_COPY retry

[Lorenzo Stoakes <ljs@kernel.org>]

On Thu, May 28, 2026 at 05:41:55PM +0300, Mike Rapoport wrote:
> On Thu, May 28, 2026 at 02:31:00PM +0100, Lorenzo Stoakes wrote:
> > On Wed, May 27, 2026 at 09:47:49PM +0300, Mike Rapoport wrote:
> > > From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> > >
> > > mfill_copy_folio_retry() drops the VMA lock for copy_from_user() and
> > > reacquires it afterwards. The destination VMA can be replaced during that
> > > window.
> > >
> > > The existing check compares vma_uffd_ops() before and after the retry, but
> > > if a shmem VMA with MAP_SHARED is replaced with a shmem VMA with
> > > MAP_PRIVATE (or vice versa) the replacement goes undetected.
> > >
> > > The change from MAP_PRIVATE to MAP_SHARED will treat the folio allocated
> > > with shmem_alloc_folio() as anonymous and this will cause BUG() when
> > > mfill_atomic_install_pte() will try to folio_add_new_anon_rmap().
> > >
> > > The change from MAP_SHARED to MAP_PRIVATE allows injection of folios into
> > > the page cache of the original VMA.
> > >
> > > There is no need to change for hugetlb because it never uses
> > > mfill_copy_folio_retry().
> > >
> > > Introduce helpers for more comprehensive comparison of VMA state:
> > > - mfill_retry_state_save() to save the relevant VMA state into a struct
> > >   mfill_retry_state (original uffd_ops, relevant VMA flags, vm_file and
> > >   pgoff) before dropping the lock
> > > - mfill_retry_state_changed() to compare the saved state with the state
> > >   of the VMA acquired after retaking the locks
> > > - mfill_retry_state_put() to release vm_file pinning.
> > >
> > > Use DEFINE_FREE() cleanup to wrap mfill_retry_state_put() to avoid
> > > complicating error handling paths in mfill_copy_folio_retry().
> > >
> > > Fixes: 292411fda25b ("mm/userfaultfd: detect VMA type change after copy retry in mfill_copy_folio_retry()")
> > > Fixes: 6ab703034f14 ("userfaultfd: mfill_atomic(): remove retry logic")
> >
> > Did we want a Cc: Stable?
>
> Andrew adds it when applying.

Hmm, I didn't think this always happened by default? :)

>
> > > Suggested-by: Peter Xu <peterx@redhat.com>
> > > Co-developed-by: David Carlier <devnexen@gmail.com>
> > > Signed-off-by: David Carlier <devnexen@gmail.com>
> > > Co-developed-by: Michael Bommarito <michael.bommarito@gmail.com>
> > > Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> > > Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> >
> > OK the logic here looks good, thanks for the changes. I have one comment below
> > re: a redundant check, with that addressed feel free to add:
> >
> > Reviewed-by: Lorenzo Stoakes <ljs@kernel.org>
> >
> > > ---
> > >  mm/userfaultfd.c | 85 +++++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 73 insertions(+), 12 deletions(-)
> >
> > > +static bool mfill_retry_state_changed(struct mfill_retry_state *state,
> > > +				      struct vm_area_struct *vma)
> > > +{
> > > +	vma_flags_t flags = vma_flags_and_mask(&vma->flags,
> > > +					       MFILL_RETRY_STATE_VMA_FLAGS);
> > > +
> > > +	/* Have any UFFD flags (missing, WP, minor) changed? */
> > > +	if (!vma_flags_same_pair(&state->flags, &flags))
> > > +		return true;
> > > +
> > > +	/* VMA type or effective uffd_ops changed while the lock was dropped */
> > > +	if (state->ops != vma_uffd_ops(vma))
> > > +		return true;
> > > +
> > > +	/* VMA was anonymous before; changed only if it no longer is */
> > > +	if (!state->file)
> > > +		return !vma_is_anonymous(vma);
> > > +
> > > +	/* VMA was file backed, but file, inode or offset has changed */
> > > +	if (!vma->vm_file || vma->vm_file->f_inode != state->file->f_inode ||
> > > +	    state->file != vma->vm_file || vma->vm_pgoff != state->pgoff)
> > > +		return true;
> >
> > Doesn't state->file != vma->vm_file render the inode check redundant?
>
> Nope, struct file is TYPESAFE_BY_RCU, it can be recycled with the same
> file * pointing to different objects.

Oh lord :)

OK ack on that then!

>
> --
> Sincerely yours,
> Mike.

Cheers, Lorenzo