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