Messages in this thread | | | Subject | Re: [PATCH v5 05/15] mm/frame-vector: Use FOLL_LONGTERM | From | John Hubbard <> | Date | Sun, 1 Nov 2020 13:13:07 -0800 |
| |
On 11/1/20 2:30 AM, Daniel Vetter wrote: > On Sun, Nov 1, 2020 at 6:22 AM John Hubbard <jhubbard@nvidia.com> wrote: >> >> On 10/31/20 7:45 AM, Daniel Vetter wrote: >>> On Sat, Oct 31, 2020 at 3:55 AM John Hubbard <jhubbard@nvidia.com> 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
| |