mm: clean up populate_vma_page_range() FOLL_* flag handling

The code wasn't exactly wrong, but it was very odd, and it used
FOLL_FORCE together with FOLL_WRITE when it really didn't need to (it
only set FOLL_WRITE for writable mappings, so then the FOLL_FORCE was
pointless).

It also pointlessly called __get_user_pages() even when it knew it
wouldn't populate anything because the vma wasn't accessible and it
explicitly tested for and did *not* set FOLL_FORCE for inaccessible
vma's.

This code does need to use FOLL_FORCE, because we want to do fault in
writable shared mappings, but then the mapping may not actually be
readable.  And we don't want to use FOLL_WRITE (which would match the
permission of the vma), because that would also dirty the pages, which
we don't want to do.

For very similar reasons, FOLL_FORCE populates a executable-only mapping
with no read permissions. We don't have a FOLL_EXEC flag.

Yes, it would probably be cleaner to split FOLL_WRITE into two bits (for
separate permission and dirty bit handling), and add a FOLL_EXEC flag
for the "GUP executable page" case.  That would allow us to avoid
FOLL_FORCE entirely here.

But that's not how our FOLL_xyz bits have traditionally worked, and that
would be a much bigger patch.

So this at least avoids the FOLL_FORCE | FOLL_WRITE combination that
made one of my experimental validation patches trigger a warning.  That
warning was a false positive (and my experimental patch was incomplete
anyway), but it all made me look at this and decide to clean at least
this small case up.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/mm/gup.c b/mm/gup.c
index df83182ec72d5d77bb86b6571814a23fe244945a..af8edadc05d1b87200050bd34ff3d58ec52abd52 100644 (file)
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1653,20 +1653,22 @@ long populate_vma_page_range(struct vm_area_struct *vma,
        if (vma->vm_flags & VM_LOCKONFAULT)
                return nr_pages;
 
+       /* ... similarly, we've never faulted in PROT_NONE pages */
+       if (!vma_is_accessible(vma))
+               return -EFAULT;
+
        gup_flags = FOLL_TOUCH;
        /*
         * We want to touch writable mappings with a write fault in order
         * to break COW, except for shared mappings because these don't COW
         * and we would not want to dirty them for nothing.
+        *
+        * Otherwise, do a read fault, and use FOLL_FORCE in case it's not
+        * readable (ie write-only or executable).
         */
        if ((vma->vm_flags & (VM_WRITE | VM_SHARED)) == VM_WRITE)
                gup_flags |= FOLL_WRITE;
-
-       /*
-        * We want mlock to succeed for regions that have any permissions
-        * other than PROT_NONE.
-        */
-       if (vma_is_accessible(vma))
+       else
                gup_flags |= FOLL_FORCE;
 
        if (locked)