On Sun, Nov 1, 2020 at 10:13 PM John Hubbard <jhubb...@nvidia.com> wrote: > > On 11/1/20 2:30 AM, Daniel Vetter wrote: > > On Sun, Nov 1, 2020 at 6:22 AM John Hubbard <jhubb...@nvidia.com> wrote: > >> > >> On 10/31/20 7:45 AM, Daniel Vetter wrote: > >>> On Sat, Oct 31, 2020 at 3:55 AM John Hubbard <jhubb...@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?
Yeah sorry, I got distracted reading code and noticed we might have another issue. > 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. Yup that's the one I've found. > > 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? It's not device drivers, but everyone else. At least my understanding is that VM_IO | VM_PFNMAP means "even if it happens to be backed by a struct page, do not treat it like normal memory". And gup/pup_fast happily break that. I tried to chase the history of that test, didn't turn up anything I understood much: commit 1ff8038988adecfde71d82c0597727fc239d4e8c Author: Linus Torvalds <torva...@g5.osdl.org> Date: Mon Dec 12 16:24:33 2005 -0800 get_user_pages: don't try to follow PFNMAP pages Nick Piggin points out that a few drivers play games with VM_IO (why? who knows..) and thus a pfn-remapped area may not have that bit set even if remap_pfn_range() set it originally. So make it explicit in get_user_pages() that we don't follow VM_PFNMAP pages, since pretty much by definition they do not have a "struct page" associated with them. Signed-off-by: Linus Torvalds <torva...@osdl.org> diff --git a/mm/memory.c b/mm/memory.c index 47c533eaa072..d22f78c8a381 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -1009,7 +1009,7 @@ int get_user_pages(struct task_struct *tsk, struct mm_struct *mm, continue; } - if (!vma || (vma->vm_flags & VM_IO) + if (!vma || (vma->vm_flags & (VM_IO | VM_PFNMAP)) || !(vm_flags & vma->vm_flags)) return i ? : -EFAULT; The VM_IO check is kinda lost in pre-history. tbh I have no idea what the various variants of pup/gup are supposed to be doing vs. these VMA flags in the various cases. Just smells a bit like potential trouble due to randomly pinning stuff without the owner of that memory having an idea what's going on. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel