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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Caching `gstage.pgd` across `mmu_lock` drops in `mmu_split_huge_pages` leads to a Use-After-Free if the page table is concurrently destroyed.
- [High] Eager huge page splitting skips PMDs when the address range is unaligned, leaving huge pages unsplit.

Pre-existing issues:
- [Critical] Non-atomic SPTE updates in `kvm_riscv_gstage_op_pte` clobber concurrent hardware Access/Dirty bit updates.
--

commit 80c4b9b5e74f99371bb99acaee95451275e9865f
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
KVM_CLEAR_DIRTY_LOG. It also ensures that splitting does not occur when
enabling dirty logging if KVM_DIRTY_LOG_INITIALLY_SET is set.

> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index 51bd1db74e1aa..d04680687f4a1 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c
> @@ -149,6 +149,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);

[Severity: Critical]
This is a pre-existing issue, but does kvm_riscv_gstage_wp_range()
non-atomically update PTEs, risking lost hardware A/D bit updates?

When kvm_riscv_gstage_wp_range() calls kvm_riscv_gstage_op_pte(), the
write-protection is done with a read-modify-write:

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 hardware page-table walker sets the Dirty (D) bit after KVM reads the
PTE but before KVM writes it back, will the hardware's update be lost? This
could lead to silent data corruption during live migration since KVM might
miss guest memory writes.

> +
> +	if (kvm_dirty_log_manual_protect_and_init_set(kvm))
> +		mmu_split_huge_pages(&gstage, start, end, true);

[Severity: Critical]
When calling mmu_split_huge_pages(), is it possible to encounter a
use-after-free if the page table is concurrently destroyed?

In mmu_split_huge_pages(), kvm->mmu_lock can be dropped:

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

		ret = kvm_riscv_gstage_split_huge(gstage, pcache, addr, 0, flush);
    ...
}

While the lock is dropped, what happens if a concurrent
kvm_mmu_notifier_release() acquires mmu_lock and frees the PGD pages? When
mmu_split_huge_pages() reacquires the lock, it continues using the cached
gstage->pgd pointer to access the now-freed page table memory. Should the
KVM PGD be re-validated after re-acquiring the lock?

[Severity: High]
Does this code skip splitting PMDs when the address range is unaligned?

Looking at the loop inside mmu_split_huge_pages():

arch/riscv/kvm/mmu.c:mmu_split_huge_pages() {
    ...
	phys_addr_t addr = start;
    ...
	while (addr < end) {
        ...
		addr += PMD_SIZE;
	}
}

If the starting address is not aligned to PMD_SIZE, addr retains this
unaligned offset. If the range [start, end) spans across a PMD_SIZE
boundary, adding PMD_SIZE to the unaligned addr could make it strictly
greater than or equal to end prematurely. Will this cause the loop to
terminate early and leave the last huge page unsplit?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603175256408L0jnqGs1cJGc0ijCdujci@zte.com.cn?part=3