Re: [PATCH v10 08/11] KVM: TDX: Get/put PAMT pages when (un)mapping private memory

[sashiko-bot@kernel.org]

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

New issues:
- [High] The retry loop in kvm_tdp_mmu_map_private_pfn() fails to top up memory caches, causing PAMT cache exhaustion and a fatal KVM_BUG_ON when S-EPT mapping encounters contention.

Pre-existing issues:
- [High] cpus_read_lock() is acquired while kvm->lock is already held during KVM_TDX_INIT_VM initialization and teardown, violating lock hierarchy.
--

commit bab02cc7c6ddf1559b28e6bffce54a70e3531bc9
Author: Kiryl Shutsemau <kas@kernel.org>

KVM: TDX: Get/put PAMT pages when (un)mapping private memory
    
This patch adds Dynamic PAMT support to KVM's S-EPT MMU by allocating PAMT
pages on demand when adding guest memory and balancing releases via
tdx_pamt_put() when removing it or on error paths.

> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 957078e8656ef..09573112540e9 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
[ ... ]
> @@ -1679,16 +1694,28 @@ static struct page *tdx_spte_to_sept_pt(struct kvm *kvm, gfn_t gfn,
>  static int tdx_sept_map_nonleaf_spte(struct kvm *kvm, gfn_t gfn,
>  				     enum pg_level level, u64 new_spte)
>  {
> +	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
>  	gpa_t gpa = gfn_to_gpa(gfn);
>  	u64 err, entry, level_state;
>  	struct page *sept_pt;
> +	int ret;
> +
> +	if (KVM_BUG_ON(!vcpu, kvm))
> +		return -EIO;
>  
>  	sept_pt = tdx_spte_to_sept_pt(kvm, gfn, new_spte, level);
>  	if (!sept_pt)
>  		return -EIO;
>  
> +	ret = tdx_pamt_get(page_to_pfn(sept_pt), &to_tdx(vcpu)->pamt_cache);
> +	if (KVM_BUG_ON(ret, kvm))
> +		return ret;

[Severity: High]
Does this error path lead to PAMT cache exhaustion and a fatal VM BUG if
mapping encounters contention?

When tdh_mem_sept_add() below encounters lock contention, which can occur
during parallel setups, it returns TDX_OPERAND_BUSY (-EBUSY/RET_PF_RETRY).

The error path calls tdx_pamt_put(), which drops the refcount and calls
free_pamt_array(). Crucially, free_pamt_array() returns the memory to the
kernel buddy allocator instead of returning it to the vCPU cache:

arch/x86/virt/vmx/tdx/tdx.c:tdx_pamt_put() {
	...
	atomic_set(dpamt_refcount, 0);
	spin_unlock(&dpamt_lock);
	free_pamt_array(pamt_pages);
}

The failure propagates as RET_PF_RETRY, and the page fault loop in
kvm_tdp_mmu_map_private_pfn() retries without calling
mmu_topup_memory_caches():

arch/x86/kvm/mmu/mmu.c:kvm_tdp_mmu_map_private_pfn() {
	...
		r = kvm_tdp_mmu_map(vcpu, &fault);
	} while (r == RET_PF_RETRY);
}

This causes a net loss of cache entries. When the unreplenished cache runs
out of memory, tdx_pamt_get() returns -ENOMEM, causing KVM to BUG the VM
here. Could tdx_pamt_put() restore pages to the cache, or could the
retry loop be updated to replenish caches?

> +
>  	err = tdh_mem_sept_add(&to_kvm_tdx(kvm)->td, gpa, level, sept_pt,
>  			       &entry, &level_state);
> +	if (err)
> +		tdx_pamt_put(page_to_pfn(sept_pt));
> +
>  	if (unlikely(tdx_operand_busy(err)))
>  		return -EBUSY;
[ ... ]

[Severity: High]
This is a pre-existing issue, but does acquiring cpus_read_lock() while
kvm->lock is already held violate the KVM lock hierarchy?

During KVM_TDX_INIT_VM initialization, kvm->lock is acquired via the state
guard before handling the init ioctl:

arch/x86/kvm/vmx/tdx.c:tdx_vm_ioctl() {
	CLASS(tdx_vm_state_guard, guard)(kvm);
	if (IS_ERR(guard))
		return PTR_ERR(guard);

	switch (tdx_cmd.id) {
	case KVM_TDX_INIT_VM:
		r = tdx_td_init(kvm, &tdx_cmd);
	...
}

Then cpus_read_lock() is acquired inside __tdx_td_init() while kvm->lock
is held:

arch/x86/kvm/vmx/tdx.c:__tdx_td_init() {
	...
	if (!zalloc_cpumask_var(&packages, GFP_KERNEL))
		goto free_tdcs;

	cpus_read_lock();
	...
}

Similarly, it is acquired on the teardown path in tdx_mmu_release_hkid()
while kvm->lock is held:

arch/x86/kvm/vmx/tdx.c:tdx_mmu_release_hkid() {
	packages_allocated = zalloc_cpumask_var(&packages, GFP_KERNEL);
	targets_allocated = zalloc_cpumask_var(&targets, GFP_KERNEL);
	cpus_read_lock();
	...
}

The KVM guidelines explicitly state that cpus_read_lock() must be the
outermost lock. Could this cause an ABBA deadlock against other paths that
acquire cpus_read_lock() before taking kvm->lock, resulting in system hangs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903015113.93343-1-rick.p.edgecombe@intel.com?part=8