Re: [PATCH v3 5/5] RISC-V: KVM: Add the eager_page_split module parameter

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The `eager_page_split` module parameter is declared globally without the `static` keyword, leading to namespace pollution.

Pre-existing issues:
- [Critical] Dropping `mmu_lock` before flushing the TLB for newly write-protected pages allows the guest to modify memory without KVM recording it in the dirty bitmap, leading to silent data corruption during live migration. This problem wasn't introduced by this patch, but it was exposed in the modified code.
--

commit 775425d655f0c265e56c7d5ec6ecfe710e56385a
Author: Wang Yechao <wang.yechao255@zte.com.cn>

RISC-V: KVM: Add the eager_page_split module parameter

This commit introduces the eager_page_split module parameter for RISC-V KVM
to control whether huge pages are proactively split during dirty logging.
Eager page splitting is enabled by default.

[ ... ]

> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index d2d36a9dca06f..89f0dc237cf7b 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c
> @@ -16,6 +16,9 @@
>  #include <asm/kvm_mmu.h>
>  #include <asm/kvm_nacl.h>
>  
> +bool __read_mostly eager_page_split = true;
> +module_param(eager_page_split, bool, 0644);

[Severity: Low]
Should this variable be declared with the static keyword?

Because eager_page_split is only used in this file, omitting static pollutes
the global namespace and might cause silent collisions with variables in
other built-in drivers or subsystems. It can also trigger a
-Wmissing-prototypes warning.

[ ... ]

> @@ -165,8 +168,10 @@ void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *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);
> +	if (kvm_dirty_log_manual_protect_and_init_set(kvm)) {
> +		if (READ_ONCE(eager_page_split))
> +			mmu_split_huge_pages(&gstage, start, end);
> +	}

[Severity: Critical]
This is a pre-existing issue, but does dropping the lock in
mmu_split_huge_pages() without first flushing the TLB allow the guest to
bypass dirty tracking?

During KVM_CLEAR_DIRTY_LOG, the generic KVM core clears the dirty bit in its
bitmap before calling the architecture hook here in
kvm_arch_mmu_enable_log_dirty_pt_masked().

kvm_riscv_gstage_wp_range() makes the KVM page tables read-only, but its
return value (which indicates a TLB flush is needed) is ignored. We then
call mmu_split_huge_pages() which periodically drops kvm->mmu_lock to call
cond_resched():

mmu_split_huge_pages() {
    ...
    if (need_topup_split_caches_or_resched(pcache, count)) {
        ...
        write_unlock(&kvm->mmu_lock);
        cond_resched();
        ...
}

While the lock is dropped, guest vCPUs can still write to memory using
stale, writable TLB entries. Since the dirty bit was already cleared, these
writes silently bypass KVM's dirty tracking and are lost during live
migration.

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