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

[Peter Xu <peterx@redhat.com>]

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