Re: [PATCH v4 4/5] RISC-V: KVM: Split huge pages during KVM_CLEAR_DIRTY_LOG

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dropping `mmu_lock` after write-protecting page tables but before flushing the TLB leads to lost dirty pages.

Pre-existing issues:
- [Critical] `kvm_riscv_gstage_split_huge` overwrites leaf PTEs non-atomically, clobbering concurrent hardware A/D bit updates.
--

commit d0195abbe59e08ad1f5c62c2d279ff75e0b3a3f5
Author: Wang Yechao <wang.yechao255@zte.com.cn>
RISC-V: KVM: Split huge pages during KVM_CLEAR_DIRTY_LOG

This commit splits huge pages on the range specified using the clear dirty log
ioctl. It also avoids splitting when enabling dirty logging if the initially
set flag is present.

> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index 056c0abe278af..29a7ac023d4db 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c
> @@ -166,6 +166,9 @@ void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
>  	kvm_riscv_gstage_init(&gstage, kvm);
>  
>  	kvm_riscv_gstage_wp_range(&gstage, start, end);
> +
> +	if (kvm_dirty_log_manual_protect_and_init_set(kvm))
> +		mmu_split_huge_pages(&gstage, start, end);

[Severity: High]
Does ignoring the boolean return value from kvm_riscv_gstage_wp_range() create
a vulnerability window?

If mmu_split_huge_pages() drops the mmu_lock and yields before the TLB is
flushed:

arch/riscv/kvm/mmu.c:mmu_split_huge_pages() {
    ...
		if (need_topup_split_caches_or_resched(kvm, count)) {
			...
			write_unlock(&kvm->mmu_lock);
			cond_resched();
			...
}

vCPUs could continue writing to these pages using stale writable TLB entries.

Since the software dirty bitmap was already cleared by the clear dirty log
ioctl prior to this call, could these writes be completely lost during
migration?

[Severity: Critical]
This isn't a bug introduced by this patch, but looking at
mmu_split_huge_pages() led me to kvm_riscv_gstage_split_huge() which
overwrites leaf PTEs non-atomically:

arch/riscv/kvm/gstage.c:kvm_riscv_gstage_split_huge() {
    ...
		huge_pte = pte_val(ptep_get(ptep));
        ...
		set_pte(ptep, pfn_pte(PFN_DOWN(__pa(next_ptep)),
				__pgprot(_PAGE_TABLE)));
    ...
}

Does this set_pte() clobber concurrent hardware accessed/dirty bit updates?

If the hardware page table walker sets the dirty bit on the huge leaf PTE
between the initial read of huge_pte and this unconditional set_pte()
overwrite, those hardware updates appear to be lost.

Should this use an atomic operation (like a cmpxchg loop) when modifying leaf
PTEs shared with hardware to avoid missing dirty pages?

>  }

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