Re: [PATCH v5 3/5] RISC-V: KVM: Remove redundant TLB flush operations

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Removing the local TLB flush exposes a race window where KVM MMU lock is dropped while hardware TLBs contain stale writable entries, leading to silently lost guest writes.

Pre-existing issues:
- [High] `kvm_riscv_gstage_op_pte` uses non-atomic operations to modify leaf PTEs, which clobbers concurrent hardware A/D bit updates.
--

--- Patch [3]: [PATCH v5 3/5] RISC-V: KVM: Remove redundant TLB flush operations ---
commit ab801407dda8f4868b984976c778a3125e138920
Author: Wang Yechao <wang.yechao255@zte.com.cn>

RISC-V: KVM: Remove redundant TLB flush operations

This commit removes a TLB flush from kvm_arch_mmu_enable_log_dirty_pt_masked()
during dirty logging initialization. It relies on the callers to perform the
remote TLB flush later.

> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index 2362eeb6c08a5..5a93cffa9ad7f 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c
> @@ -163,14 +163,15 @@ void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
>  	phys_addr_t start = (base_gfn +  __ffs(mask)) << PAGE_SHIFT;
>  	phys_addr_t end = (base_gfn + __fls(mask) + 1) << PAGE_SHIFT;
>  	struct kvm_gstage gstage;
> -	bool flush;
>  
>  	kvm_riscv_gstage_init(&gstage, kvm);
>  
> -	flush = kvm_riscv_gstage_wp_range(&gstage, start, end);

[Severity: High]
This is a pre-existing issue, but does kvm_riscv_gstage_op_pte() clobber
concurrent hardware A/D bit updates when write-protecting these pages?

Looking at kvm_riscv_gstage_op_pte() called by kvm_riscv_gstage_wp_range(),
it uses a non-atomic read-modify-write sequence for leaf PTEs:

arch/riscv/kvm/gstage.c:kvm_riscv_gstage_op_pte() {
    ...
    else if (op == GSTAGE_OP_WP)
        set_pte(ptep, __pte(pte_val(ptep_get(ptep)) & ~_PAGE_WRITE));
    ...
}

If the RISC-V hardware page-table walker concurrently sets Access or Dirty
bits on the same PTE while KVM is write-protecting it, could this non-atomic
update silently overwrite and lose those hardware updates?

> -	if (flush)
> -		kvm_flush_remote_tlbs_range(kvm, start >> PAGE_SHIFT,
> -					    (end - start) >> PAGE_SHIFT);
> +	kvm_riscv_gstage_wp_range(&gstage, start, end);
> +
> +	/*
> +	 * remote TLB flush is not needed here since callers of
> +	 * kvm_arch_mmu_enable_log_dirty_pt_masked() already do it.
> +	 */

[Severity: Critical]
Is it safe to delay the remote TLB flush until the callers execute it?

When kvm_arch_mmu_enable_log_dirty_pt_masked() returns, some callers drop
the KVM MMU lock before the remote TLB flush happens. For example, in the
dirty ring reset path:

virt/kvm/dirty_ring.c:kvm_reset_dirty_gfn() {
    ...
    KVM_MMU_LOCK(kvm);
    kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot, offset, mask);
    KVM_MMU_UNLOCK(kvm);
    ...
}

Could this expose a window where vCPUs can use stale writable TLB entries
to write to pages without triggering a page fault, silently losing those
guest writes before the TLB is eventually flushed by the caller?

>  }
>  
>  void kvm_arch_sync_dirty_log(struct kvm *kvm, struct kvm_memory_slot *memslot)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731091215.1549430-1-wang.yechao255@zte.com.cn?part=3