[PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

[PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

[Paolo Bonzini]

Warning - DRM parts (i.e. most of the patches) untested; I have Cc'd
the reporter to help with testing these patches.

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().  So, this series adjusts mm/memory.c to export
two new functions vmf_insert_pfn_mkwrite() and __vmf_insert_pfn_prot(),
and then teaches drm's two users of .pfn_mkwrite() to call them.  Let
me know if I should use another name like vmf_insert_pfn_prot_mkwrite(),
instead of the "__"-prefixed one.

The drm_gem_shmem_helper case was reported as a KVM regression, while
the vmwgfx one was found by inspection of .pfn_mkwrite() implementors.

Thanks,

Paolo

Paolo Bonzini (3):
  mm: export variants of vmf_insert_pfn* for use with pfn_mkwrite()
  drm/shmem_helper: use vmf_insert_pfn_mkwrite()
  drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is
    in use

 drivers/gpu/drm/drm_gem_shmem_helper.c     | 38 +++++------
 drivers/gpu/drm/ttm/ttm_bo_vm.c            |  3 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 44 +++++++------
 include/linux/mm.h                         |  4 ++
 mm/huge_memory.c                           |  2 +-
 mm/memory.c                                | 75 ++++++++++++++++------
 6 files changed, 107 insertions(+), 59 deletions(-)

-- 
2.55.0

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

[Paolo Bonzini]

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)
+{
+	/*
+	 * 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);
+
 /**
  * vmf_insert_pfn_prot - insert single pfn into user vma with specified pgprot
  * @vma: user vma to map to
@@ -2754,27 +2782,7 @@ static vm_fault_t insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 vm_fault_t vmf_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
 			unsigned long pfn, pgprot_t pgprot)
 {
-	/*
-	 * 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, false);
+	return __vmf_insert_pfn_prot(vma, addr, pfn, pgprot, false);
 }
 EXPORT_SYMBOL(vmf_insert_pfn_prot);
 
@@ -2805,6 +2813,31 @@ vm_fault_t vmf_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 }
 EXPORT_SYMBOL(vmf_insert_pfn);
 
+/**
+ * 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, i.e. when it has a
+ * .page_mkwrite() or .pfn_mkwrite() callback and vma_set_page_prot() has
+ * therefore cleared the write bit from @vma->vm_page_prot.
+ *
+ * Note that neither of these callbacks is invoked, so the caller must
+ * itself do whatever they would have done if @write is true.
+ *
+ * Context: Process context.  May allocate using %GFP_KERNEL.
+ * Return: vm_fault_t value.
+ */
+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(vma, addr, pfn, vma->vm_page_prot, write);
+}
+EXPORT_SYMBOL(vmf_insert_pfn_mkwrite);
+
 static bool vm_mixed_ok(struct vm_area_struct *vma, unsigned long pfn,
 			bool mkwrite)
 {
-- 
2.55.0

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

[David Hildenbrand (Arm)]

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

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

[Christoph Hellwig]

On Mon, Aug 03, 2026 at 02:16:33PM +0200, David Hildenbrand (Arm) wrote:
> > +EXPORT_SYMBOL(__vmf_insert_pfn_prot);
> 
> If this becomes a dedicated symbol, why not GPL?

Yes, no way we'd not export low-level bits like this as non-GPL..

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

[Paolo Bonzini]

On 8/4/26 15:52, Christoph Hellwig wrote:
> On Mon, Aug 03, 2026 at 02:16:33PM +0200, David Hildenbrand (Arm) wrote:
>>> +EXPORT_SYMBOL(__vmf_insert_pfn_prot);
>>
>> If this becomes a dedicated symbol, why not GPL?
> 
> Yes, no way we'd not export low-level bits like this as non-GPL..

David requested to turn the simpler functions such as vmf_insert_pfn() 
from separate exports to static inlines.  For the v2 that I have just 
posted, that's what I did.  I can either use EXPORT_SYMBOL_GPL() or 
switch to inlines, but not both because functions like vmf_insert_pfn() 
are currently EXPORT_SYMBOL().

Also, this function specifically is basically the same as 
vmf_insert_pfn_prot(), which is already exported as non GPL, but there's 
really no logic at all as to what is EXPORT_SYMBOL() and what is 
EXPORT_SYMBOL_GPL().  vmf_insert_pfn_prot() mucks with pgprot_t and is 
much lower level than vmf_insert_page_mkwrite()... but it's the latter 
that is GPL.

Paolo

[PATCH RFT 2/3] drm/shmem_helper: use vmf_insert_pfn_mkwrite()

[Paolo Bonzini]

This ensures that KVM or VFIO correctly see a writable PTE when
they request one.  Otherwise, a guest write to an unpopulated
PTE from a mapping backed by a DRM GEM BO triggers a VM exit
with EFAULT.

The code actually is simpler, because the same logic already
applied to the hugepage mapping case using vmf_insert_pfn_pmd().

Reported-by: Sergio Lopez <slp@redhat.com>
Link: https://lore.kernel.org/kvm/20260729072044.25796-1-slp@redhat.com/
Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in mmap")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 38 ++++++++++++++------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index c989459eb215..33a14f558276 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -589,11 +589,25 @@ static void drm_gem_shmem_record_mkwrite(struct vm_fault *vmf)
 	folio_mark_dirty(page_folio(shmem->pages[page_offset]));
 }
 
+/*
+ * Because the vm_ops have a .pfn_mkwrite() callback, vma_set_page_prot()
+ * has cleared the write bit from vma->vm_page_prot.  vmf_insert_pfn()
+ * would install a read-only entry even for a write fault, relying on a
+ * second fault to reach .pfn_mkwrite() and upgrade it, but that second
+ * fault never happens for fixup_user_fault() callers that directly
+ * walk the page tables with follow_pfnmap_start().  To ensure that
+ * they don't see the read-only entry, pass FAULT_FLAG_WRITE info down
+ * to install a writable entry right away.  Because .pfn_mkwrite() is
+ * not invoked, record the write afterwards.
+ */
 static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
 				 unsigned long pfn)
 {
+	bool write = vmf->flags & FAULT_FLAG_WRITE;
+	vm_fault_t ret = VM_FAULT_FALLBACK;
+
 	if (!order) {
-		return vmf_insert_pfn(vmf->vma, vmf->address, pfn);
+		ret = vmf_insert_pfn_mkwrite(vmf->vma, vmf->address, pfn, write);
 #ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
 	} else if (order == PMD_ORDER) {
 		unsigned long paddr = pfn << PAGE_SHIFT;
@@ -601,27 +615,15 @@ static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
 
 		if (aligned &&
 		    folio_test_pmd_mappable(page_folio(pfn_to_page(pfn)))) {
-			vm_fault_t ret;
-
 			pfn &= PMD_MASK >> PAGE_SHIFT;
-
-			/* Unlike PTEs which are automatically upgraded to
-			 * writeable entries, the PMD upgrades go through
-			 * .huge_fault(). Make sure we pass the "write" info
-			 * along in that case.
-			 * This also means we have to record the write fault
-			 * here, instead of in .pfn_mkwrite().
-			 */
-			ret = vmf_insert_pfn_pmd(vmf, pfn,
-						 vmf->flags & FAULT_FLAG_WRITE);
-			if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
-				drm_gem_shmem_record_mkwrite(vmf);
-
-			return ret;
+			ret = vmf_insert_pfn_pmd(vmf, pfn, write);
 		}
 #endif
 	}
-	return VM_FAULT_FALLBACK;
+
+	if (ret == VM_FAULT_NOPAGE && write)
+		drm_gem_shmem_record_mkwrite(vmf);
+	return ret;
 }
 
 static vm_fault_t drm_gem_shmem_any_fault(struct vm_fault *vmf, unsigned int order)
