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

[Tal Zussman <tz2294@columbia.edu>]

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
>