Re: [PATCH] kvm: apply VM_READ/VM_WRITE checks to all VMA types

[Paolo Bonzini <pbonzini@redhat.com>]

On 8/3/26 15:40, Sean Christopherson wrote:
> On Mon, Aug 03, 2026, Paolo Bonzini wrote:
>> On Fri, Jul 31, 2026 at 8:46 PM Sean Christopherson <seanjc@google.com> wrote:
>>>> * if the PTE is present, follow_pfnmap_start() sets args.writable to
>>>>    false and KVM_PFN_ERR_RO_FAULT is returned;
>>>>
>>>> * if no PTE is present, fixup_user_fault(FAULT_FLAG_WRITE) returns
>>>>    -EFAULT after checking vma_permits_fault(), and hva_to_pfn() ends
>>>>    up returning KVM_PFN_ERR_FAULT.
>>>>
>>>> With this patch KVM_PFN_ERR_RO_FAULT is returned uniformly.
>>>
>>> IMO, returning KVM_PFN_ERR_RO_FAULT on a read-only VMA is wrong.  AFAICT, that
>>> behavior for VM_{IO,PFNMAP} was added by commit bd2fae8da794 ("KVM: do not assume
>>> PTE is writable after follow_pfn").  Given that that's the only case where KVM
>>> returns KVM_PFN_ERR_RO_FAULT, I would much prefer to fix that wart and cross our
>>> fingers nothing has come to rely on the behavior in the last ~5 years.
>>
>> We can try, but I'd rather not do that in stable releases (while this
>> patch would be applied there, as a first step towards fixing the DRM
>> issue that Sergio reported
> 
> I don't see how this would help with fixup_user_fault() not actually fixing a
> fault.  Neither returning -EFAULT nor emulating is correct KVM behavior.
> 
>> - it avoids the sashiko issue reported for
>> https://lore.kernel.org/r/20260731160514.1101989-1-pbonzini%40redhat.com/).
> 
> But the issue Sashiko reported is just saying that KVM sometimes does what I'm
> saying KVM should do all the time: return -EFAULT.  Or did I misunderstand that
> one too? :-)

Yes, that's correct.  My point is I'd rather not introduce other changes 
to the !VM_WRITE case in stable releases.

So, the follow_pfnmap_start() patch I posted last Friday is part of the 
fix for Sergio's report; but it introduces one such change---which 
indeed we agree is desirable behavior, but which I'd rather not sneak in 
as part of an unrelated fix for a regression.

So, *this* patch removes a bunch of cases in which hva_to_pfn_remapped() 
is inconsistent, but it leaves RO_FAULT in place for now.  Then 
separately we can, uniformly, do the change from RO_FAULT to -EFAULT.

Paolo

>>>          /*
>>>           * GUP failed.  It could be an inaccessible mapping, a pfnmap one,
>>>           * or the page might be absent.
>>>           */
>>>          if (vma == NULL || unlikely(!(vma->vm_flags & VM_READ)) ||
>>>              ((kfp->flags & FOLL_WRITE) && unlikely(!(vma->vm_flags & VM_WRITE)))) {
>>>                  pfn = KVM_PFN_ERR_FAULT;
>>>          } else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
>>>                  r = hva_to_pfn_remapped(vma, kfp, &pfn);
>>>                  if (r == -EAGAIN)
>>>                          goto retry;
>>>                  if (r < 0)
>>>                          pfn = KVM_PFN_ERR_FAULT;
>>>          } else {
>>>                  pfn = kfp->flags & FOLL_NOWAIT ? KVM_PFN_ERR_NEEDS_IO :
>>>                                                   KVM_PFN_ERR_FAULT;
>>>          }
>>
>> Yes, but I'd do that only in 7.3.
>