[PATCH] mm/gup: tolerate NULL unlocked in fixup_user_fault()

[PATCH] mm/gup: tolerate NULL unlocked in fixup_user_fault()

[Stepan Ionichev]

fixup_user_fault() takes a "bool *unlocked" output parameter that
callers may set to NULL when they do not want the retry/unlock
machinery. The function honours that contract on the way in:

	if (unlocked)
		fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;

so callers passing NULL never set FAULT_FLAG_ALLOW_RETRY. In return,
handle_mm_fault() is not expected to produce VM_FAULT_RETRY or
VM_FAULT_COMPLETED for them, which is why the dereferences of
unlocked further down used to be considered unreachable.

That invariant is implicit, not enforced. At least one caller in
arch/s390/pci/pci_mmio.c does pass NULL:

	fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, NULL);

If a future change in handle_mm_fault() ever returned
VM_FAULT_COMPLETED or VM_FAULT_RETRY without ALLOW_RETRY having been
requested, the unconditional "*unlocked = true" stores would
NULL-deref and crash the kernel for this path.

smatch flags both stores:

  mm/gup.c:1597 fixup_user_fault() error: we previously assumed
    'unlocked' could be null (see line 1573)
  mm/gup.c:1612 fixup_user_fault() error: we previously assumed
    'unlocked' could be null (see line 1573)

Make the NULL handling consistent on both sides of the function:
guard the two stores with "if (unlocked)" so fixup_user_fault()
tolerates a NULL output pointer regardless of which fault outcome
handle_mm_fault() returns.

No functional change for callers that already pass a non-NULL
pointer.

Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
 mm/gup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index ad9ded396..1a8d7c7c8 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1594,7 +1594,8 @@ int fixup_user_fault(struct mm_struct *mm,
 		 * could tell the callers so they do not need to unlock.
 		 */
 		mmap_read_lock(mm);
-		*unlocked = true;
+		if (unlocked)
+			*unlocked = true;
 		return 0;
 	}
 
@@ -1608,7 +1609,8 @@ int fixup_user_fault(struct mm_struct *mm,
 
 	if (ret & VM_FAULT_RETRY) {
 		mmap_read_lock(mm);
-		*unlocked = true;
+		if (unlocked)
+			*unlocked = true;
 		fault_flags |= FAULT_FLAG_TRIED;
 		goto retry;
 	}
-- 
2.43.0

Re: [PATCH] mm/gup: tolerate NULL unlocked in fixup_user_fault()

[David Hildenbrand (Arm)]

On 5/7/26 10:30, Stepan Ionichev wrote:
> fixup_user_fault() takes a "bool *unlocked" output parameter that
> callers may set to NULL when they do not want the retry/unlock
> machinery. The function honours that contract on the way in:
> 
> 	if (unlocked)
> 		fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
> 
> so callers passing NULL never set FAULT_FLAG_ALLOW_RETRY. In return,
> handle_mm_fault() is not expected to produce VM_FAULT_RETRY or
> VM_FAULT_COMPLETED for them, which is why the dereferences of
> unlocked further down used to be considered unreachable.
> 
> That invariant is implicit, not enforced. At least one caller in
> arch/s390/pci/pci_mmio.c does pass NULL:
> 
> 	fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, NULL);
> 
> If a future change in handle_mm_fault() ever returned
> VM_FAULT_COMPLETED or VM_FAULT_RETRY without ALLOW_RETRY having been
> requested, the unconditional "*unlocked = true" stores would
> NULL-deref and crash the kernel for this path.

That would be completely broken. We must not drop the mmap lock unless
FAULT_FLAG_ALLOW_RETRY was set. Returning VM_FAULT_COMPLETED/VM_FAULT_RETRY
would mean that we did that. Broken.

And the function documents "If NULL, the caller must guarantee that fault_flags
does not contain FAULT_FLAG_ALLOW_RETRY."

-- 
Cheers,

David

Re: [PATCH] mm/gup: tolerate NULL unlocked in fixup_user_fault()

[Stepan Ionichev]

On Fri, 08 May 2026, David Hildenbrand wrote:
> That would be completely broken. We must not drop the mmap lock unless
> FAULT_FLAG_ALLOW_RETRY was set. Returning VM_FAULT_COMPLETED/VM_FAULT_RETRY
> would mean that we did that. Broken.
>
> And the function documents "If NULL, the caller must guarantee that fault_flags
> does not contain FAULT_FLAG_ALLOW_RETRY."

You're right -- the contract is already enforced by the surrounding
code and documented at the function. Defensive guards against an
impossible scenario would just hide future real bugs.

Please drop this patch.

Thanks for taking the time to explain.

Stepan