On Tue, Jun 10, 2025 at 3:31 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
Thanks! Will update with the suggested comment + commit message changes and move
this patch before the VM_WARN changes.
> --
> Cheers,
>
> David / dhildenb
>
