Re: [PATCH v2 2/4] RISC-V: KVM: Split huge pages when dirty logging is enabled

[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 without flushing remote TLBs after modifying page tables.

Pre-existing issues:
- [High] Missing memory barrier before publishing newly allocated stage-2 child page tables.
--

commit 0208ccce76fbb3c703dc5faa9c4873b1be06a0ce
Author: Wang Yechao <wang.yechao255@zte.com.cn>

RISC-V: KVM: Split huge pages when dirty logging is enabled

Split huge pages eagerly when enabling dirty logging. The goal is to
avoid doing it while faulting on write-protected pages, which
negatively impacts guest performance.

> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index 0676937bd9a1c..51bd1db74e1aa 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c
> @@ -90,6 +90,52 @@ void kvm_riscv_mmu_iounmap(struct kvm *kvm, gpa_t gpa, unsigned long size)
[ ... ]
> +/* the caller must held mmu lock */
> +static void mmu_split_huge_pages(struct kvm_gstage *gstage,
> +				 phys_addr_t start, phys_addr_t end,
> +				 bool flush)
> +{
> +	struct kvm *kvm = gstage->kvm;
> +	struct kvm_mmu_memory_cache *pcache = &kvm->arch.pgd_split_page_cache;
> +	int count = gstage->pgd_levels;
> +	phys_addr_t addr = start;
> +	int ret;
> +
> +	lockdep_assert_held(&kvm->mmu_lock);
> +
> +	while (addr < end) {
> +		if (need_topup_split_caches_or_resched(pcache, count)) {
> +			spin_unlock(&kvm->mmu_lock);
> +			cond_resched();

[Severity: High]
Does this drop the MMU lock while there are unflushed page table
modifications?

Because mmu_split_huge_pages() passes flush = false to
kvm_riscv_gstage_split_huge(), TLB flushes are deferred until
mmu_split_memory_region() completes. If cache top-up or rescheduling
is needed, the kvm->mmu_lock is dropped here without flushing the
remote TLBs for the pages that were split in previous loop iterations.

Could this allow other vCPUs to observe inconsistent stage-2 page
tables or execute with stale TLB mappings while the lock is dropped?
Perhaps a remote TLB flush should be issued for the already-processed
range before unlocking?

> +
> +			ret = kvm_mmu_topup_memory_cache(pcache, count);
> +			if (ret) {
> +				kvm_err("Failed to toup split page cache\n");
> +				spin_lock(&kvm->mmu_lock);
> +				return;
> +			}
> +			spin_lock(&kvm->mmu_lock);
> +		}
> +
> +		ret = kvm_riscv_gstage_split_huge(gstage, pcache, addr, 0, flush);

[Severity: High]
This is a pre-existing issue, but looking at kvm_riscv_gstage_split_huge()
which is called here, it appears to lack a memory barrier before publishing
the newly allocated stage-2 child page tables:

arch/riscv/kvm/gstage.c:kvm_riscv_gstage_split_huge() {
...
		for (i = 0; i < PTRS_PER_PTE; i++) {
			child_pte = make_child_pte(huge_pte, i, child_page_size);
			set_pte((pte_t *)&next_ptep[i], __pte(child_pte));
		}

		set_pte(ptep, pfn_pte(PFN_DOWN(__pa(next_ptep)),
				__pgprot(_PAGE_TABLE)));
...
}

Because RISC-V set_pte() translates to a simple WRITE_ONCE without an
implicit memory barrier, could the stores initializing the child PTEs be
reordered after the store that publishes the page table to ptep?

If so, a concurrent hardware page table walker traversing the newly
published ptep might read uninitialized or stale data from the child
page table, resulting in spurious stage-2 page faults. Would it make
sense to insert an smp_wmb() before publishing the new page table?

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