Re: [PATCH RFT 1/3] mm: export variants of vmf_insert_pfn* for use with pfn_mkwrite()

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

On 7/31/26 18:43, Paolo Bonzini wrote:
> Right now, users of .pfn_mkwrite() have no way to create a PTE
> that has gone through maybe_mkwrite().  Because vma_set_page_prot()
> will have cleared the writable PTE bit, users of fixup_user_fault()
> will see a read-only PTE and have no clue that the page needs
> a *second* fault to reach its final status.
> 
> Handling this in fixup_user_fault() is problematic: the information
> about the presence of *_mkwrite is only recorded in vma->vm_page_prot,
> which is an opaque pgprot_t, therefore only follow_pfnmap_start()
> knows how to retrieve it.
> 
> There are actually some preexisting functions that suggest how this
> is supposed to be handled, namely vmf_insert_page_mkwrite() and
> vmf_insert_pfn_pmd().  Adjust mm/memory.c to export two more
> functions: vmf_insert_pfn_mkwrite() for the common case where
> vma->vm_page_prot is okay, and __vmf_insert_pfn_prot() when
> really all parameters are needed.  This makes it possible
> to fix drivers that use .pfn_mkwrite together with
> vmf_insert_pfn() and vmf_insert_pfn_prot().
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/linux/mm.h |  4 +++
>  mm/huge_memory.c   |  2 +-
>  mm/memory.c        | 75 +++++++++++++++++++++++++++++++++-------------
>  3 files changed, 59 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 34c79b5fcb9b..33c7de36b214 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4551,6 +4551,10 @@ vm_fault_t vmf_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
>  			unsigned long pfn);
>  vm_fault_t vmf_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
>  			unsigned long pfn, pgprot_t pgprot);
> +vm_fault_t vmf_insert_pfn_mkwrite(struct vm_area_struct *vma, unsigned long addr,
> +			unsigned long pfn, bool write);
> +vm_fault_t __vmf_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
> +			unsigned long pfn, pgprot_t pgprot, bool mkwrite);
>  vm_fault_t vmf_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
>  			unsigned long pfn);
>  vm_fault_t vmf_insert_mixed_mkwrite(struct vm_area_struct *vma,
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index b5d1e9d4463d..2f4dcaa819b7 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1615,7 +1615,7 @@ static vm_fault_t insert_pmd(struct vm_area_struct *vma, unsigned long addr,
>   * @pfn: pfn to insert
>   * @write: whether it's a write fault
>   *
> - * Insert a pmd size pfn. See vmf_insert_pfn() for additional info.
> + * Insert a pmd size pfn. See vmf_insert_pfn_mkwrite() for additional info.
>   *
>   * Return: vm_fault_t value.
>   */
> diff --git a/mm/memory.c b/mm/memory.c
> index 40997a26846f..7b950be8f511 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2718,6 +2718,34 @@ static vm_fault_t insert_pfn(struct vm_area_struct *vma, unsigned long addr,
>  	return VM_FAULT_NOPAGE;
>  }
>  


[...]

> +vm_fault_t __vmf_insert_pfn_prot(struct vm_area_struct *vma,
> +			unsigned long addr, unsigned long pfn, pgprot_t pgprot,
> +			bool mkwrite)

(We indent two tabs, I assume vmf_insert_pfn_prot uses 3 for legacy reasons after
renamings)

Hm, having a __ function that looks like an internal helper exported to drivers
and then not adding kerneldocs.

Why not simply have

	vmf_insert_pfn_prot_mkwrite()

And add proper documentation?

I guess we could also turn vmf_insert_pfn(), vmf_insert_pfn_mkwrite() and
vmf_insert_pfn_prot() into simple inline functions in the header. And I'd even
say that a single excessive documentation of vmf_insert_pfn_prot_mkwrite()
might be sufficient, and keeping it very short for the wrappers.


> +{
> +	/*
> +	 * Technically, architectures with pte_special can avoid all these
> +	 * restrictions (same for remap_pfn_range).  However we would like
> +	 * consistency in testing and feature parity among all, so we should
> +	 * try to keep these invariants in place for everybody.
> +	 */
> +	BUG_ON(!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)));
> +	BUG_ON((vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) ==
> +						(VM_PFNMAP|VM_MIXEDMAP));
> +	BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
> +	BUG_ON((vma->vm_flags & VM_MIXEDMAP) && pfn_valid(pfn));
> +
> +	if (addr < vma->vm_start || addr >= vma->vm_end)
> +		return VM_FAULT_SIGBUS;
> +
> +	if (!pfn_modify_allowed(pfn, pgprot))
> +		return VM_FAULT_SIGBUS;
> +
> +	pfnmap_setup_cachemode_pfn(pfn, &pgprot);
> +
> +	return insert_pfn(vma, addr, pfn, pgprot, mkwrite);
> +}
> +EXPORT_SYMBOL(__vmf_insert_pfn_prot);

If this becomes a dedicated symbol, why not GPL?

-- 
Cheers,

David