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

[Lorenzo Stoakes <ljs@kernel.org>]

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?

> 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(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 180bad42fc79..e5d2fb3ce2c1 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -14,6 +14,8 @@
>  #include <linux/userfaultfd_k.h>
>  #include <linux/mmu_notifier.h>
>  #include <linux/hugetlb.h>
> +#include <linux/file.h>
> +#include <linux/cleanup.h>
>  #include <asm/tlbflush.h>
>  #include <asm/tlb.h>
>  #include "internal.h"
> @@ -443,16 +445,80 @@ 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,
> +#define MFILL_RETRY_STATE_VMA_FLAGS \
> +	append_vma_flags(__VMA_UFFD_FLAGS, VMA_SHARED_BIT)
> +
> +/*
> + * VMA state saved before dropping the locks in mfill_copy_folio_retry().
> + * Used to detect VMA replacement or incompatible changes after reacquiring the
> + * locks.
> + */
> +struct mfill_retry_state {
> +	const struct vm_uffd_ops *ops;
> +	struct file *file;
> +	vma_flags_t flags;
> +	pgoff_t pgoff;
> +};

Much better thanks!

> +
> +static void mfill_retry_state_save(struct mfill_retry_state *s,
> +				   struct vm_area_struct *vma)
> +{
> +	s->flags = vma_flags_and_mask(&vma->flags, MFILL_RETRY_STATE_VMA_FLAGS);
> +	s->ops = vma_uffd_ops(vma);
> +	s->pgoff = vma->vm_pgoff;
> +
> +	if (vma->vm_file)
> +		s->file = get_file(vma->vm_file);
> +}

Yeah can live with the s here :)

> +
> +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?

> +
> +	return false;
> +}
> +
> +static void mfill_retry_state_put(struct mfill_retry_state *s)
> +{
> +	if (s->file)
> +		fput(s->file);
> +}
> +
> +DEFINE_FREE(retry_put, struct mfill_retry_state *,
> +	    if (_T) mfill_retry_state_put(_T));
> +
> +static int mfill_copy_folio_retry(struct mfill_state *mfill_state,
>  				  struct folio *folio)
>  {
> -	const struct vm_uffd_ops *orig_ops = vma_uffd_ops(state->vma);
> -	unsigned long src_addr = state->src_addr;
> +	struct mfill_retry_state retry_state = { 0 };
> +	struct mfill_retry_state *for_free __free(retry_put) = &retry_state;
> +	unsigned long src_addr = mfill_state->src_addr;
>  	void *kaddr;
>  	int err;
>
> +	mfill_retry_state_save(&retry_state, mfill_state->vma);
> +
>  	/* retry copying with mm_lock dropped */
> -	mfill_put_vma(state);
> +	mfill_put_vma(mfill_state);
>
>  	kaddr = kmap_local_folio(folio, 0);
>  	err = copy_from_user(kaddr, (const void __user *) src_addr, PAGE_SIZE);
> @@ -463,19 +529,14 @@ static int mfill_copy_folio_retry(struct mfill_state *state,
>  	flush_dcache_folio(folio);
>
>  	/* reget VMA and PMD, they could change underneath us */
> -	err = mfill_get_vma(state);
> +	err = mfill_get_vma(mfill_state);
>  	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) != orig_ops)
> +	if (mfill_retry_state_changed(&retry_state, mfill_state->vma))
>  		return -EAGAIN;
>
> -	err = mfill_establish_pmd(state);
> +	err = mfill_establish_pmd(mfill_state);
>  	if (err)
>  		return err;
>
> --
> 2.53.0
>