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 <[email protected]>
> ---
> 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
