Re: [PATCH v2 1/6] mm: export vmf_insert_pfn_prot_mkwrite(), change variants to inline

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

On 8/4/26 14:05, 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().  Fixing the drivers requires similar variants
> of vm_insert_pfn(), namely vmf_insert_pfn_mkwrite() for the common
> case where vma->vm_page_prot is okay, and vmf_insert_pfn_prot_mkwrite()
> 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().
> 
> Since vmf_insert_pfn_prot_mkwrite() is the most general variant
> and all the others are just special cases, turn them into inline
> functions in the header.
> 
> Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in mmap")
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/linux/mm.h | 81 +++++++++++++++++++++++++++++++++++++++++---
>  mm/huge_memory.c   |  2 +-
>  mm/memory.c        | 84 ++++++++++++++++++++--------------------------
>  3 files changed, 114 insertions(+), 53 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 485df9c2dbdd..01184a4bdd6f 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4544,16 +4544,89 @@ int vm_map_pages_zero(struct vm_area_struct *vma, struct page **pages,
>  				unsigned long num);
>  vm_fault_t vmf_insert_page_mkwrite(struct vm_fault *vmf, struct page *page,
>  			bool write);
> -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_prot_mkwrite(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,
>  		unsigned long addr, unsigned long pfn);
>  int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long len);
>  

To not inflate mm.h too much, can we just try removing all details that can also
be had in vmf_insert_pfn_prot_mkwrite() doc, and refer to that?

> +
> +/**
> + * vmf_insert_pfn_prot - insert single pfn into user vma with specified pgprot
> + * @vma: user vma to map to
> + * @addr: target user address of this page
> + * @pfn: source kernel pfn
> + * @pgprot: pgprot flags for the inserted page
> + *
> + * This is exactly like vmf_insert_pfn(), except that it allows drivers
> + * to override pgprot on a per-page basis.  For more information,
> + * see vmf_insert_pfn_prot_mkwrite().

For example, I would keep this statement here for all 3 variants.

> + *
> + * This only makes sense for IO mappings, and it makes no sense for
> + * COW mappings.  In general, using multiple vmas is preferable;
> + * vmf_insert_pfn_prot should only be used if using multiple VMAs is
> + * impractical.

Can we just move that for vmf_insert_pfn_prot_mkwrite() and document it when
pgprot != vma->vm_page_prot ?

> + *
> + * Context: Process context.  May allocate using %GFP_KERNEL.
> + * Return: vm_fault_t value.
> + */
> +static inline vm_fault_t vmf_insert_pfn_prot(struct vm_area_struct *vma,
> +			unsigned long addr, unsigned long pfn, pgprot_t pgprot)
> +{
> +	return vmf_insert_pfn_prot_mkwrite(vma, addr, pfn, pgprot, false);
> +}
> +
> +/**
> + * vmf_insert_pfn_mkwrite - insert single pfn into user vma, possibly writable
> + * @vma: user vma to map to
> + * @addr: target user address of this page
> + * @pfn: source kernel pfn
> + * @write: whether the PTE should be installed writable
> + *
> + * Like vmf_insert_pfn(), except that @write allows installing a writable
> + * PTE even when @vma is under write notification.  For more information,
> + * see vmf_insert_pfn_prot_mkwrite().
> + *
> + * Note that neither .pfn_mkwrite() nor .page_mkwrite() is invoked, so the
> + * caller must itself do whatever they would have done if @write is true.

Similarly move that to vmf_insert_pfn_prot_mkwrite().

> + *
> + * Context: Process context.  May allocate using %GFP_KERNEL.
> + * Return: vm_fault_t value.
> + */
> +static inline vm_fault_t vmf_insert_pfn_mkwrite(struct vm_area_struct *vma,
> +			unsigned long addr, unsigned long pfn, bool write)
> +{
> +	return vmf_insert_pfn_prot_mkwrite(vma, addr, pfn, vma->vm_page_prot, write);
> +}
> +
> +/**
> + * vmf_insert_pfn - insert single pfn into user vma
> + * @vma: user vma to map to
> + * @addr: target user address of this page
> + * @pfn: source kernel pfn
> + *
> + * Similar to vm_insert_page, this allows drivers to insert individual pages
> + * they've allocated into a user vma. Same comments apply.

I know that you are moving this doc, but some things stick out:

Wouldn't it be better to also refer to vmf_insert_pfn() instead, like all the
other variants?

> + *
> + * This function should only be called from a vm_ops->fault handler, and
> + * in that case the handler should return the result of this function.

Isn't this the same for the other ones as well?

> + *
> + * vma cannot be a COW mapping.

Isn't this the same for all of them?

> + *
> + * As this is called only for pages that do not currently exist, we
> + * do not need to flush old virtual caches or the TLB.

Isn't this an implementation detail?

> + *
> + * Context: Process context.  May allocate using %GFP_KERNEL.
> + * Return: vm_fault_t value.
> + */
> +static inline vm_fault_t vmf_insert_pfn(struct vm_area_struct *vma,
> +			unsigned long addr, unsigned long pfn)
> +{
> +	return vmf_insert_pfn_mkwrite(vma, addr, pfn, false);
> +}
> +

Apart from that LGTM.

-- 
Cheers,

David