[PATCH 0/3] mm: userfaultfd: assorted fixes and cleanups

[PATCH 0/3] mm: userfaultfd: assorted fixes and cleanups

[Tal Zussman]

Two fixes and one cleanup for userfaultfd.

The second patch is a more of an RFC, as it changes previously allowed
user behavior. However, being able to unregister memory through a uffd
different from the uffd it was originally registered with seems odd enough
that I'd argue it's a bug :)

---
Tal Zussman (3):
      userfaultfd: correctly prevent registering VM_DROPPABLE regions
      userfaultfd: prevent unregistering VMAs through a different userfaultfd
      userfaultfd: remove UFFD_CLOEXEC, UFFD_NONBLOCK, and UFFD_FLAGS_SET

 fs/userfaultfd.c              | 17 +++++++++++++----
 include/linux/userfaultfd_k.h |  6 +-----
 2 files changed, 14 insertions(+), 9 deletions(-)
---
base-commit: 546b1c9e93c2bb8cf5ed24e0be1c86bb089b3253
change-id: 20250531-uffd-fixes-142331b15e63

Best regards,
-- 
Tal Zussman <tz2294@columbia.edu>

[PATCH 1/3] userfaultfd: correctly prevent registering VM_DROPPABLE regions

[Tal Zussman]

vma_can_userfault() masks off non-userfaultfd VM flags from vm_flags.
The vm_flags & VM_DROPPABLE test will then always be false, incorrectly
allowing VM_DROPPABLE regions to be registered with userfaultfd.

Additionally, vm_flags is not guaranteed to correspond to the actual
VMA's flags. Fix this test by checking the VMA's flags directly.