-- 
2.55.0

Re: [PATCH RFT 2/3] drm/shmem_helper: use vmf_insert_pfn_mkwrite()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect page offset calculation causes wrong pages to be mapped after VMA split (munmap/mremap)
--

--- Patch [2]: [PATCH RFT 2/3] drm/shmem_helper: use vmf_insert_pfn_mkwrite() ---
commit e238b92cc06e6221554d7e59a6c25c7b8c19b7ac
Author: Paolo Bonzini <pbonzini@redhat.com>

drm/shmem_helper: use vmf_insert_pfn_mkwrite()

This commit changes the shmem helper to use vmf_insert_pfn_mkwrite() to
ensure KVM or VFIO correctly see a writable PTE when requested. It also
updates the PMD mapping case to pass the write flag instead of recording
the write fault early.

Link: https://lore.kernel.org/kvm/20260729072044.25796-1-slp@redhat.com/

> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index c989459eb2159..c81be3e973173 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -589,11 +589,25 @@ static void drm_gem_shmem_record_mkwrite(struct vm_fault *vmf)
>  	folio_mark_dirty(page_folio(shmem->pages[page_offset]));
>  }

[Severity: High]
This is a pre-existing issue, but does this result in the wrong page being
marked dirty or mapped after a VMA split?

Looking at drm_gem_shmem_record_mkwrite() (line 583) and
drm_gem_shmem_any_fault() (line 638) in drivers/gpu/drm/drm_gem_shmem_helper.c,
page_offset is calculated as:

    pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff;

