[PATCH v3] KVM: have hva_to_pfn_remapped write-upgrade PTEs

[PATCH v3] KVM: have hva_to_pfn_remapped write-upgrade PTEs

[Sergio Lopez]

After 28e39181 ("drm/gem-shmem: Track folio accessed/dirty status in
mmap") was merged, a guest write to an unpopulated PTE from a mapping
backed by a DRM GEM BO triggers a VM exit with EFAULT, with
hva_to_pfn_remapped setting p_pfn to KVM_PFN_ERR_RO_FAULT.

This happens because that commit implements pfn_mkwrite for
drm_gem_shmem_vm_ops. With that function present, vma_wants_writenotify
returns true in vma_set_page_prot, clearing VM_SHARED and leading to the
entry to be installed as read-only. This is done on purpose so the
fault handler gets notified when the entry is going to be written.

In KVM, hva_to_pfn_remapped calls to fixup_user_fault to trigger the
fault handler but, as seen above, this one might install a read-only PTE
even with FAULT_FLAG_WRITE present in fault_flags. The check at the end
of hva_to_pfn_remapped notices that the entry is not writable despite
this being a write fault and sets p_pfn to KVM_PFN_ERR_RO_FAULT.

To address this issue, have hva_to_pfn_remapped issue a second
fixup_user_fault call when needed for write-upgrading the PTE.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 virt/kvm/kvm_main.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 45e784462ec6..82c5a6b94267 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2941,6 +2941,7 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 {
 	struct follow_pfnmap_args args = { .vma = vma, .address = kfp->hva };
 	bool write_fault = kfp->flags & FOLL_WRITE;
+	bool unlocked = false;
 	int r;
 
 	/*
@@ -2957,7 +2958,6 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 		 * get_user_pages fails for VM_IO and VM_PFNMAP vmas and does
 		 * not call the fault handler, so do it here.
 		 */
-		bool unlocked = false;
 		r = fixup_user_fault(current->mm, kfp->hva,
 				     (write_fault ? FAULT_FLAG_WRITE : 0),
 				     &unlocked);
@@ -2972,8 +2972,27 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 	}
 
 	if (write_fault && !args.writable) {
-		*p_pfn = KVM_PFN_ERR_RO_FAULT;
-		goto out;
+		/*
+		 * VM_PFNMAP fault handlers may install read-only PTEs via
+		 * vmf_insert_pfn(), deferring the write upgrade to a second
+		 * fault. Trigger that upgrade now.
+		 */
+		follow_pfnmap_end(&args);
+		r = fixup_user_fault(current->mm, kfp->hva, FAULT_FLAG_WRITE,
+				     &unlocked);
+		if (unlocked)
+			return -EAGAIN;
+		if (r)
+			return r;
+
+		r = follow_pfnmap_start(&args);
+		if (r)
+			return r;
+
+		if (!args.writable) {
+			*p_pfn = KVM_PFN_ERR_RO_FAULT;
+			goto out;
+		}
 	}
 
 	*p_pfn = kvm_resolve_pfn(kfp, NULL, &args, args.writable);
-- 
2.55.0

Re: [PATCH v3] KVM: have hva_to_pfn_remapped write-upgrade PTEs

[Sean Christopherson]

On Wed, Jul 29, 2026, Sergio Lopez wrote:
> After 28e39181 ("drm/gem-shmem: Track folio accessed/dirty status in
> mmap") was merged, a guest write to an unpopulated PTE from a mapping
> backed by a DRM GEM BO triggers a VM exit with EFAULT, with
> hva_to_pfn_remapped setting p_pfn to KVM_PFN_ERR_RO_FAULT.
> 
> This happens because that commit implements pfn_mkwrite for
> drm_gem_shmem_vm_ops. With that function present, vma_wants_writenotify
> returns true in vma_set_page_prot, clearing VM_SHARED and leading to the
> entry to be installed as read-only. This is done on purpose so the
> fault handler gets notified when the entry is going to be written.
> 
> In KVM, hva_to_pfn_remapped calls to fixup_user_fault to trigger the
> fault handler but, as seen above, this one might install a read-only PTE
> even with FAULT_FLAG_WRITE present in fault_flags. The check at the end
> of hva_to_pfn_remapped notices that the entry is not writable despite
> this being a write fault and sets p_pfn to KVM_PFN_ERR_RO_FAULT.
> 
> To address this issue, have hva_to_pfn_remapped issue a second
> fixup_user_fault call when needed for write-upgrading the PTE.
> 
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---

NAK, this doesn't belong in KVM.  Expecting callers of fixup_user_fault() to
retry a FAULT_FLAG_WRITE fault on *success* is absurd.  Either manually do the
retry in fixup_user_fault(), or return VM_FAULT_RETRY so that KVM will naturally
retry.  I assume the latter is the correct approach.

Re: [PATCH v3] KVM: have hva_to_pfn_remapped write-upgrade PTEs

[Paolo Bonzini]

On 7/30/26 19:31, Sean Christopherson wrote:
> On Wed, Jul 29, 2026, Sergio Lopez wrote:
>> After 28e39181 ("drm/gem-shmem: Track folio accessed/dirty status in
>> mmap") was merged, a guest write to an unpopulated PTE from a mapping
>> backed by a DRM GEM BO triggers a VM exit with EFAULT, with
>> hva_to_pfn_remapped setting p_pfn to KVM_PFN_ERR_RO_FAULT.
>>
>> This happens because that commit implements pfn_mkwrite for
>> drm_gem_shmem_vm_ops. With that function present, vma_wants_writenotify
>> returns true in vma_set_page_prot, clearing VM_SHARED and leading to the
>> entry to be installed as read-only. This is done on purpose so the
>> fault handler gets notified when the entry is going to be written.
>
> NAK, this doesn't belong in KVM.  Expecting callers of fixup_user_fault() to
> retry a FAULT_FLAG_WRITE fault on *success* is absurd.  Either manually do the
> retry in fixup_user_fault(), or return VM_FAULT_RETRY so that KVM will naturally
> retry.  I assume the latter is the correct approach.
Doing it in fixup_user_fault(), or more precisely in __do_fault(),
seems hard.  The information about the trick (about the presence
of *_mkwrite) is only recorded in vma->vm_page_prot, which is an
opaque pgprot_t.  So it's only follow_pfnmap_start() that knows
how to retrieve it.

In the driver it would be I guess something like:

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index c989459eb215..ae913481a42c 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -658,6 +658,10 @@ static vm_fault_t drm_gem_shmem_any_fault(struct vm_fault *vmf, unsigned int ord
         if (ret == VM_FAULT_NOPAGE)
                 folio_mark_accessed(folio);

+       /* Force another round to ensure that pfn_mkwrite is called.  */
+       if (!(ret & VM_FAULT_ERROR) && (vmf->flags & FAULT_FLAG_WRITE))
+               ret = VM_FAULT_RETRY;
+
  out:
         dma_resv_unlock(obj->resv);


?  but it seems that the driver is in slightly uncharted waters.
try_insert_pfn() calls vmf_insert_pfn(), which says

/**
  * 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

...

  * vma cannot be a COW mapping.

except this *is* a COW mapping in some sense, or at least it
faults like one.

Paolo