Link: https://lore.kernel.org/linux-mm/5a875a3a-2243-4eab-856f-bc53ccfec3ea@redhat.com/
Fixes: 9651fcedf7b9 ("mm: add MAP_DROPPABLE for designating always lazily freeable mappings")
Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 include/linux/userfaultfd_k.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index 75342022d144..f3b3d2c9dd5e 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -218,7 +218,7 @@ static inline bool vma_can_userfault(struct vm_area_struct *vma,
 {
 	vm_flags &= __VM_UFFD_FLAGS;
 
-	if (vm_flags & VM_DROPPABLE)
+	if (vma->vm_flags & VM_DROPPABLE)
 		return false;
 
 	if ((vm_flags & VM_UFFD_MINOR) &&

-- 
2.39.5

[PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[Tal Zussman]

Currently, a VMA registered with a uffd can be unregistered through a
different uffd asssociated with the same mm_struct.

Change this behavior to be stricter by requiring VMAs to be unregistered
through the same uffd they were registered with.

While at it, correct the comment for the no userfaultfd case. This seems
to be a copy-paste artifact from the analagous userfaultfd_register()
check.

Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 fs/userfaultfd.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 22f4bf956ba1..9289e30b24c4 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1477,6 +1477,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
 		if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
 			goto out_unlock;
 
+		/*
+		 * Check that this vma isn't already owned by a different
+		 * userfaultfd. This provides for more strict behavior by
+		 * preventing a VMA registered with a userfaultfd from being
+		 * unregistered through a different userfaultfd.
+		 */
+		if (cur->vm_userfaultfd_ctx.ctx &&
+		    cur->vm_userfaultfd_ctx.ctx != ctx)
+			goto out_unlock;
+
 		found = true;
 	} for_each_vma_range(vmi, cur, end);
 	BUG_ON(!found);
@@ -1491,10 +1501,11 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
 		cond_resched();
 
 		BUG_ON(!vma_can_userfault(vma, vma->vm_flags, wp_async));
+		BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
+		       vma->vm_userfaultfd_ctx.ctx != ctx);
 
 		/*
-		 * Nothing to do: this vma is already registered into this
-		 * userfaultfd and with the right tracking mode too.
+		 * Nothing to do: this vma is not registered with userfaultfd.
 		 */
 		if (!vma->vm_userfaultfd_ctx.ctx)
 			goto skip;

-- 
2.39.5

[PATCH 3/3] userfaultfd: remove UFFD_CLOEXEC, UFFD_NONBLOCK, and UFFD_FLAGS_SET

[Tal Zussman]

UFFD_CLOEXEC, UFFD_NONBLOCK, and UFFD_FLAGS_SET have been unused since they
were added in commit 932b18e0aec6 ("userfaultfd: linux/userfaultfd_k.h").
Remove them and the associated BUILD_BUG_ON() checks.

Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 fs/userfaultfd.c              | 2 --
 include/linux/userfaultfd_k.h | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 9289e30b24c4..00c6662ed9a5 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -2121,8 +2121,6 @@ static int new_userfaultfd(int flags)
 
 	/* Check the UFFD_* constants for consistency.  */
 	BUILD_BUG_ON(UFFD_USER_MODE_ONLY & UFFD_SHARED_FCNTL_FLAGS);
-	BUILD_BUG_ON(UFFD_CLOEXEC != O_CLOEXEC);
-	BUILD_BUG_ON(UFFD_NONBLOCK != O_NONBLOCK);
 
 	if (flags & ~(UFFD_SHARED_FCNTL_FLAGS | UFFD_USER_MODE_ONLY))
 		return -EINVAL;
diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index f3b3d2c9dd5e..ccad58602846 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -30,11 +30,7 @@
  * from userfaultfd, in order to leave a free define-space for
  * shared O_* flags.
  */
-#define UFFD_CLOEXEC O_CLOEXEC
-#define UFFD_NONBLOCK O_NONBLOCK
-
 #define UFFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
-#define UFFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS)
 
 /*
  * Start with fault_pending_wqh and fault_wqh so they're more likely

-- 
2.39.5

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[James Houghton]

On Tue, Jun 3, 2025 at 3:15 PM Tal Zussman <tz2294@columbia.edu> wrote:
>
> Currently, a VMA registered with a uffd can be unregistered through a
> different uffd asssociated with the same mm_struct.
>
> Change this behavior to be stricter by requiring VMAs to be unregistered
> through the same uffd they were registered with.
>
> While at it, correct the comment for the no userfaultfd case. This seems
> to be a copy-paste artifact from the analagous userfaultfd_register()
> check.
>
> Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>

Thanks, Tal! I like this patch, but I can't really meaningfully
comment on if it's worth it to change the UAPI.

> ---
>  fs/userfaultfd.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 22f4bf956ba1..9289e30b24c4 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1477,6 +1477,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>                 if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
>                         goto out_unlock;
>
> +               /*
> +                * Check that this vma isn't already owned by a different
> +                * userfaultfd. This provides for more strict behavior by
> +                * preventing a VMA registered with a userfaultfd from being
> +                * unregistered through a different userfaultfd.
> +                */
> +               if (cur->vm_userfaultfd_ctx.ctx &&
> +                   cur->vm_userfaultfd_ctx.ctx != ctx)
> +                       goto out_unlock;
> +

Very minor nitpick: I think this check should go above the
!vma_can_userfault() check above, as `wp_async` was derived from
`ctx`, not `cur->vm_userfaultfd_ctx.ctx`.

>                 found = true;
>         } for_each_vma_range(vmi, cur, end);

I don't really like this for_each_vma_range() for loop, but I guess it
is meaningful to the user: invalid unregistration attempts will fail
quickly instead of potentially making some progress. So unfortunately,
without a good reason, I suppose we can't get rid of it. :(

>         BUG_ON(!found);
> @@ -1491,10 +1501,11 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>                 cond_resched();
>
>                 BUG_ON(!vma_can_userfault(vma, vma->vm_flags, wp_async));
> +               BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> +                      vma->vm_userfaultfd_ctx.ctx != ctx);

IMO, this new BUG_ON should either be
(1) moved and should not be a BUG_ON. See the WARN_ON_ONCE() below,
OR
(2) removed.

Perhaps the older BUG_ON() should be removed/changed too.

>
>                 /*
> -                * Nothing to do: this vma is already registered into this
> -                * userfaultfd and with the right tracking mode too.
> +                * Nothing to do: this vma is not registered with userfaultfd.
>                  */
>                 if (!vma->vm_userfaultfd_ctx.ctx)
>                         goto skip;

if (WARN_ON_ONCE(vmx->vm_userfaultfd_ctx.ctx != ctx)) {
    ret = -EINVAL;
    break;
}

where the WARN_ON_ONCE() indicates that the VMA should have been
filtered out earlier. The WARN_ON_ONCE() isn't even really necessary.


>
> --
> 2.39.5
>
>

Re: [PATCH 1/3] userfaultfd: correctly prevent registering VM_DROPPABLE regions

[David Hildenbrand]

