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

On Mon, Aug 03, 2026, Paolo Bonzini wrote:
> 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=E2=80=AFPM Sean Christopherson <seanjc@g=
oogle.com> wrote:
> > > > > * if the PTE is present, follow_pfnmap_start() sets args.writable=
 to
> > > > >    false and KVM_PFN_ERR_RO_FAULT is returned;
> > > > >=20
> > > > > * if no PTE is present, fixup_user_fault(FAULT_FLAG_WRITE) return=
s
> > > > >    -EFAULT after checking vma_permits_fault(), and hva_to_pfn() e=
nds
> > > > >    up returning KVM_PFN_ERR_FAULT.
> > > > >=20
> > > > > With this patch KVM_PFN_ERR_RO_FAULT is returned uniformly.
> > > >=20
> > > > IMO, returning KVM_PFN_ERR_RO_FAULT on a read-only VMA is wrong.  A=
FAICT, 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 cas=
e 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 yea=
rs.
> > >=20
> > > 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
> >=20
> > I don't see how this would help with fixup_user_fault() not actually fi=
xing a
> > fault.  Neither returning -EFAULT nor emulating is correct KVM behavior=
.
> >=20
> > > - it avoids the sashiko issue reported for
> > > https://lore.kernel.org/r/20260731160514.1101989-1-pbonzini%40redhat.=
com/).
> >=20
> > But the issue Sashiko reported is just saying that KVM sometimes does w=
hat I'm
> > saying KVM should do all the time: return -EFAULT.  Or did I misunderst=
and that
> > one too? :-)
>=20
> Yes, that's correct.  My point is I'd rather not introduce other changes =
to
> the !VM_WRITE case in stable releases.

Yeah, agreed.  But what I don't understand is why this would be sent to sta=
ble@
in the first place.

> So, the follow_pfnmap_start() patch I posted last Friday is part of the f=
ix
> 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.
>=20
> 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 w=
e
> can, uniformly, do the change from RO_FAULT to -EFAULT.
>=20
> Paolo
>=20
> > > >          /*
> > > >           * GUP failed.  It could be an inaccessible mapping, a pfn=
map one,
> > > >           * or the page might be absent.
> > > >           */
> > > >          if (vma =3D=3D NULL || unlikely(!(vma->vm_flags & VM_READ)=
) ||
> > > >              ((kfp->flags & FOLL_WRITE) && unlikely(!(vma->vm_flags=
 & VM_WRITE)))) {
> > > >                  pfn =3D KVM_PFN_ERR_FAULT;
> > > >          } else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
> > > >                  r =3D hva_to_pfn_remapped(vma, kfp, &pfn);
> > > >                  if (r =3D=3D -EAGAIN)
> > > >                          goto retry;
> > > >                  if (r < 0)
> > > >                          pfn =3D KVM_PFN_ERR_FAULT;
> > > >          } else {
> > > >                  pfn =3D kfp->flags & FOLL_NOWAIT ? KVM_PFN_ERR_NEE=
DS_IO :
> > > >                                                   KVM_PFN_ERR_FAULT=
;
> > > >          }
> > >=20
> > > Yes, but I'd do that only in 7.3.
> >=20
>=20
>=20