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

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

[Mike Rapoport]

From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>

... and two more small fixes.

This applies on top of the current mm-unstable with
"userfaultfd: snapshot VMA state across UFFDIO_COPY retry"
reverted.

v2 changes:
* rename vma_snapshot to mfill_retry_state and related functions,
  variables and constants 
* drop redundant check for effective ops
* drop vma_uffd_copy_ops()
* restore large comment about MAP_PRIVATE handling and massage other
  comments
* new patches:
  - guard against bugs that could end up with NULL ops in
    __mfill_atomic_pte()
  - remove redundant check in vm_uffd_ops()

v1: https://lore.kernel.org/all/20260519052516.3315196-1-rppt@kernel.org

Mike Rapoport (Microsoft) (3):
  userfaultfd: verify VMA state across UFFDIO_COPY retry
  userfaultfd: refuse to __mfill_atomic_pte() for unsupported VMAs
  userfaultfd: remove redundant check in vm_uffd_ops()

 mm/userfaultfd.c | 92 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 79 insertions(+), 13 deletions(-)

-- 
2.53.0

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

[Mike Rapoport]

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")
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>
---
 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;
+};
+
+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);
+}
+
+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;
+
+	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

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

[Lorenzo Stoakes]

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
>

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

[Mike Rapoport]

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.
 
> > 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.

-- 
Sincerely yours,
Mike.

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

[Andrew Morton]

On Thu, 28 May 2026 17:41:55 +0300 Mike Rapoport <rppt@kernel.org> wrote:

> > > 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.

Both the above commits were added to 7.1-rc1, so I haven't added cc:stable.

[PATCH v2 2/3] userfaultfd: refuse to __mfill_atomic_pte() for unsupported VMAs

[Mike Rapoport]

From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>

__mfill_atomic_pte() unconditionally dereferences ops because there is an
assumption that VMAs that can undergo mfill_* operations are vetted on
registration and must have valid vm_uffd_ops.

Add a guard against potential bugs and make sure __mfill_atomic_pte() bails
out if ops is NULL.

Suggested-by: Lorenzo Stoakes <ljs@kernel.org>
Fixes: ad9ac3081332 ("userfaultfd: introduce vm_uffd_ops->alloc_folio()")
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
 mm/userfaultfd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index e5d2fb3ce2c1..2872c71bbf36 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -552,6 +552,11 @@ static int __mfill_atomic_pte(struct mfill_state *state,
 	struct folio *folio;
 	int ret;
 
+	if (!ops) {
+		VM_WARN_ONCE(1, "UFFDIO_COPY for unsupported VMA");
+		return -EOPNOTSUPP;
+	}
+
 	folio = ops->alloc_folio(state->vma, state->dst_addr);
 	if (!folio)
 		return -ENOMEM;
-- 
2.53.0

Re: [PATCH v2 2/3] userfaultfd: refuse to __mfill_atomic_pte() for unsupported VMAs

[David CARLIER]

Hi,

just chiming it

On Wed, 27 May 2026 at 19:48, Mike Rapoport <rppt@kernel.org> wrote:
>
> From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
>
> __mfill_atomic_pte() unconditionally dereferences ops because there is an
> assumption that VMAs that can undergo mfill_* operations are vetted on
> registration and must have valid vm_uffd_ops.
>
> Add a guard against potential bugs and make sure __mfill_atomic_pte() bails
> out if ops is NULL.
>
> Suggested-by: Lorenzo Stoakes <ljs@kernel.org>
> Fixes: ad9ac3081332 ("userfaultfd: introduce vm_uffd_ops->alloc_folio()")
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> ---
>  mm/userfaultfd.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index e5d2fb3ce2c1..2872c71bbf36 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -552,6 +552,11 @@ static int __mfill_atomic_pte(struct mfill_state *state,
>         struct folio *folio;
>         int ret;
>
> +       if (!ops) {
> +               VM_WARN_ONCE(1, "UFFDIO_COPY for unsupported VMA");

nit: Is the warning message a bit too specific ? i.e. Also handle
MFILL_ATOMIC_ZEROPAGE case.


> +               return -EOPNOTSUPP;
> +       }
> +
>         folio = ops->alloc_folio(state->vma, state->dst_addr);
>         if (!folio)
>                 return -ENOMEM;
> --
> 2.53.0
>

Cheers.

Re: [PATCH v2 2/3] userfaultfd: refuse to __mfill_atomic_pte() for unsupported VMAs

[Mike Rapoport]

On Wed, May 27, 2026 at 08:09:01PM +0100, David CARLIER wrote:
> Hi,
> 
> just chiming it
> 
> On Wed, 27 May 2026 at 19:48, Mike Rapoport <rppt@kernel.org> wrote:
> >
> > From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> >
> > __mfill_atomic_pte() unconditionally dereferences ops because there is an
> > assumption that VMAs that can undergo mfill_* operations are vetted on
> > registration and must have valid vm_uffd_ops.
> >
> > Add a guard against potential bugs and make sure __mfill_atomic_pte() bails
> > out if ops is NULL.
> >
> > Suggested-by: Lorenzo Stoakes <ljs@kernel.org>
> > Fixes: ad9ac3081332 ("userfaultfd: introduce vm_uffd_ops->alloc_folio()")
> > Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> > ---
> >  mm/userfaultfd.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index e5d2fb3ce2c1..2872c71bbf36 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -552,6 +552,11 @@ static int __mfill_atomic_pte(struct mfill_state *state,
> >         struct folio *folio;
> >         int ret;
> >
> > +       if (!ops) {
> > +               VM_WARN_ONCE(1, "UFFDIO_COPY for unsupported VMA");
> 
> nit: Is the warning message a bit too specific ? i.e. Also handle
> MFILL_ATOMIC_ZEROPAGE case.

It handles ZEROPAGE case by actually copying a zeroed page there so in a
sense it's still a copy.

Differentiating COPY and ZEROPAGE here will add complexity that is not
justified for a message that should be never printed.
 
> > +               return -EOPNOTSUPP;
> > +       }
> > +
> >         folio = ops->alloc_folio(state->vma, state->dst_addr);
> >         if (!folio)
> >                 return -ENOMEM;
> > --
> > 2.53.0
> >
> 
> Cheers.
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 2/3] userfaultfd: refuse to __mfill_atomic_pte() for unsupported VMAs

[David CARLIER]

On Thu, 28 May 2026 at 08:33, Mike Rapoport <rppt@kernel.org> wrote:
>
> On Wed, May 27, 2026 at 08:09:01PM +0100, David CARLIER wrote:
> > Hi,
> >
> > just chiming it
> >
> > On Wed, 27 May 2026 at 19:48, Mike Rapoport <rppt@kernel.org> wrote:
> > >
> > > From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> > >
> > > __mfill_atomic_pte() unconditionally dereferences ops because there is an
> > > assumption that VMAs that can undergo mfill_* operations are vetted on
> > > registration and must have valid vm_uffd_ops.
> > >
> > > Add a guard against potential bugs and make sure __mfill_atomic_pte() bails
> > > out if ops is NULL.
> > >
> > > Suggested-by: Lorenzo Stoakes <ljs@kernel.org>
> > > Fixes: ad9ac3081332 ("userfaultfd: introduce vm_uffd_ops->alloc_folio()")
> > > Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> > > ---
> > >  mm/userfaultfd.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > index e5d2fb3ce2c1..2872c71bbf36 100644
> > > --- a/mm/userfaultfd.c
> > > +++ b/mm/userfaultfd.c
> > > @@ -552,6 +552,11 @@ static int __mfill_atomic_pte(struct mfill_state *state,
> > >         struct folio *folio;
> > >         int ret;
> > >
> > > +       if (!ops) {
> > > +               VM_WARN_ONCE(1, "UFFDIO_COPY for unsupported VMA");
> >
> > nit: Is the warning message a bit too specific ? i.e. Also handle
> > MFILL_ATOMIC_ZEROPAGE case.
>
> It handles ZEROPAGE case by actually copying a zeroed page there so in a
> sense it's still a copy.
>
> Differentiating COPY and ZEROPAGE here will add complexity that is not
> justified for a message that should be never printed.
>
> > > +               return -EOPNOTSUPP;
> > > +       }
> > > +
> > >         folio = ops->alloc_folio(state->vma, state->dst_addr);
> > >         if (!folio)
> > >                 return -ENOMEM;
> > > --
> > > 2.53.0
> > >
> >
> > Cheers.
> >
>
> --
> Sincerely yours,
> Mike.

ACK.

Reviewed-by: David CARLIER <devnexen@gmail.com>

Re: [PATCH v2 2/3] userfaultfd: refuse to __mfill_atomic_pte() for unsupported VMAs

[Lorenzo Stoakes]

On Wed, May 27, 2026 at 09:47:50PM +0300, Mike Rapoport wrote:
> From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
>
> __mfill_atomic_pte() unconditionally dereferences ops because there is an
> assumption that VMAs that can undergo mfill_* operations are vetted on
> registration and must have valid vm_uffd_ops.
>
> Add a guard against potential bugs and make sure __mfill_atomic_pte() bails
> out if ops is NULL.
>
> Suggested-by: Lorenzo Stoakes <ljs@kernel.org>
> Fixes: ad9ac3081332 ("userfaultfd: introduce vm_uffd_ops->alloc_folio()")
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

LGTM, so:

Reviewed-by: Lorenzo Stoakes <ljs@kernel.org>

> ---
>  mm/userfaultfd.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index e5d2fb3ce2c1..2872c71bbf36 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -552,6 +552,11 @@ static int __mfill_atomic_pte(struct mfill_state *state,
>  	struct folio *folio;
>  	int ret;
>
> +	if (!ops) {
> +		VM_WARN_ONCE(1, "UFFDIO_COPY for unsupported VMA");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	folio = ops->alloc_folio(state->vma, state->dst_addr);
>  	if (!folio)
>  		return -ENOMEM;
> --
> 2.53.0
>

[PATCH v2 3/3] userfaultfd: remove redundant check in vm_uffd_ops()

[Mike Rapoport]

From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>

Lorenzo says:

  static const struct vm_uffd_ops *vma_uffd_ops(struct vm_area_struct *vma)
  {
          if (vma_is_anonymous(vma))
                  return &anon_uffd_ops;
          return vma->vm_ops ? vma->vm_ops->uffd_ops : NULL;
  }

  This is doing a redundant check _and_ making life confusing, as if
  !vma->vm_ops is a condition that can be reached there, it can't, as
  vma_is_anonymous() is literally a !vma->vm_ops check :)

Remove the redundant check.

Suggested-by: Lorenzo Stoakes <ljs@kernel.org>
Fixes: 0f48947c4232 ("userfaultfd: introduce vm_uffd_ops")
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
 mm/userfaultfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 2872c71bbf36..80cc8be5725f 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -68,7 +68,7 @@ static const struct vm_uffd_ops *vma_uffd_ops(struct vm_area_struct *vma)
 {
 	if (vma_is_anonymous(vma))
 		return &anon_uffd_ops;
-	return vma->vm_ops ? vma->vm_ops->uffd_ops : NULL;
+	return vma->vm_ops->uffd_ops;
 }
 
 static __always_inline
-- 
2.53.0

Re: [PATCH v2 3/3] userfaultfd: remove redundant check in vm_uffd_ops()

[Lorenzo Stoakes]

On Wed, May 27, 2026 at 09:47:51PM +0300, Mike Rapoport wrote:
> From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
>
> Lorenzo says:
>
>   static const struct vm_uffd_ops *vma_uffd_ops(struct vm_area_struct *vma)
>   {
>           if (vma_is_anonymous(vma))
>                   return &anon_uffd_ops;
>           return vma->vm_ops ? vma->vm_ops->uffd_ops : NULL;
>   }
>
>   This is doing a redundant check _and_ making life confusing, as if
>   !vma->vm_ops is a condition that can be reached there, it can't, as
>   vma_is_anonymous() is literally a !vma->vm_ops check :)
>
> Remove the redundant check.
>
> Suggested-by: Lorenzo Stoakes <ljs@kernel.org>
> Fixes: 0f48947c4232 ("userfaultfd: introduce vm_uffd_ops")
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

:) thanks! LGTM so:

Reviewed-by: Lorenzo Stoakes <ljs@kernel.org>

> ---
>  mm/userfaultfd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 2872c71bbf36..80cc8be5725f 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -68,7 +68,7 @@ static const struct vm_uffd_ops *vma_uffd_ops(struct vm_area_struct *vma)
>  {
>  	if (vma_is_anonymous(vma))
>  		return &anon_uffd_ops;
> -	return vma->vm_ops ? vma->vm_ops->uffd_ops : NULL;
> +	return vma->vm_ops->uffd_ops;
>  }
>
>  static __always_inline
> --
> 2.53.0
>

Cheers, Lorenzo