Re: [PATCH] mm, gpu: fix error when FOLL_MLOCK an unpresent page

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Mon, Aug 30, 2021 at 06:55:02PM +0800, Yafang Shao wrote:
> On Mon, Aug 30, 2021 at 6:08 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Mon, Aug 30, 2021 at 05:12:32PM +0800, Yafang Shao wrote:
> > > > Which is not a valid way to call get_user_pages.  What we need to do is
> > > > to reject that case.
> > >
> > > Do you mean below change ?
> >
> > Sory of.  I think once touching this we should do a few more cleanups
> > including making many of the flags private to gup.c.  I'll try to find
> > some time to post a more complete series.
> 
> JFYI, below test case can also hit the bug I reported above.

How does the bug manifests with the test case? I don't see any crash with
it in my setup.

Or do you mean you can hit __get_user_pages() with FOLL_MLOCK, but without
FOLL_POPULATE?

My guess is that you have wrong expectation from GUP: it will return a
number of pages it advanced in the mapping, not number of present pages
there. For your case it means that the array of pages can have gaps and
it's okay.

Fill the array with zeros before calling GUP and check if the entry is
non-NULL before dereferencing it.

> 
> #define _GNU_SOURCE
> #include <stdio.h>
> #include <sys/mman.h>
> 
> #define LEN 4096
> 
> int main()
> {
>         char *addr;
>         int ret;
> 
>         addr = mmap(NULL, LEN, PROT_READ|PROT_WRITE, MAP_PRIVATE |
> MAP_ANON , -1, 0);
>         if (addr == MAP_FAILED) {
>                 perror("mmap");
>                 return ret;
>         }
> 
>         /*
>          * MLOCK_ONFAULT  will hit below if condition.
>          *  if ((*flags & (FOLL_POPULATE | FOLL_MLOCK)) == FOLL_MLOCK)
>          *       return -ENOENT;
>          */
>         ret = mlock2(addr, LEN, MLOCK_ONFAULT);
> //      ret = mlock2(addr, LEN, 0);
>         if (ret < 0) {
>                 perror("mlock2");
>                 return ret;
>         }
> 
>         return 0;
> }
> 
> -- 
> Thanks
> Yafang
> 

-- 
 Kirill A. Shutemov