On Wed, Jun 12, 2019 at 09:54:58PM +0800, Pingfan Liu wrote: > On Tue, Jun 11, 2019 at 04:29:11PM +0000, Weiny, Ira wrote: > > > Pingfan Liu <kernelf...@gmail.com> writes: > > > > > > > As for FOLL_LONGTERM, it is checked in the slow path > > > > __gup_longterm_unlocked(). But it is not checked in the fast path, > > > > which means a possible leak of CMA page to longterm pinned requirement > > > > through this crack. > > > > > > Shouldn't we disallow FOLL_LONGTERM with get_user_pages fastpath? W.r.t > > > dax check we need vma to ensure whether a long term pin is allowed or not. > > > If FOLL_LONGTERM is specified we should fallback to slow path. > > > > Yes, the fastpath bails to the slowpath if FOLL_LONGTERM _and_ DAX. But it > > does this while walking the page tables. I missed the CMA case and > > Pingfan's patch fixes this. We could check for CMA pages while walking the > > page tables but most agreed that it was not worth it. For DAX we already > > had checks for *_devmap() so it was easier to put the FOLL_LONGTERM checks > > there. > > > Then for CMA pages, are you suggesting something like:
I'm not suggesting this. Sorry I wrote this prior to seeing the numbers in your other email. Given the numbers it looks like performing the check whilst walking the tables is worth the extra complexity. I was just trying to summarize the thread. I don't think we should disallow FOLL_LONGTERM because it only affects CMA and DAX. Other pages will be fine with FOLL_LONGTERM. Why penalize every call if we don't have to. Also in the case of DAX the use of vma will be going away...[1] Eventually... ;-) Ira [1] https://lkml.org/lkml/2019/6/5/1049 > diff --git a/mm/gup.c b/mm/gup.c > index 42a47c0..8bf3cc3 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2251,6 +2251,8 @@ int get_user_pages_fast(unsigned long start, int > nr_pages, > if (unlikely(!access_ok((void __user *)start, len))) > return -EFAULT; > > + if (unlikely(gup_flags & FOLL_LONGTERM)) > + goto slow; > if (gup_fast_permitted(start, nr_pages)) { > local_irq_disable(); > gup_pgd_range(addr, end, gup_flags, pages, &nr); > @@ -2258,6 +2260,7 @@ int get_user_pages_fast(unsigned long start, int > nr_pages, > ret = nr; > } > > +slow: > if (nr < nr_pages) { > /* Try to get the remaining pages with get_user_pages */ > start += nr << PAGE_SHIFT; > > Thanks, > Pingfan