On 04.06.25 00:14, Tal Zussman wrote:
> vma_can_userfault() masks off non-userfaultfd VM flags from vm_flags.
> The vm_flags & VM_DROPPABLE test will then always be false, incorrectly
> allowing VM_DROPPABLE regions to be registered with userfaultfd.
> 
> Additionally, vm_flags is not guaranteed to correspond to the actual
> VMA's flags. Fix this test by checking the VMA's flags directly.
> 
> Link: https://lore.kernel.org/linux-mm/5a875a3a-2243-4eab-856f-bc53ccfec3ea@redhat.com/
> Fixes: 9651fcedf7b9 ("mm: add MAP_DROPPABLE for designating always lazily freeable mappings")
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> ---
>   include/linux/userfaultfd_k.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> index 75342022d144..f3b3d2c9dd5e 100644
> --- a/include/linux/userfaultfd_k.h
> +++ b/include/linux/userfaultfd_k.h
> @@ -218,7 +218,7 @@ static inline bool vma_can_userfault(struct vm_area_struct *vma,
>   {
>   	vm_flags &= __VM_UFFD_FLAGS;
>   
> -	if (vm_flags & VM_DROPPABLE)
> +	if (vma->vm_flags & VM_DROPPABLE)
>   		return false;
>   
>   	if ((vm_flags & VM_UFFD_MINOR) &&

Nice catch!

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[David Hildenbrand]

On 04.06.25 00:14, Tal Zussman wrote:
> Currently, a VMA registered with a uffd can be unregistered through a
> different uffd asssociated with the same mm_struct.
> 
> Change this behavior to be stricter by requiring VMAs to be unregistered
> through the same uffd they were registered with.
> 
> While at it, correct the comment for the no userfaultfd case. This seems
> to be a copy-paste artifact from the analagous userfaultfd_register()
> check.

I consider it a BUG that should be fixed. Hoping Peter can share his 
opinion.

> 
> Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> ---
>   fs/userfaultfd.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 22f4bf956ba1..9289e30b24c4 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1477,6 +1477,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>   		if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
>   			goto out_unlock;
>   
> +		/*
> +		 * Check that this vma isn't already owned by a different
> +		 * userfaultfd. This provides for more strict behavior by
> +		 * preventing a VMA registered with a userfaultfd from being
> +		 * unregistered through a different userfaultfd.
> +		 */
> +		if (cur->vm_userfaultfd_ctx.ctx &&
> +		    cur->vm_userfaultfd_ctx.ctx != ctx)
> +			goto out_unlock;

So we allow !cur->vm_userfaultfd_ctx.ctx to allow unregistering when 
there was nothing registered.

A bit weird to set "found = true" in that case. Maybe it's fine, just 
raising it ...

> +
>   		found = true;
>   	} for_each_vma_range(vmi, cur, end);
>   	BUG_ON(!found);
> @@ -1491,10 +1501,11 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>   		cond_resched();
>   
>   		BUG_ON(!vma_can_userfault(vma, vma->vm_flags, wp_async));
> +		BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> +		       vma->vm_userfaultfd_ctx.ctx != ctx);
>   

No new BUG_ON please. VM_WARN_ON_ONCE() if we really care. After all, we 
checked this above ...

>   		/*
> -		 * Nothing to do: this vma is already registered into this
> -		 * userfaultfd and with the right tracking mode too.
> +		 * Nothing to do: this vma is not registered with userfaultfd.
>   		 */
>   		if (!vma->vm_userfaultfd_ctx.ctx)
>   			goto skip;
> 


-- 
Cheers,

David / dhildenb

Re: [PATCH 3/3] userfaultfd: remove UFFD_CLOEXEC, UFFD_NONBLOCK, and UFFD_FLAGS_SET

[David Hildenbrand]

On 04.06.25 00:14, Tal Zussman wrote:
> UFFD_CLOEXEC, UFFD_NONBLOCK, and UFFD_FLAGS_SET have been unused since they
> were added in commit 932b18e0aec6 ("userfaultfd: linux/userfaultfd_k.h").
> Remove them and the associated BUILD_BUG_ON() checks.
> 
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[Peter Xu]

On Wed, Jun 04, 2025 at 03:23:38PM +0200, David Hildenbrand wrote:
> On 04.06.25 00:14, Tal Zussman wrote:
> > Currently, a VMA registered with a uffd can be unregistered through a
> > different uffd asssociated with the same mm_struct.
> > 
> > Change this behavior to be stricter by requiring VMAs to be unregistered
> > through the same uffd they were registered with.
> > 
> > While at it, correct the comment for the no userfaultfd case. This seems
> > to be a copy-paste artifact from the analagous userfaultfd_register()
> > check.
> 
> I consider it a BUG that should be fixed. Hoping Peter can share his
> opinion.

Agree it smells like unintentional, it's just that the man page indeed
didn't mention what would happen if the userfaultfd isn't the one got
registered but only requesting them to be "compatible".

DESCRIPTION
       Unregister a memory address range from userfaultfd.  The pages in
       the range must be “compatible” (see UFFDIO_REGISTER(2const)).

So it sounds still possible if we have existing userapp creating multiple
userfaultfds (for example, for scalability reasons on using multiple
queues) to manage its own mm address space, one uffd in charge of a portion
of VMAs, then it can randomly take one userfaultfd to do unregistrations.
Such might break.

> 
> > 
> > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> > Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> > ---
> >   fs/userfaultfd.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 22f4bf956ba1..9289e30b24c4 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -1477,6 +1477,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >   		if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
> >   			goto out_unlock;
> > +		/*
> > +		 * Check that this vma isn't already owned by a different
> > +		 * userfaultfd. This provides for more strict behavior by
> > +		 * preventing a VMA registered with a userfaultfd from being
> > +		 * unregistered through a different userfaultfd.
> > +		 */
> > +		if (cur->vm_userfaultfd_ctx.ctx &&
> > +		    cur->vm_userfaultfd_ctx.ctx != ctx)
> > +			goto out_unlock;
> 
> So we allow !cur->vm_userfaultfd_ctx.ctx to allow unregistering when there
> was nothing registered.
> 
> A bit weird to set "found = true" in that case. Maybe it's fine, just
> raising it ...

This part should be ok, as found is defined as:

	/*
	 * Search for not compatible vmas.
	 */
	found = false;

So it's still compatible VMA even if not registered.

It's just that I'm not yet sure how this change benefits the kernel
(besides the API can look slightly cleaner).  There seems to still have a
low risk of breaking userapps.  It could be a matter of whether there can
be any real security concerns.

If not, maybe we don't need to risk such a change for almost nothing (I
almost never think "API cleaness" a goal when it's put together with
compatilibities).

