Re: [PATCH v7 11/11] x86/virt/tdx: Optimize tdx_pamt_get/put()

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Sat, 2026-07-18 at 02:00 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Using `atomic_dec_and_lock` in `tdx_pamt_put` lacks underflow protection, causing an unbalanced put (refcount 0) to silently underflow the refcount to -1, which breaks future `tdx_pamt_get` calls for that PFN.

The report is essentially that this patch doesn't handle unbalanced
tdx_pamt_put() calls (a tdx_pamt_put() without a previous paired
tdx_pamt_get()). So this isn't a bug per-se, but it is true that the
optimization patch has a bit worse warnings in the case of buggy calling code.

More analysis... The function is:

int atomic_dec_and_lock(atomic_t *atomic, spinlock_t *lock)
{
	/* Subtract 1 from counter unless that drops it to 0 (ie. it was 1) */
	if (atomic_add_unless(atomic, -1, 1))
		return 0;

	/* Otherwise do it the slow way */
	spin_lock(lock);
	if (atomic_dec_and_test(atomic))
		return 1;
	spin_unlock(lock);
	return 0;
}

In the fast path atomic_add_unless() subtracts 1 unless the counter was exactly
1. So if it was 0 (no paired get has been called), the counter would underflow.

In the non-optimized patch, the put code was more robust to this scenario. In
that code if a put was done with the refcount at 0, the put would see it was
less than 1 and try to remove it. The result would be a warning from the
SEAMCALL failure.

So this patch trades some warnings for performance. But the warning loss is
pretty small. Future buggy code is unlikely to get away without a warning.
Missing PAMT is not like double free or bug like that. With the count
unbalanced, then next tdx_pamt_get() will fail to add the Dynamic PAMT page pair
and the TDX module will return an error when it tries to use the page.

Getting a warning exactly at the point of the unpaired put would be better. But
this version has other clear benefits. On balance, I'd prefer this one. It might
be possible to utilize refcount_t to get some better underflow warnings, but
probably for the cost of more memory ordering things to consider.

Hmm, another cheap, but not complete option would be to just warn on get/put out
paths if any strange refcounts are seen. Since the point is just to catch the
error closer to where it happens. It's just developer help. Simpler code with
less well timed warnings seems like a good tradeoff for developers?

> --
> 
> --- Patch [11]: [PATCH v7 11/11] x86/virt/tdx: Optimize tdx_pamt_get/put() ---
> commit f3bde8ccf9659f8b1e1cedb9b81df0b3b209a9fa
> Author: Kiryl Shutsemau <kas@kernel.org>
> 
> x86/virt/tdx: Optimize tdx_pamt_get/put()
> 
> The Dynamic PAMT get/put helpers use a global spinlock to serialize all
> refcount updates and SEAMCALL invocations. This gives correct behavior for
> concurrent callers, but leads to contention.
> 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 69834b0d3e602..4394bba7c9a7c 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> 
> [ ... ]
> 
> > @@ -2214,17 +2227,13 @@ void tdx_pamt_put(kvm_pfn_t pfn)
> >  
> >  	pamt_refcount = tdx_find_pamt_refcount(pfn);
> >  
> > -	spin_lock(&pamt_lock);
> >  	/*
> >  	 * If there is more than 1 reference on the pamt page, don't
> >  	 * remove it yet. Just decrement the refcount.
> >  	 */
> > -	if (atomic_read(pamt_refcount) > 1) {
> > -		atomic_dec(pamt_refcount);
> > -		goto out_unlock;
> > -	}
> > +	if (!atomic_dec_and_lock(pamt_refcount, &pamt_lock))
> > +		return;
> 
> [Severity: Medium]
> If an unbalanced put occurs on an un-added PAMT page (where the refcount
> is 0), does atomic_dec_and_lock() lack underflow protection and silently
> drop the refcount to -1 instead of proceeding to the SEAMCALL failure
> warning as the previous logic did?
> 
> lib/dec_and_lock.c:atomic_dec_and_lock() {
>     /* Subtract 1 from counter unless that drops it to 0 (ie. it was 1) */
>     if (atomic_add_unless(atomic, -1, 1))
>         return 0;
>     ...
> }
> 
> If the refcount silently becomes -1, would a subsequent call to
> tdx_pamt_get() for this PFN incorrectly succeed without adding the PAMT
> memory?
> 
> arch/x86/virt/vmx/tdx/tdx.c:tdx_pamt_get() {
>     ...
>     if (atomic_inc_not_zero(pamt_refcount))
>         return 0;
>     ...
> }
> 
> Since -1 is not zero, atomic_inc_not_zero() would increment the refcount
> to 0 and return true, bypassing the tdh_phymem_pamt_add() SEAMCALL.
>