Re: [PATCH 2/4] KVM: x86/mmu: Harden "map private PFN" against unexpected root invalidation

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

New issues:
- [High] Moving `kvm_mmu_reload()` inside the retry loop after `mmu_topup_memory_caches()` causes deterministic memory cache underflows on the first page fault if roots are reloaded.
- [Medium] Failure to re-reload the MMU upon detecting an obsolete root.

Pre-existing issues:
- [High] Memory cache depletion during the `RET_PF_RETRY` loop due to missing `mmu_topup_memory_caches()` inside the loop.
--

--- Patch [2]: [PATCH 2/4] KVM: x86/mmu: Harden "map private PFN" against unexpected root invalidation ---
commit 22d402057a7ea26b753baa57f8845602fe5a1b81
Author: Sean Christopherson <[email protected]>

KVM: x86/mmu: Harden "map private PFN" against unexpected root invalidation

This commit moves the MMU reload inside the tight loop in
kvm_tdp_mmu_map_private_pfn() to better handle unexpected root invalidations.
It also adds a warning if the root is invalidated after the initial reload.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 621b0a42f2a14..c6cac893cbad8 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c

[ ... ]

> @@ -5184,10 +5184,6 @@ int kvm_tdp_mmu_map_private_pfn(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
>  	if (kvm_gfn_is_write_tracked(kvm, fault.slot, fault.gfn))
>  		return -EPERM;
>  
> -	r = kvm_mmu_reload(vcpu);
> -	if (r)
> -		return r;
> -
>  	r = mmu_topup_memory_caches(vcpu, false);
>  	if (r)
>  		return r;

[Severity: High]
Since mmu_topup_memory_caches() is outside the loop, but kvm_mmu_reload()
was moved inside, is it possible for the reload to consume cache items
when allocating new roots?

If the root is invalid and kvm_mmu_load() allocates new roots, this
consumes items from mmu_page_header_cache and mmu_shadow_page_cache. When
execution proceeds to kvm_tdp_mmu_map() expecting fully topped up
caches, could this lead to a cache underflow and a host warning or panic?

> @@ -5199,10 +5195,21 @@ int kvm_tdp_mmu_map_private_pfn(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
>  		if (kvm_test_request(KVM_REQ_VM_DEAD, vcpu))
>  			return -EIO;
>  
> +		r = kvm_mmu_reload(vcpu);
> +		if (r)
> +			return r;
> +
>  		cond_resched();
>  
>  		guard(read_lock)(&kvm->mmu_lock);
>  
> +		/*
> +		 * Because slots_lock is held, it should be impossible for roots
> +		 * to be invalidated after the initial MMU reload.  WARN, but
> +		 * continue and re-reload the MMU to try and keep the VM alive.
> +		 */
> +		WARN_ON_ONCE(kvm_test_request(KVM_REQ_MMU_FREE_OBSOLETE_ROOTS, vcpu));
> +

[Severity: Medium]
The comment mentions "continue and re-reload the MMU", but does this actually
fall through directly to kvm_tdp_mmu_map() with an obsolete root?

If the mapping succeeds, it might return RET_PF_FIXED. This would exit the
loop instead of forcing a retry. Should there be a continue statement after
the WARN_ON_ONCE to enforce the retry behavior described in the comment?

>  		r = kvm_tdp_mmu_map(vcpu, &fault);
>  	} while (r == RET_PF_RETRY);
>  

[Severity: High]
This isn't a bug introduced by this patch, but retrying on RET_PF_RETRY
in this loop seems vulnerable to memory cache depletion without the cache topup.

If kvm_tdp_mmu_map() encounters contention, frees its locally allocated shadow
pages, and returns RET_PF_RETRY, the loop repeats without calling
mmu_topup_memory_caches() again. Could this underflow the cache during
repeated retries under high contention?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2