Re: [PATCH v2 1/3] KVM: riscv: Implement KVM_PRE_FAULT_MEMORY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] RISC-V KVM_PRE_FAULT_MEMORY leaks -EAGAIN to userspace on mmu_invalidate_retry or HWPOISON, which violates the ABI and differs from x86.

Pre-existing issues:
- [High] kvm_riscv_gstage_set_pte overwrites a non-leaf PTE with a leaf PTE without freeing the lower-level page table, causing a memory leak on THP collapse.
--

--- Patch [1]: [PATCH v2 1/3] KVM: riscv: Implement KVM_PRE_FAULT_MEMORY ---
commit 39a9f049be58ef7b5671f786fdc8299c1f217042
Author: Jinyu Tang <jinyu.tang@linux.dev>

KVM: riscv: Implement KVM_PRE_FAULT_MEMORY

The generic KVM_PRE_FAULT_MEMORY ioctl lets userspace populate KVM page
tables before running a vCPU over a GPA range. This commit adds the
RISC-V arch hook and reuses the existing G-stage fault mapping path.

> diff --git a/arch/riscv/kvm/gstage.c b/arch/riscv/kvm/gstage.c
> index b0474fcf065aa..ebd1e8a4b9e38 100644
> --- a/arch/riscv/kvm/gstage.c
> +++ b/arch/riscv/kvm/gstage.c

[ ... ]

> @@ -280,6 +280,9 @@ int kvm_riscv_gstage_map_page(struct kvm_gstage *gstage,
>  						    out_map->level, true);
>  		} else if (ALIGN_DOWN(PFN_PHYS(pte_pfn(ptep_get(ptep))), page_size) == hpa) {
>  			kvm_riscv_gstage_update_pte_prot(gstage, ptep_level, gpa, ptep, prot);
> +			out_map->addr = ALIGN_DOWN(gpa, page_size);
> +			out_map->level = ptep_level;
> +			out_map->pte = ptep_get(ptep);
>  			return 0;
>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does kvm_riscv_gstage_set_pte()
overwrite non-leaf PTEs with leaf PTEs without freeing the lower-level page
table?

When a THP collapse occurs, KVM attempts to map the huge page at level 1.
If the previous 4K leaf PTEs at level 0 were unmapped (e.g., via 
kvm_riscv_gstage_unmap_range()) but the level-0 page table was left allocated,
kvm_riscv_gstage_get_leaf() returns false.

This function then falls through and calls kvm_riscv_gstage_set_pte() to
install the level-1 leaf PTE. That appears to blindly overwrite the
level-1 non-leaf PTE with a leaf PTE via set_pte(ptep, map->pte) without
freeing the level-0 page table it pointed to.

Could this lead to a memory leak of level-0 page tables over time?

> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index 8a0aa5e0e216e..926b53723e31c 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c

[ ... ]

> @@ -661,6 +661,39 @@ int kvm_riscv_mmu_map(struct kvm_vcpu *vcpu, struct kvm_memory_slot *memslot,
>  	return ret;
>  }
>  
> +long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
> +				    struct kvm_pre_fault_memory *range)
> +{

[ ... ]

> +	ret = kvm_riscv_mmu_map(vcpu, memslot, range->gpa, hva, false, &out_map);
> +	if (ret)
> +		return ret;
> +
> +	if (!pte_val(out_map.pte))
> +		return -EAGAIN;

[Severity: High]
Does returning -EAGAIN here leak this internal error code to userspace?

When kvm_riscv_mmu_map() handles a concurrent host MMU invalidation
(mmu_invalidate_retry() returns true) or a HWPOISONed page, it returns 0
but does not populate out_map.pte.

This arch hook then translates the internal !pte_val(out_map.pte) state
into an -EAGAIN return value. Since the generic KVM ioctl loop bubbles
this up, unprivileged userspace VMMs executing the KVM_PRE_FAULT_MEMORY
ioctl concurrently with memory pressure or page migration will receive
an undocumented -EAGAIN error, potentially causing them to fatally abort
VM initialization.

Should this return 0 instead to indicate no progress, or is a different
mechanism needed to avoid breaking the ABI?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811230316.218770-1-jinyu.tang@linux.dev?part=1