[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

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

[Sergio Lopez Pascual]

Sean Christopherson <seanjc@google.com> writes:

> 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.

The issue goes beyond fixup_user_fault() behavior. If we're faulting on
a page which already has a ready-only PTE installed,
follow_pfnmap_start() returns success, fixup_user_fault isn't called at
all, and we're still hitting the "(write_fault && !args.writable)"
condition, setting p_pfn to KVM_PFN_ERR_RO_FAULT.

Without this change, KVM can't deal with VM_PFNMAP vmas with read-only
PTEs installed.

Thanks,
Sergio.

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

[Sean Christopherson]

On Fri, Jul 31, 2026, Sergio Lopez Pascual wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > 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.
> 
> The issue goes beyond fixup_user_fault() behavior. If we're faulting on
> a page which already has a ready-only PTE installed,
> follow_pfnmap_start() returns success, fixup_user_fault isn't called at
> all, and we're still hitting the "(write_fault && !args.writable)"
> condition, setting p_pfn to KVM_PFN_ERR_RO_FAULT.
> 
> Without this change, KVM can't deal with VM_PFNMAP vmas with read-only
> PTEs installed.

IMO, that's a bug in the APIs.  If a normal #PF handler looked at a write fault
and decided a read-only mapping sufficed, that would be a bug.  I don't see why
this is any different.

And if KVM has this problem, so will other users of follow_pfnmap_start() and
friends.

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

[Sergio Lopez Pascual]

Sean Christopherson <seanjc@google.com> writes:

> On Fri, Jul 31, 2026, Sergio Lopez Pascual wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>>
>> > 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.
>>
>> The issue goes beyond fixup_user_fault() behavior. If we're faulting on
>> a page which already has a ready-only PTE installed,
>> follow_pfnmap_start() returns success, fixup_user_fault isn't called at
>> all, and we're still hitting the "(write_fault && !args.writable)"
>> condition, setting p_pfn to KVM_PFN_ERR_RO_FAULT.
>>
>> Without this change, KVM can't deal with VM_PFNMAP vmas with read-only
>> PTEs installed.
>
> IMO, that's a bug in the APIs.  If a normal #PF handler looked at a write fault
> and decided a read-only mapping sufficed, that would be a bug.  I don't see why
> this is any different.

In the scenario I've described above, were a read-only PTE has been
installed on a fault generated by a read access, and a second fault is
generated later when writting to the page, we aren't invoking any #PF
handler. The call graph looks (roughly) like this:

          kvm_handle_guest_abort()
            ├─ gfn_to_memslot() → gfn_to_hva()
            └─ user_mem_abort()
                └─ kvm_s2_fault_pin_pfn()
                    └─ __kvm_faultin_pfn()
                        └─ hva_to_pfn()
                            ├─ hva_to_pfn_fast()
                            ├─ hva_to_pfn_slow()
                            └─ hva_to_pfn_remapped()

GUP rejects VM_PFNMAP vmas, and hva_to_pfn_remapped() is not calling
fixup_user_fault() because follow_pfnmap_start() finds the PTE. So no
fault handler is involved in this path.

Thanks,
Sergio.

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

[Paolo Bonzini]

On Fri, Jul 31, 2026 at 3:37 PM Sergio Lopez Pascual <slp@redhat.com> wrote:
> > IMO, that's a bug in the APIs.  If a normal #PF handler looked at a write fault
> > and decided a read-only mapping sufficed, that would be a bug.  I don't see why
> > this is any different.
>
> In the scenario I've described above, were a read-only PTE has been
> installed on a fault generated by a read access, and a second fault is
> generated later when writting to the page, we aren't invoking any #PF
> handler. The call graph looks (roughly) like this:
>
>           kvm_handle_guest_abort()
>             ├─ gfn_to_memslot() → gfn_to_hva()
>             └─ user_mem_abort()
>                 └─ kvm_s2_fault_pin_pfn()
>                     └─ __kvm_faultin_pfn()
>                         └─ hva_to_pfn()
>                             ├─ hva_to_pfn_fast()
>                             ├─ hva_to_pfn_slow()
>                             └─ hva_to_pfn_remapped()

There are two different bugs. First, let's look at this case where, as
you said, fixup_user_fault() is not executed. To force execution of
fixup_user_fault(), you'd need something like this in KVM:

     r = follow_pfnmap_start(&args);
    if (!r && write_fault && !args.writable) {
        follow_pfnmap_end(&args);
        r = -EACCES;
    }

     if (r) {

But that's also a bad idea for two reasons. First because you'd need
it for all callers.  Second, because all of follow_pfnmap_start()'s
callers anyway check something like "write_fault && !args.writable",
so the function might as well receive a new args.write_fault, and fail
with -EFAULT if args.write_fault && !writable.

Separately there is the case where fixup_user_fault() is executed, and
which right now would have to be done twice. The drm code already has
a solution in try_insert_pfn():

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

The driver could do the same even for PTEs, not just for PMDs. The
issue of code duplication is now on the other side, because every
caller has to do the job of .pfn_mkwrite() and in fact the same bug is
there in vmwgfx. But the exact kind of processing to be done in
.pfn_mkwrite() is different per driver, as opposed to
fixup_user_fault() users, and there is a precedent for the API with
vm_insert_page_mkwrite().

There is no function to do it, but that's fixable. I will send an RFC
series shortly.

Paolo