Thanks,

-- 
Peter Xu

Re: [PATCH 1/3] userfaultfd: correctly prevent registering VM_DROPPABLE regions

[Peter Xu]

On Tue, Jun 03, 2025 at 06:14:20PM -0400, Tal Zussman wrote:
> vma_can_userfault() masks off non-userfaultfd VM flags from vm_flags.
> The vm_flags & VM_DROPPABLE test will then always be false, incorrectly
> allowing VM_DROPPABLE regions to be registered with userfaultfd.
> 
> Additionally, vm_flags is not guaranteed to correspond to the actual
> VMA's flags. Fix this test by checking the VMA's flags directly.
> 
> Link: https://lore.kernel.org/linux-mm/5a875a3a-2243-4eab-856f-bc53ccfec3ea@redhat.com/
> Fixes: 9651fcedf7b9 ("mm: add MAP_DROPPABLE for designating always lazily freeable mappings")
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 3/3] userfaultfd: remove UFFD_CLOEXEC, UFFD_NONBLOCK, and UFFD_FLAGS_SET

[Peter Xu]

On Tue, Jun 03, 2025 at 06:14:22PM -0400, Tal Zussman wrote:
> UFFD_CLOEXEC, UFFD_NONBLOCK, and UFFD_FLAGS_SET have been unused since they
> were added in commit 932b18e0aec6 ("userfaultfd: linux/userfaultfd_k.h").
> Remove them and the associated BUILD_BUG_ON() checks.
> 
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[Tal Zussman]

On Tue, Jun 3, 2025 at 8:52 PM James Houghton <jthoughton@google.com> wrote:
>
> On Tue, Jun 3, 2025 at 3:15 PM Tal Zussman <tz2294@columbia.edu> wrote:
> >
> > Currently, a VMA registered with a uffd can be unregistered through a
> > different uffd asssociated with the same mm_struct.
> >
> > Change this behavior to be stricter by requiring VMAs to be unregistered
> > through the same uffd they were registered with.
> >
> > While at it, correct the comment for the no userfaultfd case. This seems
> > to be a copy-paste artifact from the analagous userfaultfd_register()
> > check.
> >
> > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> > Signed-off-by: Tal Zussman <tz2294@columbia.edu>
>
> Thanks, Tal! I like this patch, but I can't really meaningfully
> comment on if it's worth it to change the UAPI.
>
> > ---
> >  fs/userfaultfd.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 22f4bf956ba1..9289e30b24c4 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -1477,6 +1477,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >                 if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
> >                         goto out_unlock;
> >
> > +               /*
> > +                * Check that this vma isn't already owned by a different
> > +                * userfaultfd. This provides for more strict behavior by
> > +                * preventing a VMA registered with a userfaultfd from being
> > +                * unregistered through a different userfaultfd.
> > +                */
> > +               if (cur->vm_userfaultfd_ctx.ctx &&
> > +                   cur->vm_userfaultfd_ctx.ctx != ctx)
> > +                       goto out_unlock;
> > +
>
> Very minor nitpick: I think this check should go above the
> !vma_can_userfault() check above, as `wp_async` was derived from
> `ctx`, not `cur->vm_userfaultfd_ctx.ctx`.

