Re: [PATCH v3 08/23] vfio, mm: fix get_user_pages_remote() and FOLL_LONGTERM

[Dan Williams]

On Tue, Nov 12, 2019 at 2:24 PM John Hubbard <jhubbard@xxxxxxxxxx> wrote:
>
> On 11/12/19 1:57 PM, Dan Williams wrote:
> ...
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index d864277ea16f..017689b7c32b 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -348,24 +348,20 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
> >>                 flags |= FOLL_WRITE;
> >>
> >>         down_read(&mm->mmap_sem);
> >> -       if (mm == current->mm) {
> >> -               ret = get_user_pages(vaddr, 1, flags | FOLL_LONGTERM, page,
> >> -                                    vmas);
> >> -       } else {
> >> -               ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags, page,
> >> -                                           vmas, NULL);
> >> -               /*
> >> -                * The lifetime of a vaddr_get_pfn() page pin is
> >> -                * userspace-controlled. In the fs-dax case this could
> >> -                * lead to indefinite stalls in filesystem operations.
> >> -                * Disallow attempts to pin fs-dax pages via this
> >> -                * interface.
> >> -                */
> >> -               if (ret > 0 && vma_is_fsdax(vmas[0])) {
> >> -                       ret = -EOPNOTSUPP;
> >> -                       put_page(page[0]);
> >> -               }
> >> +       ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,
> >> +                                   page, vmas, NULL);
> >
> > Hmm, what's the point of passing FOLL_LONGTERM to
> > get_user_pages_remote() if get_user_pages_remote() is not going to
> > check the vma? I think we got to this code state because the
>
> FOLL_LONGTERM is short-lived in this location, because patch 23
> ("mm/gup: remove support for gup(FOLL_LONGTERM)") removes it, after
> callers are changed over to pin_longterm_pages*().
>
> So FOLL_LONGTERM is not doing much now, but it is basically a marker for
> "change gup(FOLL_LONGTERM) to pin_longterm_pages()", and patch 18
> actually makes that change.
>
> And then pin_longterm_pages*() is, in turn, a way to mark all the
> places that need file system and/or user space interactions (layout
> leases, etc), as per "Case 2: RDMA" in the new
> Documentation/vm/pin_user_pages.rst.

Ah, sorry. This was the first time I had looked at this series and
jumped in without reading the background.

Your patch as is looks ok, I assume you've removed the FOLL_LONGTERM
warning in get_user_pages_remote in another patch?

>
> > get_user_pages() vs get_user_pages_remote() split predated the
> > introduction of FOLL_LONGTERM.
>
> Yes. And I do want clean this up as I go, so we don't end up with
> stale concepts lingering in gup.c...
>
> >
> > I think check_vma_flags() should do the ((FOLL_LONGTERM | FOLL_GET) &&
> > vma_is_fsdax()) check and that would also remove the need for
> > __gup_longterm_locked.
> >
>
> Good idea, but there is still the call to check_and_migrate_cma_pages(),
> inside __gup_longterm_locked().  So it's a little more involved and
> we can't trivially delete __gup_longterm_locked() yet, right?

[ add Aneesh ]

Yes, you're right. I had overlooked that had snuck in there. That to
me similarly needs to be pushed down into the core with its own FOLL
flag, or it needs to be an explicit fixup that each caller does after
get_user_pages. The fact that migration silently happens as a side
effect of gup is too magical for my taste.