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

[David Hildenbrand <david@redhat.com>]

On 07.06.25 08:40, Tal Zussman wrote:
> Currently, a VMA registered with a uffd can be unregistered through a
> different uffd associated with the same mm_struct.
> 
> The existing behavior is slightly broken and may incorrectly reject
> unregistering some VMAs due to the following check:
> 
> 	if (!vma_can_userfault(cur, cur->vm_flags, wp_async))
> 		goto out_unlock;
> 
> where wp_async is derived from ctx, not from cur. For example, a file-backed
> VMA registered with wp_async enabled and UFFD_WP mode cannot be unregistered
> through a uffd that does not have wp_async enabled.
> 
> Rather than fix this and maintain this odd behavior, make unregistration
> stricter by requiring VMAs to be unregistered through the same uffd they
> were registered with. Additionally, reorder the WARN() checks to avoid
> the aforementioned wp_async issue in the WARN()s.
> 
> This change slightly modifies the ABI. It should not be backported to
> -stable.

Probably add that the expectation is that nobody really depends on this 
behavior, and that no such cases are known.

> 
> While at it, correct the comment for the no userfaultfd case. This seems to
> be a copy-paste artifact from the analogous userfaultfd_register() check.
> 
> Fixes: 86039bd3b4e6 ("userfaultfd: add new syscall to provide memory externalization")

Fixes should come before anything else in a series (Andrew even prefers 
a separate series for fixes vs. follow-up cleanups).

> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> ---
>   fs/userfaultfd.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 80c95c712266..10e8037f5216 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1466,6 +1466,16 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>   		VM_WARN_ON_ONCE(!!cur->vm_userfaultfd_ctx.ctx ^
>   				!!(cur->vm_flags & __VM_UFFD_FLAGS));
>   
> +		/*
> +		 * 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.
> +		 */

Probably we can shorted to:

/*
  * Prevent unregistering through another userfaultfd than used for
  * registering.
  */

?

> +		if (cur->vm_userfaultfd_ctx.ctx &&
> +		    cur->vm_userfaultfd_ctx.ctx != ctx)
> +			goto out_unlock;
> +
>   		/*
>   		 * Check not compatible vmas, not strictly required
>   		 * here as not compatible vmas cannot have an
> @@ -1489,15 +1499,14 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
>   	for_each_vma_range(vmi, vma, end) {
>   		cond_resched();
>   
> -		VM_WARN_ON_ONCE(!vma_can_userfault(vma, vma->vm_flags, wp_async));
> -
>   		/*
> -		 * 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.
>   		 */

Maybe

		/* VMA not registered with userfaultfd. */

The "skip" below is rather clear. :)

>   		if (!vma->vm_userfaultfd_ctx.ctx)
>   			goto skip;
>   
> +		VM_WARN_ON_ONCE(vma->vm_userfaultfd_ctx.ctx != ctx);
> +		VM_WARN_ON_ONCE(!vma_can_userfault(vma, vma->vm_flags, wp_async));
>   		WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
>   
>   		if (vma->vm_start > start)
> 

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

-- 
Cheers,

David / dhildenb