Thanks, this is a good point! I'll change it for v2.

This also seems to indicate that the current behavior is broken and may reject
unregistering some VMAs incorrectly. For example, a file-backed VMA registered
with `wp_async` and UFFD_WP cannot be unregistered through a VMA that does not
have `wp_async` set.

> >                 found = true;
> >         } for_each_vma_range(vmi, cur, end);
>
> I don't really like this for_each_vma_range() for loop, but I guess it
> is meaningful to the user: invalid unregistration attempts will fail
> quickly instead of potentially making some progress. So unfortunately,
> without a good reason, I suppose we can't get rid of it. :(
>
> >         BUG_ON(!found);
> > @@ -1491,10 +1501,11 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >                 cond_resched();
> >
> >                 BUG_ON(!vma_can_userfault(vma, vma->vm_flags, wp_async));
> > +               BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> > +                      vma->vm_userfaultfd_ctx.ctx != ctx);
>
> IMO, this new BUG_ON should either be
> (1) moved and should not be a BUG_ON. See the WARN_ON_ONCE() below,
> OR
> (2) removed.
>
> Perhaps the older BUG_ON() should be removed/changed too.

I added this mainly to maintain symmetry with the userfaulfd_register()
implementation. I'm happy to leave it out, so  I'll either convert it, and
the other one, to a VM_WARN_ON_ONCE(), as per David, or remove it.

> >
> >                 /*
> > -                * Nothing to do: this vma is already registered into this
> > -                * userfaultfd and with the right tracking mode too.
> > +                * Nothing to do: this vma is not registered with userfaultfd.
> >                  */
> >                 if (!vma->vm_userfaultfd_ctx.ctx)
> >                         goto skip;
>
> if (WARN_ON_ONCE(vmx->vm_userfaultfd_ctx.ctx != ctx)) {
>     ret = -EINVAL;
>     break;
> }
>
> where the WARN_ON_ONCE() indicates that the VMA should have been
> filtered out earlier. The WARN_ON_ONCE() isn't even really necessary.
>
>
> >
> > --
> > 2.39.5
> >
> >

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[Tal Zussman]

On Wed, Jun 4, 2025 at 9:23 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 04.06.25 00:14, Tal Zussman wrote:
> > Currently, a VMA registered with a uffd can be unregistered through a
> > different uffd asssociated with the same mm_struct.
> >
> > Change this behavior to be stricter by requiring VMAs to be unregistered
> > through the same uffd they were registered with.
> >
> > While at it, correct the comment for the no userfaultfd case. This seems
> > to be a copy-paste artifact from the analagous userfaultfd_register()
> > check.
>
> I consider it a BUG that should be fixed. Hoping Peter can share his
> opinion.
>
> >
> > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> > Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> > ---
> >   fs/userfaultfd.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 22f4bf956ba1..9289e30b24c4 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -1477,6 +1477,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >   if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
> >   goto out_unlock;
> >
> > + /*
> > + * Check that this vma isn't already owned by a different
> > + * userfaultfd. This provides for more strict behavior by
> > + * preventing a VMA registered with a userfaultfd from being
> > + * unregistered through a different userfaultfd.
> > + */
> > + if (cur->vm_userfaultfd_ctx.ctx &&
> > +    cur->vm_userfaultfd_ctx.ctx != ctx)
> > + goto out_unlock;
>
> So we allow !cur->vm_userfaultfd_ctx.ctx to allow unregistering when
> there was nothing registered.
>
> A bit weird to set "found = true" in that case. Maybe it's fine, just
> raising it ...
>
> > +
> >   found = true;
> >   } for_each_vma_range(vmi, cur, end);
> >   BUG_ON(!found);
> > @@ -1491,10 +1501,11 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> >   cond_resched();
> >
> >   BUG_ON(!vma_can_userfault(vma, vma->vm_flags, wp_async));
> > + BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> > +       vma->vm_userfaultfd_ctx.ctx != ctx);
> >
>
> No new BUG_ON please. VM_WARN_ON_ONCE() if we really care. After all, we
> checked this above ...