If userspace maps a GEM object and then splits the VMA (e.g., via a partial
munmap() at the beginning of the VMA), the kernel advances vma->vm_pgoff to
account for the split. When a page fault subsequently occurs in the new VMA,
vmf->pgoff evaluates to the fake offset plus the split offset plus the fault
offset.

This makes the calculation mathematically equivalent to
(address - vma->vm_start) >> PAGE_SHIFT. This completely discards the split
offset, meaning the remaining VMA will erroneously map the GEM object starting
from page 0 again, rather than the correct sub-page.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731164341.1109827-1-pbonzini@redhat.com?part=2

Re: [PATCH RFT 2/3] drm/shmem_helper: use vmf_insert_pfn_mkwrite()

[Boris Brezillon]

On Fri, 31 Jul 2026 18:43:40 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This ensures that KVM or VFIO correctly see a writable PTE when
> they request one.  Otherwise, a guest write to an unpopulated
> PTE from a mapping backed by a DRM GEM BO triggers a VM exit
> with EFAULT.
> 
> The code actually is simpler, because the same logic already
> applied to the hugepage mapping case using vmf_insert_pfn_pmd().
> 
> Reported-by: Sergio Lopez <slp@redhat.com>
> Link: https://lore.kernel.org/kvm/20260729072044.25796-1-slp@redhat.com/
> Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in mmap")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Glad some MM expert looked at this, because we had this partial "get
rid of pfn_mkwrite()" workaround pending [1], which apparently never
made it to drm-misc-fixes for some reason.

[1]https://lore.kernel.org/dri-devel/20260528114744.463484-1-tzimmermann@suse.de/

> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 38 ++++++++++++++------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index c989459eb215..33a14f558276 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -589,11 +589,25 @@ static void drm_gem_shmem_record_mkwrite(struct vm_fault *vmf)
>  	folio_mark_dirty(page_folio(shmem->pages[page_offset]));
>  }
>  
> +/*
> + * Because the vm_ops have a .pfn_mkwrite() callback, vma_set_page_prot()
> + * has cleared the write bit from vma->vm_page_prot.  vmf_insert_pfn()
> + * would install a read-only entry even for a write fault, relying on a
> + * second fault to reach .pfn_mkwrite() and upgrade it, but that second
> + * fault never happens for fixup_user_fault() callers that directly
> + * walk the page tables with follow_pfnmap_start().  To ensure that
> + * they don't see the read-only entry, pass FAULT_FLAG_WRITE info down
> + * to install a writable entry right away.  Because .pfn_mkwrite() is
> + * not invoked, record the write afterwards.
> + */
>  static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
>  				 unsigned long pfn)
>  {
> +	bool write = vmf->flags & FAULT_FLAG_WRITE;
> +	vm_fault_t ret = VM_FAULT_FALLBACK;
> +
>  	if (!order) {
> -		return vmf_insert_pfn(vmf->vma, vmf->address, pfn);
> +		ret = vmf_insert_pfn_mkwrite(vmf->vma, vmf->address, pfn, write);
>  #ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
>  	} else if (order == PMD_ORDER) {
>  		unsigned long paddr = pfn << PAGE_SHIFT;
> @@ -601,27 +615,15 @@ static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
>  
>  		if (aligned &&
>  		    folio_test_pmd_mappable(page_folio(pfn_to_page(pfn)))) {
> -			vm_fault_t ret;
> -
>  			pfn &= PMD_MASK >> PAGE_SHIFT;
> -
> -			/* Unlike PTEs which are automatically upgraded to
> -			 * writeable entries, the PMD upgrades go through
> -			 * .huge_fault(). Make sure we pass the "write" info
> -			 * along in that case.
> -			 * This also means we have to record the write fault
> -			 * here, instead of in .pfn_mkwrite().
> -			 */
> -			ret = vmf_insert_pfn_pmd(vmf, pfn,
> -						 vmf->flags & FAULT_FLAG_WRITE);
> -			if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
> -				drm_gem_shmem_record_mkwrite(vmf);
> -
> -			return ret;
> +			ret = vmf_insert_pfn_pmd(vmf, pfn, write);
>  		}
>  #endif
>  	}
> -	return VM_FAULT_FALLBACK;
> +
> +	if (ret == VM_FAULT_NOPAGE && write)
> +		drm_gem_shmem_record_mkwrite(vmf);
> +	return ret;
>  }
>  
>  static vm_fault_t drm_gem_shmem_any_fault(struct vm_fault *vmf, unsigned int order)

