Re: [PATCH v5 05/15] mm/frame-vector: Use FOLL_LONGTERM

[John Hubbard]

On 11/1/20 2:30 AM, Daniel Vetter wrote:
> On Sun, Nov 1, 2020 at 6:22 AM John Hubbard <jhubbard@xxxxxxxxxx> wrote:
> > On 10/31/20 7:45 AM, Daniel Vetter wrote:
> > > On Sat, Oct 31, 2020 at 3:55 AM John Hubbard <jhubbard@xxxxxxxxxx> wrote:
> > > > On 10/30/20 3:08 AM, Daniel Vetter wrote:
> > ...
> > > > By removing this check from this location, and changing from
> > > > pin_user_pages_locked() to pin_user_pages_fast(), I *think* we end up
> > > > losing the check entirely. Is that intended? If so it could use a comment
> > > > somewhere to explain why.
> > >
> > > Yeah this wasn't intentional. I think I needed to drop the _locked
> > > version to prep for FOLL_LONGTERM, and figured _fast is always better.
> > > But I didn't realize that _fast doesn't have the vma checks, gup.c got
> > > me a bit confused.
> >
> > Actually, I thought that the change to _fast was a very nice touch, btw.
> >
> > > I'll remedy this in all the patches where this applies (because a
> > > VM_IO | VM_PFNMAP can point at struct page backed memory, and that
> > > exact use-case is what we want to stop with the unsafe_follow_pfn work
> > > since it wreaks things like cma or security).
> > >
> > > Aside: I do wonder whether the lack for that check isn't a problem.
> > > VM_IO | VM_PFNMAP generally means driver managed, which means the
> > > driver isn't going to consult the page pin count or anything like that
> > > (at least not necessarily) when revoking or moving that memory, since
> > > we're assuming it's totally under driver control. So if pup_fast can
> > > get into such a mapping, we might have a problem.
> > > -Daniel
> >
> > Yes. I don't know why that check is missing from the _fast path.
> > Probably just an oversight, seeing as how it's in the slow path. Maybe
> > the appropriate response here is to add a separate patch that adds the
> > check.
> >
> > I wonder if I'm overlooking something, but it certainly seems correct to
> > do that.
>
> You'll need the mmap_sem to get at the vma to be able to do this
> check. If you add that to _fast, you made it as fast as the slow one.

Arggh, yes of course. Strike that, please. :)

> Plus there's _fast_only due to locking recurion issues in fast-paths
> (I assume, I didn't check all the callers).
>
> I'm just wondering whether we have a bug somewhere with device
> drivers. For CMA regions we always check in try_grab_page, but for dax

OK, so here you're talking about a different bug than the VM_IO | VM_PFNMAP
pages, I think. This is about the "FOLL_LONGTERM + CMA + gup/pup _fast"
combination that is not allowed, right?

For that: try_grab_page() doesn't check anything, but try_grab_compound_head()
does, but only for pup_fast, not gup_fast. That was added by commit
df3a0a21b698d ("mm/gup: fix omission of check on FOLL_LONGTERM in gup fast
path") in April.

I recall that the patch was just plugging a very specific hole, as opposed
to locking down the API against mistakes or confused callers. And it does
seem that there are some holes.

> I'm not seeing where the checks in the _fast fastpaths are, and that
> all still leaves random device driver mappings behind which aren't
> backed by CMA but still point to something with a struct page behind
> it. I'm probably just missing something, but no idea what.
> -Daniel

Certainly we've established that we can't check VMA flags by that time,
so I'm not sure that there is much we can check by the time we get to
gup/pup _fast. Seems like the device drivers have to avoid calling _fast
with pages that live in VM_IO | VM_PFNMAP, by design, right? Or maybe
you're talking about CMA checks only?


thanks,
--
John Hubbard
NVIDIA