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

[sashiko-bot@kernel.org]

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.
--

--- 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.

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