[PATCH RFT 3/3] drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is in use

[Paolo Bonzini]

This ensures that fixup_user_fault() users see a writable PTE when
they request one.  The flip side is that vmw_bo_vm_fault() now has
to record by hand the write fault, because .pfn_mkwrite() is
not invoked.

Prefaulting works as before because only the first entry comes
out writable, while the following ones still end up executing
the .pfn_mkwrite() callback.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/gpu/drm/ttm/ttm_bo_vm.c            |  3 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 42 ++++++++++++----------
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index a80510489c45..ef27a2d7afc0 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -263,7 +263,8 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
 		 * at arbitrary times while the data is mmap'ed.
 		 * See vmf_insert_pfn_prot() for a discussion.
 		 */
-		ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
+		ret = __vmf_insert_pfn_prot(vma, address, pfn, prot,
+					    i == 0 && !!(vmf->flags & FAULT_FLAG_WRITE));
 
 		/* Never error on prefaulted PTEs */
 		if (unlikely((ret & VM_FAULT_ERROR))) {
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
index 45561bc1c9ef..2cc490e7d758 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
@@ -398,15 +398,33 @@ void vmw_bo_dirty_clear_res(struct vmw_resource *res)
 		dirty->end = res_start;
 }
 
+static vm_fault_t vmw_bo_dirty_mkwrite(struct vm_fault *vmf, struct ttm_buffer_object *bo)
+{
+	unsigned long page_offset;
+	struct vmw_bo *vbo = to_vmw_bo(&bo->base);
+
+	page_offset = vmf->pgoff - drm_vma_node_start(&bo->base.vma_node);
+	if (unlikely(page_offset >= PFN_UP(bo->resource->size)))
+		return VM_FAULT_SIGBUS;
+
+	if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE &&
+	    !test_bit(page_offset, &vbo->dirty->bitmap[0])) {
+		struct vmw_bo_dirty *dirty = vbo->dirty;
+
+		__set_bit(page_offset, &dirty->bitmap[0]);
+		dirty->start = min(dirty->start, page_offset);
+		dirty->end = max(dirty->end, page_offset + 1);
+	}
+	return 0;
+}
+
 vm_fault_t vmw_bo_vm_mkwrite(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	struct ttm_buffer_object *bo = (struct ttm_buffer_object *)
 	    vma->vm_private_data;
 	vm_fault_t ret;
-	unsigned long page_offset;
 	unsigned int save_flags;
-	struct vmw_bo *vbo = to_vmw_bo(&bo->base);
 
 	/*
 	 * mkwrite() doesn't handle the VM_FAULT_RETRY return value correctly.
@@ -419,22 +437,7 @@ vm_fault_t vmw_bo_vm_mkwrite(struct vm_fault *vmf)
 	if (ret)
 		return ret;
 
-	page_offset = vmf->pgoff - drm_vma_node_start(&bo->base.vma_node);
-	if (unlikely(page_offset >= PFN_UP(bo->resource->size))) {
-		ret = VM_FAULT_SIGBUS;
-		goto out_unlock;
-	}
-
-	if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE &&
-	    !test_bit(page_offset, &vbo->dirty->bitmap[0])) {
-		struct vmw_bo_dirty *dirty = vbo->dirty;
-
-		__set_bit(page_offset, &dirty->bitmap[0]);
-		dirty->start = min(dirty->start, page_offset);
-		dirty->end = max(dirty->end, page_offset + 1);
-	}
-
-out_unlock:
+	ret = vmw_bo_dirty_mkwrite(vmf, bo);
 	dma_resv_unlock(bo->base.resv);
 	return ret;
 }
@@ -484,6 +487,9 @@ vm_fault_t vmw_bo_vm_fault(struct vm_fault *vmf)
 		prot = vm_get_page_prot(vma->vm_flags);
 
 	ret = ttm_bo_vm_fault_reserved(vmf, prot, num_prefault);
+	if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
+		WARN_ON_ONCE(vmw_bo_dirty_mkwrite(vmf, bo));
+
 	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
 		return ret;
 
-- 
2.55.0

Re: [PATCH RFT 3/3] drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is in use

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The dirty tracking structure `vbo->dirty` can be freed concurrently by `vmw_bo_dirty_release()` without holding the buffer object's reservation lock, causing a use-after-free in `vmw_bo_dirty_mkwrite()`.
- [High] Inverted error handling in ttm_bo_vm_fault_reserved masks primary fault errors (causing infinite page fault loops) and erroneously surfaces speculative prefault errors.
--

--- Patch [3]: [PATCH RFT 3/3] drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is in use ---
commit 894d8cfbec765789074f38bcd9f120fdcf49cba5
Author: Paolo Bonzini <pbonzini@redhat.com>

drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is in use

This commit changes the fault handler to directly insert writable PTEs for the
first page when write access is requested. It achieves this by manually
recording the write fault instead of relying on the .pfn_mkwrite() callback.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index a80510489c456..ef27a2d7afc07 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -263,7 +263,8 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
>  		 * at arbitrary times while the data is mmap'ed.
>  		 * See vmf_insert_pfn_prot() for a discussion.
>  		 */
> -		ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
> +		ret = __vmf_insert_pfn_prot(vma, address, pfn, prot,
> +					    i == 0 && !!(vmf->flags & FAULT_FLAG_WRITE));
>  
>  		/* Never error on prefaulted PTEs */
>  		if (unlikely((ret & VM_FAULT_ERROR))) {
>  			if (i == 0)
>  				return VM_FAULT_NOPAGE;
>  			else
>  				break;
>  		}

[Severity: High]
This is a pre-existing issue, but could this error handling in
ttm_bo_vm_fault_reserved() lead to infinite page fault loops or unwarranted
SIGBUS crashes?

If __vmf_insert_pfn_prot() returns an error like VM_FAULT_SIGBUS or
VM_FAULT_OOM, and it is the primary fault (i == 0), this block returns
VM_FAULT_NOPAGE (success). Does this cause the kernel to infinitely retry
the faulting instruction? 

Also, if it is a speculative prefault (i > 0), the loop breaks and returns
the prefault error, crashing the application even though the primary fault
succeeded. Is this the intended behavior?

[ ... ]

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> index 45561bc1c9eff..3099558c0762f 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> @@ -398,15 +398,33 @@ void vmw_bo_dirty_clear_res(struct vmw_resource *res)
>  		dirty->end = res_start;
>  }
>  
> +static vm_fault_t vmw_bo_dirty_mkwrite(struct vm_fault *vmf, struct ttm_buffer_object *bo)
> +{
> +	unsigned long page_offset;
> +	struct vmw_bo *vbo = to_vmw_bo(&bo->base);
> +
> +	page_offset = vmf->pgoff - drm_vma_node_start(&bo->base.vma_node);
> +	if (unlikely(page_offset >= PFN_UP(bo->resource->size)))
> +		return VM_FAULT_SIGBUS;
> +
> +	if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE &&
> +	    !test_bit(page_offset, &vbo->dirty->bitmap[0])) {
> +		struct vmw_bo_dirty *dirty = vbo->dirty;
> +
> +		__set_bit(page_offset, &dirty->bitmap[0]);
> +		dirty->start = min(dirty->start, page_offset);
> +		dirty->end = max(dirty->end, page_offset + 1);
> +	}

