Re: [PATCH] mm: pull writability check to follow_pfnmap_start()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 7/31/26 18:05, Paolo Bonzini wrote:
> All callers of follow_pfnmap_start() except s390_pci_mmio_write() are
> following it, if they are doing a write, with a check that args.writable
> is true; for s390_pci_mmio_write() that's a bug.  Most of them return
> -EFAULT if it is not, but because follow_pfnmap_start() returned success,
> they do not attempt to upgrade the page from read-only to read-write
> with fixup_user_fault().
> 
> Pull the check directly into follow_pfnmap_start() through another
> input parameter args.write_fault; this eliminates the need to do it in
> the caller and, for callers that do use fixup_user_fault(), lets it
> attempt to upgrade the page.
> 
> The change in return code to -EFAULT is okay:
> 
> - s390 is the only one to have a functional change in this respect;
>   but EINVAL is only documented for "Invalid length argument", while
>   "The address in mmio_addr is invalid" should already return EFAULT
>   (https://www.man7.org/linux/man-pages/man2/s390_pci_mmio_read.2.html)
> 
> - for VFIO and ACRN, -EFAULT was returned already
> 
> - for KVM, -EFAULT is eaten
> 
> - for generic_access_phys() the caller is __access_remote_vm()
>   which does not care about the error code.
> 
> Reported-by: Sergio Lopez <slp@redhat.com>

Reported-by: without Fixes: is odd.

> Link: https://lore.kernel.org/kvm/CAAiTLFU1ALsDoJoKW3d9bUvv990AozAoX=bEHmfnG54qyBAHFg@mail.gmail.com/
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

[...]

> index 485df9c2dbdd..34c79b5fcb9b 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3136,9 +3136,12 @@ struct follow_pfnmap_args {
>  	 * Inputs:
>  	 * @vma: Pointer to @vm_area_struct struct
>  	 * @address: the virtual address to walk
> +	 * @write_fault: if true, fail with -EFAULT unless the mapping is

Just wondering whether EPERM would be better.

> +	 * writable
>  	 */
>  	struct vm_area_struct *vma;
>  	unsigned long address;
> +	bool write_fault;

"write_fault" is a rather odd name for this, given that this function will not
trigger a write fault.

You want something that matches FOLL_WRITE.

"write_access" / "check_writable" maybe?

-- 
Cheers,

David