Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Peter Xu <peterx@redhat.com>
To:     linux-mm@kvack.org, linux-kernel@vger.kernel.org
Cc:     John Hubbard <jhubbard@nvidia.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Lorenzo Stoakes <lstoakes@gmail.com>, peterx@redhat.com,
        David Hildenbrand <david@redhat.com>
Subject: [PATCH] mm/gup: Fixes FOLL_UNLOCKABLE against FOLL_NOWAIT
Date:   Thu, 11 May 2023 20:31:02 -0400
Message-Id: <20230512003102.3149737-1-peterx@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

This is a follow up on f04740f54594 ("mm/gup: add FOLL_UNLOCKABLE").

FOLL_NOWAIT is the gup alias of FAULT_FLAG_RETRY_NOWAIT, which means when
FOLL_NOWAIT is set we definitely don't want to release the mmap read lock
when faulting.  It's against the meaning of the newly introduced flag
FOLL_UNLOCKABLE.

E.g., with current code we could at last have FAULT_FLAG_RETRY_NOWAIT set
even if with a FOLL_UNLOCKABLE gup which doesn't make a lot of sense.

Code-wise, it _seems_ all still fine, because when NOWAIT+UNLOCKABLE both
set it'll be the same as old NOWAIT plus FAULT_FLAG_KILLABLE (since luckily
both of them leverage ALLOW_RETRY OTOH), which I don't see a major issue so
far.  So not copying stable or attaching fixes, as there's no immediate
issue found.  Still better clarify the use.

Since at it, the same commit added unconditional FOLL_UNLOCKABLE in
faultin_vma_page_range(), which is code-wise correct becuase the helper
only has one user right now and it always has "locked" set.  However it can
be abused if someone reuse faultin_vma_page_range() in other call sites in
the future.  Add a sanity check for that, also add the missing comment for
UNLOCKABLE.

Cc: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---

This is something I found when I was reading the code alongside only.  I
hope I didn't miss something.
---
 mm/gup.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 90d9b65ff35c..202097627667 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1621,6 +1621,9 @@ long faultin_vma_page_range(struct vm_area_struct *vma, unsigned long start,
 	VM_BUG_ON_VMA(end > vma->vm_end, vma);
 	mmap_assert_locked(mm);
 
+	/* We'll do unconditional FOLL_UNLOCKABLE */
+	VM_WARN_ON_ONCE(!locked);
+
 	/*
 	 * FOLL_TOUCH: Mark page accessed and thereby young; will also mark
 	 *	       the page dirty with FOLL_WRITE -- which doesn't make a
@@ -1629,6 +1632,7 @@ long faultin_vma_page_range(struct vm_area_struct *vma, unsigned long start,
 	 * FOLL_HWPOISON: Return -EHWPOISON instead of -EFAULT when we hit
 	 *		  a poisoned page.
 	 * !FOLL_FORCE: Require proper access permissions.
+	 * FOLL_UNLOCKABLE: Allow the fault to unlock mmap read lock
 	 */
 	gup_flags = FOLL_TOUCH | FOLL_HWPOISON | FOLL_UNLOCKABLE;
 	if (write)
@@ -2334,10 +2338,13 @@ EXPORT_SYMBOL(get_user_pages);
 long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
 			     struct page **pages, unsigned int gup_flags)
 {
+	unsigned int extra = FOLL_TOUCH;
 	int locked = 0;
 
-	if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags,
-			       FOLL_TOUCH | FOLL_UNLOCKABLE))
+	if (!(gup_flags & FOLL_NOWAIT))
+		extra |= FOLL_UNLOCKABLE;
+
+	if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags, extra))
 		return -EINVAL;
 
 	return __get_user_pages_locked(current->mm, start, nr_pages, pages,
-- 
2.39.1