Re: [PATCH] arch/s390/pci: fix fixup_user_fault() calls with NULL unlocked parameter

[Gerd Bayer]

On Wed, 2026-08-26 at 14:42 +0800, Liu Dalin wrote:
> The s390 PCI MMIO functions call fixup_user_fault() with the 'unlocked'
> parameter set to NULL. This means when handle_mm_fault() returns
> VM_FAULT_COMPLETED or VM_FAULT_RETRY, the function re-acquires the
> mmap_lock but cannot notify the caller about this state change.

Hi Liu,

I'm far from being a mm-expert, but from reading the function
description of fixup_user_fault() in mm/gup.c I believe that mmap_lock
is unlocked *only* if retries are allowed by way of setting the
FAULT_FLAG_ALLOW_RETRY. Otherwise, the unlocked parameter is allowed to
be NULL.

Since s390_pci_mmio_write()/_read() calls fixup_user_fault() without
the FAULT_FLAG_ALLOW_RETRY flag, I thought your patch is not needed.

But then I got concerned about the comment in the handling of the
VM_FAULT_COMPLETED return from handle_mm_fault() - which got introduced
with commit d92725256b4f ("mm: avoid unnecessary page fault retires on
shared memory types") without changing the documentation of the
unlocked parameter of fixup_user_fault().

I'm under the impression, that so far we saw
s390_pci_mmio_write()/_read() only in use on VMAs that ended in EFAULT?
But that's a question to @Matt and/or @Niklas...

> This is problematic because the caller subsequently calls
> mmap_read_unlock() at the end of the function. If the lock was
> re-acquired inside fixup_user_fault(), the unlock happens correctly.
> But passing NULL makes the code fragile and hard to reason about.

For a full solution, this would have to make the call to
mmap_read_lock() dependent on unlocked being true at the end of both
sys-calls.

> 
> Fix this by providing a proper 'unlocked' variable to fixup_user_fault()
> so the lock state is properly tracked.
> 
> This is a follow-up to the defensive NULL check added in fixup_user_fault()
> by the previous patch in this series.
> 
> Fixes: 41a0926e82f4 ("s390/pci: Fix s390_mmio_read/write syscall page fault handling")
> Signed-off-by: Liu Dalin <liudalin@xxxxxxxxxxxxxxx>
> ---
>  arch/s390/pci/pci_mmio.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
> index f3f79ba78410..5ef2d6436d4b 100644
> --- a/arch/s390/pci/pci_mmio.c
> +++ b/arch/s390/pci/pci_mmio.c
> @@ -182,7 +182,9 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr,
>  	args.vma = vma;
>  	ret = follow_pfnmap_start(&args);
>  	if (ret) {
> -		fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, NULL);
> +		bool unlocked = false;
> +
> +		fixup_user_fault(current->mm, mmio_addr, FAULT_FLAG_WRITE, &unlocked);
>  		ret = follow_pfnmap_start(&args);
>  		if (ret)
>  			goto out_unlock_mmap;
> @@ -335,7 +337,9 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr,
>  	args.address = mmio_addr;
>  	ret = follow_pfnmap_start(&args);
>  	if (ret) {
> -		fixup_user_fault(current->mm, mmio_addr, 0, NULL);
> +		bool unlocked = false;
> +
> +		fixup_user_fault(current->mm, mmio_addr, 0, &unlocked);
>  		ret = follow_pfnmap_start(&args);
>  		if (ret)
>  			goto out_unlock_mmap;

Thank you,
Gerd