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

Sean Christopherson <[email protected]> writes:

> On Fri, Jul 31, 2026, Sergio Lopez Pascual wrote:
>> Sean Christopherson <[email protected]> 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 <[email protected]>
>> >> ---
>> >
>> > 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.