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

[Yafang Shao <laoar.shao@gmail.com>]

On Tue, Aug 31, 2021 at 9:36 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> 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?
>

Right, that is what I meant.

> 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.
>

Right, I misunderstood __get_user_pages().  Thanks for the explanation.

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

Sure.

> >
> > #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



-- 
Thanks
Yafang