Re: [PATCH] KVM: riscv: Fix infinite loop in NACL hfence entry allocation

[sashiko-bot@kernel.org]

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

New issues:
- [High] Silent drop of critical guest TLB flushes when the shared memory queue is full

Pre-existing issues:
- [High] Unsafe usage of this_cpu_ptr() in preemptible context leads to lost TLB flushes and data races
- [High] TOCTOU race and memory ordering violation in NACL shared memory update
--

--- Patch [1]: [PATCH] KVM: riscv: Fix infinite loop in NACL hfence entry allocation ---
commit 30204e283205d026c21e76906b9dab006ad09704
Author: Zongmin Zhou <zhouzongmin@kylinos.cn>

KVM: riscv: Fix infinite loop in NACL hfence entry allocation

try_count is initialized to 5 but never decremented in the retry path,
making the if (try_count) check always true. If all NACL shared memory
hfence entries remain in the pending state after sync, the function loops
forever, causing a soft lockup. Decrement try_count on each retry so the
fallback warning and return become reachable.

> diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
> index 08a95ad9ada2c..54a0158bd38a9 100644
> --- a/arch/riscv/kvm/nacl.c
> +++ b/arch/riscv/kvm/nacl.c
> @@ -33,7 +33,7 @@ void __kvm_riscv_nacl_hfence(void *shmem,
>  	}
>  
>  	if (ent < 0) {
> -		if (try_count) {
> +		if (try_count--) {
>  			nacl_sync_hfence(-1UL);
>  			goto again;
>  		} else {

[Severity: High]
Does this silent return leave stale translations in the hardware TLB?

By making the fallback return reachable when the queue is full:

		} else {
			pr_warn("KVM: No free entry in NACL shared memory\n");
			return;
		}

the caller is unaware the flush failed. Missing a mandatory guest TLB flush
violates core memory management invariants and could allow the guest to
access invalid physical memory.

Should KVM propagate an error to userspace or fall back to a synchronous
local flush instead of silently discarding the request?

[Severity: High]
This is a pre-existing issue, but could the subsequent write sequence cause a
TOCTOU race with the L0 hypervisor?

Looking further down in __kvm_riscv_nacl_hfence():

	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_CONFIG(i);
	*entp = cpu_to_lelong(control);
	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_PNUM(i);
	*entp = cpu_to_lelong(page_num);
	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_PCOUNT(i);
	*entp = cpu_to_lelong(page_count);

The CONFIG field, containing the SBI_NACL_SHMEM_HFENCE_CONFIG_PEND flag,
is written before the PNUM and PCOUNT payload fields without any memory
barriers.

If the compiler or a weakly ordered RISC-V CPU reorders these writes, could
the L0 hypervisor read the PEND bit while the payload is incomplete, leading
to incorrect or missing TLB invalidations?

[Severity: High]
This is a pre-existing issue, but does the TLB flush processing happen while
preemption is still enabled?

Looking at kvm_arch_vcpu_ioctl_run() in arch/riscv/kvm/vcpu.c:

	ret = kvm_riscv_check_vcpu_requests(vcpu);
	if (ret <= 0)
		continue;

	preempt_disable();

If kvm_riscv_check_vcpu_requests() processes TLB flushes via
kvm_riscv_hfence_process(), it resolves the shared memory pointer using
nacl_shmem(), which relies on this_cpu_ptr().

If the VCPU thread is preempted and migrated to a different physical CPU
immediately after the pointer is resolved, will it write to the old CPU's
memory and cause lost TLB flushes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716073756.44153-1-min_halo@163.com?part=1