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

[Tal Zussman <tz2294@columbia.edu>]

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
>