Re: [PATCH] KVM: riscv: Only fast-path dirty-log write faults on host-writable leaves

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] KVM: riscv: missing host page dirtying in kvm_riscv_mmu_map() on read fault
--

commit 62242a1df73a6632040ee4bbfc1be21570c1f2af
Author: SeungJu Cheon <suunj1331@gmail.com>

KVM: riscv: Only fast-path dirty-log write faults on host-writable leaves

This commit records host writability in RSW bit 9 (KVM_GSTAGE_PTE_HOST_WRITABLE)
on G-stage leaf PTEs before dirty logging forces the mapping read-only, and
checks this bit in kvm_riscv_mmu_dirty_log_write_fault_fast() so that
read-only host mappings fall back to the slow path.

> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index 371fccaf9df08..bbc8ab632c8a1 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c
> @@ -586,6 +586,12 @@ static bool kvm_riscv_mmu_dirty_log_write_fault_fast(struct kvm *kvm,
>  			break;
>  		}
>  
> +		/* Let the slow path handle host-side write protection and CoW. */
> +		if (!(pte_val(old_pte) & KVM_GSTAGE_PTE_HOST_WRITABLE)) {
> +			ret = false;
> +			break;
> +		}
> +
>  		if (!dirty_marked) {
>  			mark_page_dirty_in_slot(kvm, memslot, gfn);
>  			dirty_marked = true;
> @@ -629,6 +635,7 @@ int kvm_riscv_mmu_map(struct kvm_vcpu *vcpu, struct kvm_memory_slot *memslot,
>  	bool is_hugetlb;
>  	bool unused = false;
>  	bool writable;
> +	bool host_writable;
>  	unsigned int vma_pageshift;
>  	gfn_t gfn = gpa >> PAGE_SHIFT;
>  	struct vm_area_struct *vma;
> @@ -713,6 +720,8 @@ int kvm_riscv_mmu_map(struct kvm_vcpu *vcpu, struct kvm_memory_slot *memslot,
>  	if (is_error_noslot_pfn(hfn))
>  		return -EFAULT;
>  
> +	host_writable = writable;
> +
>  	/*
>  	 * If logging is active then we allow writable pages only
>  	 * for write faults.
> @@ -738,10 +747,12 @@ int kvm_riscv_mmu_map(struct kvm_vcpu *vcpu, struct kvm_memory_slot *memslot,
>  	if (writable) {
>  		mark_page_dirty_in_slot(kvm, memslot, gfn);
>  		ret = kvm_riscv_gstage_map_page(&gstage, pcache, gpa, hfn << PAGE_SHIFT,
> -						vma_pagesize, false, true, out_map);
> +						vma_pagesize, false, true, host_writable,
> +						out_map);
>  	} else {
>  		ret = kvm_riscv_gstage_map_page(&gstage, pcache, gpa, hfn << PAGE_SHIFT,
> -						vma_pagesize, true, true, out_map);
> +						vma_pagesize, true, true, host_writable,
> +						out_map);
>  	}
>  
>  	if (ret)

[Severity: Critical]
Should the out_unlock path in kvm_riscv_mmu_map() also be updated to pass
host_writable instead of writable to kvm_release_faultin_page()?

Currently, the end of kvm_riscv_mmu_map() does:

out_unlock:
	kvm_release_faultin_page(kvm, page,
				 unused || (ret && ret != -EEXIST), writable);

When dirty logging is enabled on a writable memslot (logging == true) and the
guest triggers a read fault (is_write == false) on a page whose primary MMU
PTE already has write permission (such as a clean MAP_SHARED shmem/memfd folio
faulted in for read, or an anonymous folio after MADV_FREE),
__kvm_faultin_pfn() sees pte_write() via get_user_page_fast_only() and sets
writable = true without marking the host PTE or folio dirty.

kvm_riscv_mmu_map() then records host_writable = true, sets writable = false
because (logging && !is_write), and installs a read-only G-stage PTE with
KVM_GSTAGE_PTE_HOST_WRITABLE set. Because writable is false at out_unlock,
kvm_release_faultin_page() calls kvm_release_page_clean(page) instead of
kvm_release_page_dirty(page).

If the guest subsequently writes to that GPA,
kvm_riscv_mmu_dirty_log_write_fault_fast() sees KVM_GSTAGE_PTE_HOST_WRITABLE
and makes the G-stage PTE writable under read_lock(&kvm->mmu_lock) without
marking the backing host folio dirty:

include/linux/kvm_host.h:kvm_release_faultin_page()
	/*
	 * If the page that KVM got from the *primary MMU* is writable, and KVM
	 * installed or reused a SPTE, mark the page/folio dirty.  Note, this
	 * may mark a folio dirty even if KVM created a read-only SPTE, e.g. if
	 * the GFN is write-protected.  Folios can't be safely marked dirty
	 * outside of mmu_lock as doing so could race with writeback on the
	 * folio.  As a result, KVM can't mark folios dirty in the fast page
	 * fault handler, and so KVM must (somewhat) speculatively mark the
	 * folio dirty if KVM could locklessly make the SPTE writable.
	 */

Since G-stage unmapping only clears the PTE without propagating dirty state to
the host struct page, can host page reclaim later evict the guest-modified
folio as clean without writeback or swap-out, losing the guest's written data?

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