diff mbox series

mm/gup: Fixes FOLL_UNLOCKABLE against FOLL_NOWAIT

Message ID 20230512003102.3149737-1-peterx@redhat.com
State New
Headers
Series mm/gup: Fixes FOLL_UNLOCKABLE against FOLL_NOWAIT |

Commit Message

Peter Xu May 12, 2023, 12:31 a.m. UTC 
  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(-)

Comments

Jason Gunthorpe May 12, 2023, 5:06 p.m. UTC | #1 
On Thu, May 11, 2023 at 08:31:02PM -0400, Peter Xu wrote:

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

I would say NOWAIT and UNLOCKABLE are different things. UNLOCKABLE
says the mmap sem is allowed to be unlocked, which is true, and NOWAIT
says it shouldn't "wait" (which is something more nebulous than just
sleep). In FOLL_ flag terms it would be fine if the mmap sem was
unlocked while doing NOWAIT - even though the fault hanlder will not
doe this.

The only caller is fine with this too.

!UNLOCKABLE literally means not to ever drop the mmap lock which is
not something KVM needs at all.

So I'd say it is fine as is. A caller should never assume that calling
an unlocked function or passing null locked means that the mmap sem
won't be unlocked while running indirectly because of other GUP
flags. If it wants this behavior it needs to ask for it explicitly
with a locked GUP call and a NULL locked.

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

Not quite, it is correct because that is the API contract of this
function. The caller must provide a non-NULL locked and non-NULL
locked at the external interfaces always mean it can be unlocked while
running.

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

Then we have bigger problems because the API has become confusing if a
non-NULL locked somehow means 'don't ever unlock', but only sometimes.

Jason
Peter Xu May 15, 2023, 3:48 p.m. UTC | #2 
On Fri, May 12, 2023 at 02:06:36PM -0300, Jason Gunthorpe wrote:
> On Thu, May 11, 2023 at 08:31:02PM -0400, Peter Xu wrote:
> 
> > 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.
> 
> I would say NOWAIT and UNLOCKABLE are different things. UNLOCKABLE
> says the mmap sem is allowed to be unlocked, which is true, and NOWAIT
> says it shouldn't "wait" (which is something more nebulous than just
> sleep). In FOLL_ flag terms it would be fine if the mmap sem was
> unlocked while doing NOWAIT - even though the fault hanlder will not
> doe this.
> 
> The only caller is fine with this too.
> 
> !UNLOCKABLE literally means not to ever drop the mmap lock which is
> not something KVM needs at all.

The problem is FOLL_NOWAIT implies FAULT_FLAG_RETRY_NOWAIT internally.

Then we'll have FAULT_FLAG_RETRY_NOWAIT+FAULT_FLAG_KILLABLE which makes it
very confusing, because RETRY_NOWAIT means we never release mmap lock or
retry, then KILL means "if we wait, allow us to be killed".

Considering FOLL_UNLOCKABLE is an internal flag while FOLL_NOWAIT a public
(even if only with a single caller...), I'd still think it makes more sense
and cleaner to just remove FOLL_UNLOCKABLE if FOLL_NOWAIT, no?

Again, nothing to blame for previous commit (I explained in the commit
message too that we don't need fixes, but simply a cleanup), but it seems
removing this confusion of NOWAIT+UNLOCKABLE could be helpful to me.

> 
> So I'd say it is fine as is. A caller should never assume that calling
> an unlocked function or passing null locked means that the mmap sem
> won't be unlocked while running indirectly because of other GUP
> flags. If it wants this behavior it needs to ask for it explicitly
> with a locked GUP call and a NULL locked.
> 
> > 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.  
> 
> Not quite, it is correct because that is the API contract of this
> function. The caller must provide a non-NULL locked and non-NULL
> locked at the external interfaces always mean it can be unlocked while
> running.

Hmm yes, that's the contract.  But then it makes more sense to assert on
that contract (by checking locked)?

How about I rework the commit message but keep the change (which literally
only add the assertion)?
Jason Gunthorpe May 15, 2023, 5 p.m. UTC | #3 
On Mon, May 15, 2023 at 11:48:17AM -0400, Peter Xu wrote:

> The problem is FOLL_NOWAIT implies FAULT_FLAG_RETRY_NOWAIT internally.
> 
> Then we'll have FAULT_FLAG_RETRY_NOWAIT+FAULT_FLAG_KILLABLE which makes it
> very confusing, because RETRY_NOWAIT means we never release mmap lock or
> retry, then KILL means "if we wait, allow us to be killed".

I don't know if it is so confusing, the flags still make sense when
composed together even if one is a NOP.

> Considering FOLL_UNLOCKABLE is an internal flag while FOLL_NOWAIT a public
> (even if only with a single caller...), I'd still think it makes more sense
> and cleaner to just remove FOLL_UNLOCKABLE if FOLL_NOWAIT, no?

I don't really like it..

The FOLL_ flags are supposed to be statements about what the caller is
expecting. In this context the caller is clearly perfectly happy with
unlocking the mmap sem during operation. It should set the flag.

That the underlying code can't possibly do that when FOLL_NOWAIT is
set too doesn't really matter to the caller.

If there is something that needs tidying I'd say it is adjusting the
FAULT_FLAG logic to not make the combination, but I don't think it is
actually that confusing. "don't sleep" & "allow kill if you do sleep"
are still logically combinable operations.

Jason
diff mbox series

Patch

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,