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

On Mon, Aug 03, 2026, Paolo Bonzini wrote:
> On Fri, Jul 31, 2026 at 8:46 PM Sean Christopherson <[email protected]> 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? :-)

> > > For regular mappings a PROT_READ VMA, if placed in a writable memslot,
> > > would return KVM_PFN_ERR_FAULT instead of KVM_PFN_ERR_RO_FAULT when
> > > the guest writes to it.  This would cause a -EFAULT exit to userspace,
> > > instead of triggering emulation as the VM_IO|VM_PFNMAP arm would do;
> >
> > No, arm64 is checking the memslot, not the VMA.
> >
> >         hva = gfn_to_hva_memslot_prot(memslot, gfn, &writable);
> >         write_fault = kvm_is_write_fault(vcpu);
> >         if (kvm_is_error_hva(hva) || (write_fault && !writable)) {
> >
> > Or are you talking about different code?
> 
> I am talking about the "arm" of the if/else if/else. :)

LOL, overthought that one a bit.

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