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

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

On Mon, Aug 30, 2021 at 3:00 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sun, Aug 29, 2021 at 01:19:53AM +0000, Yafang Shao wrote:
> > After some analyzation, I found it was caused by a bug in GUP.
> > When the kernel module calls get_user_pages() with FOLL_MLOCK being set but
> > FOLL_POPULATE being unset, if the page of the user addr isn't present, the
>
> 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 ?

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7ca22e6e694a..10f7d6f2ad7b 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2855,7 +2855,7 @@ struct page *follow_page(struct vm_area_struct
*vma, unsigned long address,
 #define FOLL_NUMA      0x200   /* force NUMA hinting page fault */
 #define FOLL_MIGRATION 0x400   /* wait for page to replace migration entry */
 #define FOLL_TRIED     0x800   /* a retry, previous pass started an IO */
-#define FOLL_MLOCK     0x1000  /* lock present pages */
+#define FOLL_MLOCK     0x1000  /* lock present pages, must be set
with FOLL_POPULATE */
 #define FOLL_REMOTE    0x2000  /* we are working on non-current tsk/mm */
 #define FOLL_COW       0x4000  /* internal GUP flag */
 #define FOLL_ANON      0x8000  /* don't do file mappings */
diff --git a/mm/gup.c b/mm/gup.c
index b94717977d17..dfdc0654f7a5 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -929,9 +929,9 @@ static int faultin_page(struct vm_area_struct *vma,
        unsigned int fault_flags = 0;
        vm_fault_t ret;

-       /* mlock all present pages, but do not fault in new pages */
-       if ((*flags & (FOLL_POPULATE | FOLL_MLOCK)) == FOLL_MLOCK)
-               return -ENOENT;
+       /* FOLL_MLOCK must be set with FOLL_POPULATE */
+       BUG_ON((*flags & (FOLL_POPULATE | FOLL_MLOCK)) == FOLL_MLOCK);
+
        if (*flags & FOLL_WRITE)
                fault_flags |= FAULT_FLAG_WRITE;
        if (*flags & FOLL_REMOTE)
@@ -1181,8 +1181,6 @@ static long __get_user_pages(struct mm_struct *mm,
                        case -ENOMEM:
                        case -EHWPOISON:
                                goto out;
-                       case -ENOENT:
-                               goto next_page;
                        }
                        BUG();
                } else if (PTR_ERR(page) == -EEXIST) {
@@ -1823,6 +1821,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,

 static bool is_valid_gup_flags(unsigned int gup_flags)
 {
+       /* FOLL_MLOCK must be set with FOLL_POPULATE */
+       if (WARN_ON_ONCE((gup_flags & (FOLL_POPULATE | FOLL_MLOCK)) ==
FOLL_MLOCK))
+               return false;
+
        /*
         * FOLL_PIN must only be set internally by the pin_user_pages*() APIs,
         * never directly by the caller, so enforce that with an assertion:


>  No-tree user does this so that bug is what ever
> crap out of tree code you're using.

populate_vma_page_range() may trigger this bug, but I haven't verified it yet.

populate_vma_page_range
    gup_flags = FOLL_TOUCH | FOLL_POPULATE | FOLL_MLOCK;
    if (vma->vm_flags & VM_LOCKONFAULT)
        gup_flags &= ~FOLL_POPULATE;  // FOLL_MLOCK without FOLL_POPULATE then.

    __get_user_pages(..., gup_flags, ...);


-- 
Thanks
Yafang