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

[David Hildenbrand <david@redhat.com>]

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