Yeah, I mainly added this to maintain symmetry with userfaultfd_register().
I don't think it's really necessary to add this, so I'll remove it for v2.

I'm happy to send another patch (preceding this one) converting all of the
pre-existing userfaultfd BUG_ON()s to VM_WARN_ON_ONCE(). Although I do see
that all uses of VM_WARN_ON_ONCE() are in arch/ or mm/ code, while this file
is under fs/. Is that fine? Alternatively, I can remove them, but I defer to
you.

> >   /*
> > - * Nothing to do: this vma is already registered into this
> > - * userfaultfd and with the right tracking mode too.
> > + * Nothing to do: this vma is not registered with userfaultfd.
> >   */
> >   if (!vma->vm_userfaultfd_ctx.ctx)
> >   goto skip;
> >
>
>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[David Hildenbrand]

On 04.06.25 17:09, Peter Xu wrote:
> On Wed, Jun 04, 2025 at 03:23:38PM +0200, David Hildenbrand wrote:
>> On 04.06.25 00:14, Tal Zussman wrote:
>>> Currently, a VMA registered with a uffd can be unregistered through a
>>> different uffd asssociated with the same mm_struct.
>>>
>>> Change this behavior to be stricter by requiring VMAs to be unregistered
>>> through the same uffd they were registered with.
>>>
>>> While at it, correct the comment for the no userfaultfd case. This seems
>>> to be a copy-paste artifact from the analagous userfaultfd_register()
>>> check.
>>
>> I consider it a BUG that should be fixed. Hoping Peter can share his
>> opinion.
> 
> Agree it smells like unintentional, it's just that the man page indeed
> didn't mention what would happen if the userfaultfd isn't the one got
> registered but only requesting them to be "compatible".
> 
> DESCRIPTION
>         Unregister a memory address range from userfaultfd.  The pages in
>         the range must be “compatible” (see UFFDIO_REGISTER(2const)).
> 
> So it sounds still possible if we have existing userapp creating multiple
> userfaultfds (for example, for scalability reasons on using multiple
> queues) to manage its own mm address space, one uffd in charge of a portion
> of VMAs, then it can randomly take one userfaultfd to do unregistrations.
> Such might break.

Not sure if relevant, but consider the following:

an app being controlled by another process using userfaultfd.

The app itself can "escape" uffd control of the other process by simply 
creating a userfaultfd and unregistering VMAs.

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[Tal Zussman]

On Wed, Jun 4, 2025 at 11:10 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Jun 04, 2025 at 03:23:38PM +0200, David Hildenbrand wrote:
> > On 04.06.25 00:14, Tal Zussman wrote:
> > > Currently, a VMA registered with a uffd can be unregistered through a
> > > different uffd asssociated with the same mm_struct.
> > >
> > > Change this behavior to be stricter by requiring VMAs to be unregistered
> > > through the same uffd they were registered with.
> > >
> > > While at it, correct the comment for the no userfaultfd case. This seems
> > > to be a copy-paste artifact from the analagous userfaultfd_register()
> > > check.
> >
> > I consider it a BUG that should be fixed. Hoping Peter can share his
> > opinion.
>
> Agree it smells like unintentional, it's just that the man page indeed
> didn't mention what would happen if the userfaultfd isn't the one got
> registered but only requesting them to be "compatible".
>
> DESCRIPTION
>        Unregister a memory address range from userfaultfd.  The pages in
>        the range must be “compatible” (see UFFDIO_REGISTER(2const)).
>
> So it sounds still possible if we have existing userapp creating multiple
> userfaultfds (for example, for scalability reasons on using multiple
> queues) to manage its own mm address space, one uffd in charge of a portion
> of VMAs, then it can randomly take one userfaultfd to do unregistrations.
> Such might break.

As I mentioned in my response to James, it seems like the existing behavior
is broken as well, due to the following in in userfaultfd_unregister():

    if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
            goto out_unlock;

where wp_async is derived from ctx, not cur.

Pasting here:

This also seems to indicate that the current behavior is broken and may reject
unregistering some VMAs incorrectly. For example, a file-backed VMA registered
with `wp_async` and UFFD_WP cannot be unregistered through a VMA that does not
have `wp_async` set.

> >
> > >
> > > Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")
> > > Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> > > ---
> > >   fs/userfaultfd.c | 15 +++++++++++++--
> > >   1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > > index 22f4bf956ba1..9289e30b24c4 100644
> > > --- a/fs/userfaultfd.c
> > > +++ b/fs/userfaultfd.c
> > > @@ -1477,6 +1477,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
> > >   if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
> > >   goto out_unlock;
> > > + /*
> > > + * Check that this vma isn't already owned by a different
> > > + * userfaultfd. This provides for more strict behavior by
> > > + * preventing a VMA registered with a userfaultfd from being
> > > + * unregistered through a different userfaultfd.
> > > + */
> > > + if (cur->vm_userfaultfd_ctx.ctx &&
> > > +    cur->vm_userfaultfd_ctx.ctx != ctx)
> > > + goto out_unlock;
> >
> > So we allow !cur->vm_userfaultfd_ctx.ctx to allow unregistering when there
> > was nothing registered.
> >
> > A bit weird to set "found = true" in that case. Maybe it's fine, just
> > raising it ...
>
> This part should be ok, as found is defined as:
>
> /*
> * Search for not compatible vmas.
> */
> found = false;
>
> So it's still compatible VMA even if not registered.
>
> It's just that I'm not yet sure how this change benefits the kernel
> (besides the API can look slightly cleaner).  There seems to still have a
> low risk of breaking userapps.  It could be a matter of whether there can
> be any real security concerns.
>
> If not, maybe we don't need to risk such a change for almost nothing (I
> almost never think "API cleaness" a goal when it's put together with
> compatilibities).
>
> Thanks,
>
> --
> Peter Xu
>

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[Tal Zussman]

On Thu, Jun 5, 2025 at 5:06 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 04.06.25 17:09, Peter Xu wrote:
> > On Wed, Jun 04, 2025 at 03:23:38PM +0200, David Hildenbrand wrote:
> >> On 04.06.25 00:14, Tal Zussman wrote:
> >>> Currently, a VMA registered with a uffd can be unregistered through a
> >>> different uffd asssociated with the same mm_struct.
> >>>
> >>> Change this behavior to be stricter by requiring VMAs to be unregistered
> >>> through the same uffd they were registered with.
> >>>
> >>> While at it, correct the comment for the no userfaultfd case. This seems
> >>> to be a copy-paste artifact from the analagous userfaultfd_register()
> >>> check.
> >>
> >> I consider it a BUG that should be fixed. Hoping Peter can share his
> >> opinion.
> >
> > Agree it smells like unintentional, it's just that the man page indeed
> > didn't mention what would happen if the userfaultfd isn't the one got
> > registered but only requesting them to be "compatible".
> >
> > DESCRIPTION
> >         Unregister a memory address range from userfaultfd.  The pages in
> >         the range must be “compatible” (see UFFDIO_REGISTER(2const)).
> >
> > So it sounds still possible if we have existing userapp creating multiple
> > userfaultfds (for example, for scalability reasons on using multiple
> > queues) to manage its own mm address space, one uffd in charge of a portion
> > of VMAs, then it can randomly take one userfaultfd to do unregistrations.
> > Such might break.
>
> Not sure if relevant, but consider the following:
>
> an app being controlled by another process using userfaultfd.
>
> The app itself can "escape" uffd control of the other process by simply
> creating a userfaultfd and unregistering VMAs.

Yes, this is exactly what I was thinking. Or (less likely) a child process
that inherits a uffd from its parent can then mess with memory the parent
registers with a different uffd after the fork.

> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[Peter Xu]

On Thu, Jun 05, 2025 at 11:06:38PM +0200, David Hildenbrand wrote:
> Not sure if relevant, but consider the following:
> 
> an app being controlled by another process using userfaultfd.
> 
> The app itself can "escape" uffd control of the other process by simply
> creating a userfaultfd and unregistering VMAs.

IMHO it's okay if it's intentional by the child. E.g., even after this
patch, the child, if intentional, can also mmap() a new VMA on top of the
uffd tracked region to stop being trapped by the parent.  The parent might
still get a UNMAP event if registered, but it'll not be able to track the
new VMAs mapped.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[David Hildenbrand]