[Severity: Critical]
This isn't a bug introduced by this patch, but is it possible for vbo->dirty
to be freed concurrently without holding the buffer object's reservation lock?

If user space destroys the framebuffer, vmw_bo_dirty_release() is called,
which modifies and frees vbo->dirty:

void vmw_bo_dirty_release(struct vmw_bo *vbo)
{
	struct vmw_bo_dirty *dirty = vbo->dirty;

	if (dirty && kref_put(&dirty->ref_count, vmw_bo_dirty_free))
		vbo->dirty = NULL;
}

Since vmw_bo_dirty_release() doesn't appear to take the resv lock, can a
concurrent write to a mapped buffer object (which triggers the page fault
handler and calls vmw_bo_dirty_mkwrite() under the resv lock) dereference
the freed vbo->dirty memory here, leading to a use-after-free?

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731164341.1109827-1-pbonzini@redhat.com?part=3

Re: [PATCH RFT 3/3] drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is in use

[Christian König]

On 7/31/26 18:43, Paolo Bonzini wrote:
> This ensures that fixup_user_fault() users see a writable PTE when
> they request one.  The flip side is that vmw_bo_vm_fault() now has
> to record by hand the write fault, because .pfn_mkwrite() is
> not invoked.
> 
> Prefaulting works as before because only the first entry comes
> out writable, while the following ones still end up executing
> the .pfn_mkwrite() callback.

