Re: [PATCH] riscv: kvm: Skip preventive hfence.gvma for new G-stage mappings with Svvptc

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `smp_wmb()` before linking newly allocated intermediate page tables, exposing uninitialized memory to concurrent hardware page table walkers.
- [High] Non-atomic write-protection of G-stage PTEs clobbers hardware Access/Dirty bit updates.
--

commit 8c5791358f444706f77027dbaa2cea7abc95b1e0
Author: Dylan.Wu <fredwudi0305@gmail.com>

riscv: kvm: Skip preventive hfence.gvma for new G-stage mappings with Svvptc

For Invalid->Valid G-stage leaf PTE updates, the flush in
kvm_riscv_gstage_set_pte() is preventive. When Svvptc is available, new
valid PTEs become visible to the page-table walker within a bounded
time, so an immediate hfence.gvma is unnecessary. Skip gstage_tlb_flush()
for that case.

> diff --git a/arch/riscv/kvm/gstage.c b/arch/riscv/kvm/gstage.c
> index c4c3b79567f10..7b805112d2829 100644
> --- a/arch/riscv/kvm/gstage.c
> +++ b/arch/riscv/kvm/gstage.c

[ ... ]

> @@ -171,8 +173,11 @@ int kvm_riscv_gstage_set_pte(struct kvm_gstage *gstage,

[Severity: High]
This isn't a bug introduced by this patch, but does
kvm_riscv_gstage_set_pte() need an smp_wmb() before linking a newly
allocated intermediate page table?

When a new page table is allocated during a guest page fault, the setup
looks like this:

	next_ptep = kvm_mmu_memory_cache_alloc(pcache);
	if (!next_ptep)
		return -ENOMEM;
	set_pte(ptep, pfn_pte(PFN_DOWN(__pa(next_ptep)),
			      __pgprot(_PAGE_TABLE)));

Without a memory barrier before set_pte(), the zeroing of the newly
allocated page table could be delayed past its publication in the parent
page table.

If a concurrent hardware page table walker follows the pointer to the new
page table and observes uninitialized memory, could it interpret random old
data as valid PTEs and cause wild memory accesses or guest memory
corruption?

>  	}
>  
>  	if (pte_val(*ptep) != pte_val(map->pte)) {
> +		bool was_invalid = !pte_val(*ptep);
>  		set_pte(ptep, map->pte);
> -		if (gstage_pte_leaf(ptep))
> +		if (gstage_pte_leaf(ptep) &&
> +		    !(was_invalid &&
> +		      riscv_has_extension_unlikely(RISCV_ISA_EXT_SVVPTC)))
>  			gstage_tlb_flush(gstage, current_level, map->addr);
>  	}
>  

[Severity: High]
This is a pre-existing issue, but can the non-atomic write-protection in
kvm_riscv_gstage_op_pte() clobber hardware Access/Dirty bit updates?

During dirty logging, GSTAGE_OP_WP uses a non-atomic read-modify-write
to clear the write bit:

	else if (op == GSTAGE_OP_WP)
		set_pte(ptep, __pte(pte_val(ptep_get(ptep)) & ~_PAGE_WRITE));

If the hardware page-table walker asynchronously updates the Access or
Dirty bit between ptep_get() and set_pte(), would that hardware update be
silently overwritten and lost?

Losing a Dirty bit might cause KVM to treat a modified page as clean
during dirty logging. Could this lead to silent data corruption during
live migration since the modified guest data wouldn't be transferred?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701075239.95542-1-fredwudi0305@gmail.com?part=1