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

KVM:

On Tue, Aug 04, 2026, Paolo Bonzini wrote:
> The VM_READ and VM_WRITE flags are checked only at the very end of
> hva_to_pfn().  For both the hva_to_pfn_remapped() case and for regular
> mappings, this adds unnecessary cases and inconsistent error behavior.
> 
> For hva_to_pfn_remapped(), the code is relying on fixup_user_fault() to
> detect this situation.  This is fragile because hva_to_pfn_remapped()
> returns different error codes for a !VM_WRITE VMA depending on whether
> the PTE happens to be mapped:
> 
> * 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.  Likewise,
> a PROT_NONE pfnmap VMA would be mapped into the guest if the PTE was
> pte_present()[1] when the guest attempted to read it; with the patch
> instead KVM uniformly returns KVM_PFN_ERR_FAULT.  Doing the check early
> avoids these special cases and also sidesteps the issue pointed out at
> https://sashiko.dev/#/patchset/20260731160514.1101989-1-pbonzini%40redhat.com.
> 
> 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;
> however it should be considered part of the KVM API because mmu_stress_test
> relies on it.
> 
> Still, even with this snag about the returned pfn error code, pull the
> vm_flags checks in front so that they are done for all VMAs and the
> above inconsistency goes away for the VM_IO|VM_PFNMAP case.
> 
> [1] on x86, for example, such a page would have _PAGE_PRESENT clear
>   but _PAGE_PROTNONE set
> 
> Fixes: 28e3918179aa ("drm/gem-shmem: Track folio accessed/dirty status in mmap")
> Cc: [email protected]
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
>  virt/kvm/kvm_main.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 45e784462ec6..576bcb21be3a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2925,17 +2925,6 @@ static int hva_to_pfn_slow(struct kvm_follow_pfn *kfp, kvm_pfn_t *pfn)
>  	return npages;
>  }
>  
> -static bool vma_is_valid(struct vm_area_struct *vma, bool write_fault)
> -{
> -	if (unlikely(!(vma->vm_flags & VM_READ)))
> -		return false;
> -
> -	if (write_fault && (unlikely(!(vma->vm_flags & VM_WRITE))))
> -		return false;
> -
> -	return true;
> -}
> -
>  static int hva_to_pfn_remapped(struct vm_area_struct *vma,
>  			       struct kvm_follow_pfn *kfp, kvm_pfn_t *p_pfn)
>  {
> @@ -3008,20 +2997,29 @@ kvm_pfn_t hva_to_pfn(struct kvm_follow_pfn *kfp)
>  retry:
>  	vma = vma_lookup(current->mm, kfp->hva);
>  
> -	if (vma == NULL)
> +	/*
> +	 * GUP failed.  It could be an inaccessible mapping, a pfnmap one,
> +	 * or the page might be absent.
> +	 */
> +

Unnecessary newline, IMO.

> +	if (vma == NULL || unlikely(!(vma->vm_flags & VM_READ))) {
>  		pfn = KVM_PFN_ERR_FAULT;
> -	else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
> +	} else if ((kfp->flags & FOLL_WRITE) && unlikely(!(vma->vm_flags & VM_WRITE))) {
> +		/*
> +		 * Exit to userspace for PROT_READ mappings in a writable
> +		 * memslot, as this is part of the API.

Can we say something along the lines of "for backwards compatibility" instead
of saying this is part of the API?  Because that's definitely not KVM's documented
API, and we're hoping it's not part of KVM's undocumented API either.

> +		 */
> +		pfn = vma->vm_flags & (VM_IO | VM_PFNMAP) ? KVM_PFN_ERR_RO_FAULT :
> +			KVM_PFN_ERR_FAULT;

Please align the two branches of the ternary operators:

		pfn = vma->vm_flags & (VM_IO | VM_PFNMAP) ? KVM_PFN_ERR_RO_FAULT :
							    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;