On 06.06.25 15:03, Peter Xu wrote:
> On Thu, Jun 05, 2025 at 11:06:38PM +0200, David Hildenbrand wrote:
>> Not sure if relevant, but consider the following:
>>
>> an app being controlled by another process using userfaultfd.
>>
>> The app itself can "escape" uffd control of the other process by simply
>> creating a userfaultfd and unregistering VMAs.
> 
> IMHO it's okay if it's intentional by the child. E.g., even after this
> patch, the child, if intentional, can also mmap() a new VMA on top of the
> uffd tracked region to stop being trapped by the parent.  The parent might
> still get a UNMAP event if registered, but it'll not be able to track the
> new VMAs mapped.

Ah, I thought there was a way yo track/intercept all new mappings as 
well, but looks like that is at least not the case through UFFD_EVENT_* 
as it seems.

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[Peter Xu]

On Thu, Jun 05, 2025 at 05:11:53PM -0400, Tal Zussman wrote:
> On Wed, Jun 4, 2025 at 11:10 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Wed, Jun 04, 2025 at 03:23:38PM +0200, David Hildenbrand wrote:
> > > On 04.06.25 00:14, Tal Zussman wrote:
> > > > Currently, a VMA registered with a uffd can be unregistered through a
> > > > different uffd asssociated with the same mm_struct.
> > > >
> > > > Change this behavior to be stricter by requiring VMAs to be unregistered
> > > > through the same uffd they were registered with.
> > > >
> > > > While at it, correct the comment for the no userfaultfd case. This seems
> > > > to be a copy-paste artifact from the analagous userfaultfd_register()
> > > > check.
> > >
> > > I consider it a BUG that should be fixed. Hoping Peter can share his
> > > opinion.
> >
> > Agree it smells like unintentional, it's just that the man page indeed
> > didn't mention what would happen if the userfaultfd isn't the one got
> > registered but only requesting them to be "compatible".
> >
> > DESCRIPTION
> >        Unregister a memory address range from userfaultfd.  The pages in
> >        the range must be “compatible” (see UFFDIO_REGISTER(2const)).
> >
> > So it sounds still possible if we have existing userapp creating multiple
> > userfaultfds (for example, for scalability reasons on using multiple
> > queues) to manage its own mm address space, one uffd in charge of a portion
> > of VMAs, then it can randomly take one userfaultfd to do unregistrations.
> > Such might break.
> 
> As I mentioned in my response to James, it seems like the existing behavior
> is broken as well, due to the following in in userfaultfd_unregister():
> 
>     if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
>             goto out_unlock;
> 
> where wp_async is derived from ctx, not cur.
> 
> Pasting here:
> 
> This also seems to indicate that the current behavior is broken and may reject
> unregistering some VMAs incorrectly. For example, a file-backed VMA registered
> with `wp_async` and UFFD_WP cannot be unregistered through a VMA that does not
> have `wp_async` set.

This is true.  Meanwhile it seems untrivial to fix the flag alone with the
prior per-vma loop to check compatibility.  We could drop the prior check
but then it slightly breaks the abi in another way..

Then let's go with the change to see our luck.

Could you mention more things when repost in the commit log?  (1) wp_async
bug, (2) explicitly mention that this is a slight ABI change, and (3) not
needed to backport to stable.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/3] userfaultfd: prevent unregistering VMAs through a different userfaultfd

[Tal Zussman]

On Fri, Jun 6, 2025 at 9:25 AM Peter Xu <peterx@redhat.com> wrote:
> On Thu, Jun 05, 2025 at 05:11:53PM -0400, Tal Zussman wrote:
> >
> > As I mentioned in my response to James, it seems like the existing behavior
> > is broken as well, due to the following in in userfaultfd_unregister():
> >
> >     if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
> >             goto out_unlock;
> >
> > where wp_async is derived from ctx, not cur.
> >
> > Pasting here:
> >
> > This also seems to indicate that the current behavior is broken and may reject
> > unregistering some VMAs incorrectly. For example, a file-backed VMA registered
> > with `wp_async` and UFFD_WP cannot be unregistered through a VMA that does not
> > have `wp_async` set.
>
> This is true.  Meanwhile it seems untrivial to fix the flag alone with the
> prior per-vma loop to check compatibility.  We could drop the prior check
> but then it slightly breaks the abi in another way..
>
> Then let's go with the change to see our luck.
>
> Could you mention more things when repost in the commit log?  (1) wp_async
> bug, (2) explicitly mention that this is a slight ABI change, and (3) not
> needed to backport to stable.

Will do!

> Thanks,
>
> --
> Peter Xu
>