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

On Fri, Jul 31, 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.

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.

> 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;

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?

> however it should be considered part of the KVM API because mmu_stress_test
> relies on it.

...

>  	} else {
> -		if ((kfp->flags & FOLL_NOWAIT) &&
> -		    vma_is_valid(vma, kfp->flags & FOLL_WRITE))
> -			pfn = KVM_PFN_ERR_NEEDS_IO;
> -		else
> -			pfn = KVM_PFN_ERR_FAULT;
> +		pfn = kfp->flags & FOLL_NOWAIT
> +			? KVM_PFN_ERR_NEEDS_IO : KVM_PFN_ERR_FAULT;

I find this style much easier to read:

		pfn = kfp->flags & FOLL_NOWAIT ? KVM_PFN_ERR_NEEDS_IO :
						 KVM_PFN_ERR_FAULT;

If we "fix" the read-only behavior, isn't it this?

	/*
	 * 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;
	}


>  	}
>  	mmap_read_unlock(current->mm);
>  	return pfn;
> -- 
> 2.55.0
>