Please split that patch for TTM/VMWGFX. The TTM part looks reasonable, but VMGFX is a completely different beast.

Regards,
Christian.

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo_vm.c            |  3 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 42 ++++++++++++----------
>  2 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index a80510489c45..ef27a2d7afc0 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -263,7 +263,8 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
>  		 * at arbitrary times while the data is mmap'ed.
>  		 * See vmf_insert_pfn_prot() for a discussion.
>  		 */
> -		ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
> +		ret = __vmf_insert_pfn_prot(vma, address, pfn, prot,
> +					    i == 0 && !!(vmf->flags & FAULT_FLAG_WRITE));
>  
>  		/* Never error on prefaulted PTEs */
>  		if (unlikely((ret & VM_FAULT_ERROR))) {
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> index 45561bc1c9ef..2cc490e7d758 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> @@ -398,15 +398,33 @@ void vmw_bo_dirty_clear_res(struct vmw_resource *res)
>  		dirty->end = res_start;
>  }
>  
> +static vm_fault_t vmw_bo_dirty_mkwrite(struct vm_fault *vmf, struct ttm_buffer_object *bo)
> +{
> +	unsigned long page_offset;
> +	struct vmw_bo *vbo = to_vmw_bo(&bo->base);
> +
> +	page_offset = vmf->pgoff - drm_vma_node_start(&bo->base.vma_node);
> +	if (unlikely(page_offset >= PFN_UP(bo->resource->size)))
> +		return VM_FAULT_SIGBUS;
> +
> +	if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE &&
> +	    !test_bit(page_offset, &vbo->dirty->bitmap[0])) {
> +		struct vmw_bo_dirty *dirty = vbo->dirty;
> +
> +		__set_bit(page_offset, &dirty->bitmap[0]);
> +		dirty->start = min(dirty->start, page_offset);
> +		dirty->end = max(dirty->end, page_offset + 1);
> +	}
> +	return 0;
> +}
> +
>  vm_fault_t vmw_bo_vm_mkwrite(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  	struct ttm_buffer_object *bo = (struct ttm_buffer_object *)
>  	    vma->vm_private_data;
>  	vm_fault_t ret;
> -	unsigned long page_offset;
>  	unsigned int save_flags;
> -	struct vmw_bo *vbo = to_vmw_bo(&bo->base);
>  
>  	/*
>  	 * mkwrite() doesn't handle the VM_FAULT_RETRY return value correctly.
> @@ -419,22 +437,7 @@ vm_fault_t vmw_bo_vm_mkwrite(struct vm_fault *vmf)
>  	if (ret)
>  		return ret;
>  
> -	page_offset = vmf->pgoff - drm_vma_node_start(&bo->base.vma_node);
> -	if (unlikely(page_offset >= PFN_UP(bo->resource->size))) {
> -		ret = VM_FAULT_SIGBUS;
> -		goto out_unlock;
> -	}
> -
> -	if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE &&
> -	    !test_bit(page_offset, &vbo->dirty->bitmap[0])) {
> -		struct vmw_bo_dirty *dirty = vbo->dirty;
> -
> -		__set_bit(page_offset, &dirty->bitmap[0]);
> -		dirty->start = min(dirty->start, page_offset);
> -		dirty->end = max(dirty->end, page_offset + 1);
> -	}
> -
> -out_unlock:
> +	ret = vmw_bo_dirty_mkwrite(vmf, bo);
>  	dma_resv_unlock(bo->base.resv);
>  	return ret;
>  }
> @@ -484,6 +487,9 @@ vm_fault_t vmw_bo_vm_fault(struct vm_fault *vmf)
>  		prot = vm_get_page_prot(vma->vm_flags);
>  
>  	ret = ttm_bo_vm_fault_reserved(vmf, prot, num_prefault);
> +	if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
> +		WARN_ON_ONCE(vmw_bo_dirty_mkwrite(vmf, bo));
> +
>  	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
>  		return ret;
>  

Re: [PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

[Sergio Lopez Pascual]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Warning - DRM parts (i.e. most of the patches) untested; I have Cc'd
> the reporter to help with testing these patches.

Tested alongside with the mm fix [1], and it fixes the virtio-gpu
mapping issue [2] for both the absent PTE case and the pre-faulted
read-only PTE case.

Tested-by: Sergio Lopez <slp@redhat.com>

Thanks, Paolo.

[1] https://lore.kernel.org/kvm/CABgObfbkqYNsPQnKxK1_4adXF_tSdtScPU5-Xrg-sXyeWfMVfQ@mail.gmail.com/T/#t
[2] https://lore.kernel.org/kvm/299bddc0-fafc-48b3-a9c6-ec171136a46a@redhat.com/T/#t

Re: [PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

[David Hildenbrand (Arm)]

On 7/31/26 18:43, Paolo Bonzini wrote:
> Warning - DRM parts (i.e. most of the patches) untested; I have Cc'd
> the reporter to help with testing these patches.
> 
> 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.

How is mprotect() supposed to work in that case?

-- 
Cheers,

David

Re: [PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

[Paolo Bonzini]

On 8/3/26 13:54, David Hildenbrand (Arm) wrote:
> On 7/31/26 18:43, Paolo Bonzini wrote:
>> Warning - DRM parts (i.e. most of the patches) untested; I have Cc'd
>> the reporter to help with testing these patches.
>>
>> 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.
> 
> How is mprotect() supposed to work in that case?

Hi David,

not sure what you are worried about specifically, but fixup_user_fault() 
catches !VM_WRITE VMAs and returns early (see vma_permits_fault()).

Also, do_wp_page() has the comment:

         /*
          * Shared mapping: we are guaranteed to have VM_WRITE and
          * FAULT_FLAG_WRITE set at this point.
          */

before the call to wp_pfn_shared() which is where .pfn_mkwrite() is called.

Let me know if this was not what you were asking.

Thanks for the review of patch 1---I mentioned here in the cover letter 
that the name was temporary and I'll take your suggestion.  I can either 
use EXPORT_SYMBOL_GPL or switch to inlines, but not both because the 
existing functions like vmf_insert_pfn_prot() need to stay non-GPL-only.

Anyhow, now that the series has a Tested-by I'll clean up everything, 
and repost later this week.

Paolo

Re: [PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

[David Hildenbrand (Arm)]

On 8/3/26 16:19, Paolo Bonzini wrote:
> On 8/3/26 13:54, David Hildenbrand (Arm) wrote:
>> On 7/31/26 18:43, Paolo Bonzini wrote:
>>> Warning - DRM parts (i.e. most of the patches) untested; I have Cc'd
>>> the reporter to help with testing these patches.
>>>
>>> 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.
>>
>> How is mprotect() supposed to work in that case?
> 
> Hi David,

Hi!

> 
> not sure what you are worried about specifically, but fixup_user_fault() catches !VM_WRITE VMAs and returns early (see vma_permits_fault()).

That part is clear, I was wondering about the following:

mprotect(PROT_READ)

followed by

mprotect(PROT_READ | PROT_WRITE)

You'd similarly end up without the writable bit in the PTE, and apparently there is not really a way
to recover from this.

Maybe that's just ok (just sounded odd :) ).

> 
> Also, do_wp_page() has the comment:
> 
>         /*
>          * Shared mapping: we are guaranteed to have VM_WRITE and
>          * FAULT_FLAG_WRITE set at this point.
>          */
> 
> before the call to wp_pfn_shared() which is where .pfn_mkwrite() is called.
> 
> Let me know if this was not what you were asking.
> 
> Thanks for the review of patch 1---I mentioned here in the cover letter that the name was temporary and I'll take your suggestion.  I can either use EXPORT_SYMBOL_GPL or switch to inlines, but not
> both because the existing functions like vmf_insert_pfn_prot() need to stay non-GPL-only.
> 
> Anyhow, now that the series has a Tested-by I'll clean up everything, and repost later this week.

Thanks!

-- 
Cheers,

David

Re: [PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

[Paolo Bonzini]

On Mon, Aug 3, 2026 at 5:18 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
> >>> 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.
> >>
> >> How is mprotect() supposed to work in that case?
> >
> I was wondering about the following:
>
> mprotect(PROT_READ)
>
> followed by
>
> mprotect(PROT_READ | PROT_WRITE)
>
> You'd similarly end up without the writable bit in the PTE, and apparently there is not really a way
> to recover from this.

Why not? mprotect_fixup() calls vma_set_page_prot(), the PTE as you
say lacks the writable bit (unless pte_dirty(pte)), and then the next
fault calls .pfn_mkwrite().

Paolo

Re: [PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

[David Hildenbrand (Arm)]

On 8/4/26 09:50, Paolo Bonzini wrote:
> On Mon, Aug 3, 2026 at 5:18 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
>>>
>> I was wondering about the following:
>>
>> mprotect(PROT_READ)
>>
>> followed by
>>
>> mprotect(PROT_READ | PROT_WRITE)
>>
>> You'd similarly end up without the writable bit in the PTE, and apparently there is not really a way
>> to recover from this.
> 
> Why not? mprotect_fixup() calls vma_set_page_prot(), the PTE as you
> say lacks the writable bit (unless pte_dirty(pte)), and then the next
> fault calls .pfn_mkwrite().

Ah, if this works, great. I guess I was confused about your explanation about
fixup_user_fault().

So this really only about avoiding the second fault, makes sense thanks!

-- 
Cheers,

David

Re: [PATCH RFT 0/3] mm, drm: ensure .fault() does not have to be followed by .pfn_mkwrite() for write faults

[Paolo Bonzini]

On 8/3/26 10:55, David Hildenbrand (Arm) wrote:
> On 7/31/26 18:05, Paolo Bonzini wrote:
>> Reported-by: Sergio Lopez <slp@redhat.com>
> 
> Reported-by: without Fixes: is odd.

Fixes: 6da8e9634bb7 ("mm: new follow_pfnmap API") would also be odd :) 
but I can certainly add it.
>> +	 * @write_fault: if true, fail with -EFAULT unless the mapping is
> 
> Just wondering whether EPERM would be better.

It would be EACCES if anything, not EPERM; but almost all callers 
already pass EFAULT to userspace, and write() to a PROT_READ area 
returns EFAULT, so I don't think EACCES is the right choice.

>> +	 * 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?
There are no for_write, write_access or check_write in mm/,  but there 
are a handful of each of these

	int write = (gup_flags & FOLL_WRITE);
	bool write = vmf->flags & FAULT_FLAG_WRITE;

so I'll go for just "write".

Thanks,

Paolo