Re: [PATCH 4/4] mm: Apply vm_uffd_ops API to core mm

[James Houghton]

On Wed, Jun 25, 2025 at 2:21 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> On Wed, Jun 25, 2025 at 01:31:49PM -0700, James Houghton wrote:
> > > -static inline bool vma_can_userfault(struct vm_area_struct *vma,
> > > -                                    unsigned long vm_flags,
> > > -                                    bool wp_async)
> > > +static inline const vm_uffd_ops *vma_get_uffd_ops(struct vm_area_struct *vma)
> > >  {
> > > -       vm_flags &= __VM_UFFD_FLAGS;
> > > -
> > > -       if (vma->vm_flags & VM_DROPPABLE)
> > > -               return false;
> > > -
> > > -       if ((vm_flags & VM_UFFD_MINOR) &&
> > > -           (!is_vm_hugetlb_page(vma) && !vma_is_shmem(vma)))
> > > -               return false;
> > > -
> > > -       /*
> > > -        * If wp async enabled, and WP is the only mode enabled, allow any
> > > -        * memory type.
> > > -        */
> > > -       if (wp_async && (vm_flags == VM_UFFD_WP))
> > > -               return true;
> > > -
> > > -#ifndef CONFIG_PTE_MARKER_UFFD_WP
> > > -       /*
> > > -        * If user requested uffd-wp but not enabled pte markers for
> > > -        * uffd-wp, then shmem & hugetlbfs are not supported but only
> > > -        * anonymous.
> > > -        */
> > > -       if ((vm_flags & VM_UFFD_WP) && !vma_is_anonymous(vma))
> > > -               return false;
> > > -#endif
> >
> > Hi Peter,
> >
> > Thanks for this cleanup!
> >
> > It looks like the above two checks, the wp-async one and the PTE
> > marker check, have been reordered in this patch. Does this result in a
> > functional difference?
> >
> > The rest of this series looks fine to me. :)
>
> Thanks for the very careful review, James!
>
> Yes that's a small tweak I did when moving. I don't expect to have any
> functional change.  Maybe I should at least mention that in the commit log.

Yeah if you could leave a small explanation, like what you have below,
in the commit log, that would be good. :)

>
> Here I did the movement because fundamentally wp_async depends on the pte
> markers, so it may be slightly more intuitive to check pte markers first,
> rejecting any form of file wr-protect traps.  Otherwise it may looks like
> we could return the true for wp_async==true too early.  In reality IIUC it
> can't happen.
>
> For example, currently userfaultfd_api() has:
>
> #ifndef CONFIG_PTE_MARKER_UFFD_WP
>         uffdio_api.features &= ~UFFD_FEATURE_WP_HUGETLBFS_SHMEM;
>         uffdio_api.features &= ~UFFD_FEATURE_WP_UNPOPULATED;
>         uffdio_api.features &= ~UFFD_FEATURE_WP_ASYNC;
> #endif

Ah, I see, thanks!

> So when wp_async can be true above, pte markers must be compiled..  IOW,
> above code clip should work identically with below lines:
>
>     #ifdef CONFIG_PTE_MARKER_UFFD_WP
>        if (wp_async && (vm_flags == VM_UFFD_WP))
>             return true;
>     #endif
>
>     #ifndef CONFIG_PTE_MARKER_UFFD_WP
>         if ((vm_flags & VM_UFFD_WP) && !vma_is_anonymous(vma))
>             return false;
>     #endif
>
> Then it means both chunks of code cannot be compiled together.  The order
> shouldn't matter.
>
> But maybe I should just move it back as before, to save the explain and
> confusions.  Let me know if you have any preference.

Feel free to leave this patch as it is, that's fine. Thanks for the
explanation. :)

If you'd like, feel free to add:

Reviewed-by: James Houghton <jthoughton@xxxxxxxxxx>