linux-kernel - Re: [RFC PATCH v5 16/45] x86/virt/tdx: Add tdx_alloc/free_control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <655724f8-0098-40ee-a097-ce4c0249933d@intel.com>
Date: Tue, 10 Feb 2026 09:44:16 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Sean Christopherson <seanjc@...gle.com>, Thomas Gleixner
 <tglx@...nel.org>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 x86@...nel.org, Kiryl Shutsemau <kas@...nel.org>,
 Paolo Bonzini <pbonzini@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-coco@...ts.linux.dev,
 kvm@...r.kernel.org, Kai Huang <kai.huang@...el.com>,
 Rick Edgecombe <rick.p.edgecombe@...el.com>, Yan Zhao
 <yan.y.zhao@...el.com>, Vishal Annapurve <vannapurve@...gle.com>,
 Ackerley Tng <ackerleytng@...gle.com>, Sagi Shahar <sagis@...gle.com>,
 Binbin Wu <binbin.wu@...ux.intel.com>, Xiaoyao Li <xiaoyao.li@...el.com>,
 Isaku Yamahata <isaku.yamahata@...el.com>
Subject: Re: [RFC PATCH v5 16/45] x86/virt/tdx: Add
 tdx_alloc/free_control_page() helpers

On 1/28/26 17:14, Sean Christopherson wrote:
> +static void tdx_pamt_put(struct page *page)
> +{
> +	u64 pamt_pa_array[MAX_NR_DPAMT_ARGS];
> +	atomic_t *pamt_refcount;
> +	u64 tdx_status;
> +
> +	if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> +		return;
> +
> +	pamt_refcount = tdx_find_pamt_refcount(page_to_pfn(page));
> +
> +	scoped_guard(spinlock, &pamt_lock) {
> +		/*
> +		 * If the there are more than 1 references on the pamt page,
> +		 * don't remove it yet. Just decrement the refcount.
> +		 */
> +		if (atomic_read(pamt_refcount) > 1) {
> +			atomic_dec(pamt_refcount);
> +			return;
> +		}
> +
> +		/* Try to remove the pamt page and take the refcount 1->0. */
> +		tdx_status = tdh_phymem_pamt_remove(page, pamt_pa_array);
> +
> +		/*
> +		 * Don't free pamt_pa_array as it could hold garbage when
> +		 * tdh_phymem_pamt_remove() fails.  Don't panic/BUG_ON(), as
> +		 * there is no risk of data corruption, but do yell loudly as
> +		 * failure indicates a kernel bug, memory is being leaked, and
> +		 * the dangling PAMT entry may cause future operations to fail.
> +		 */
> +		if (WARN_ON_ONCE(!IS_TDX_SUCCESS(tdx_status)))
> +			return;
> +
> +		atomic_dec(pamt_refcount);
> +	}
> +
> +	/*
> +	 * pamt_pa_array is populated up to tdx_dpamt_entry_pages() by the TDX
> +	 * module with pages, or remains zero inited. free_pamt_array() can
> +	 * handle either case. Just pass it unconditionally.
> +	 */
> +	free_pamt_array(pamt_pa_array);
> +}

This looks funky.

Right now, this is:

	spin_lock(pamt_lock)
	atomic_inc/dec(fine-grained-refcount)
	tdcall_blah_blah()
	spin_unlock(pamt_lock)

Where it *always* acquires the global lock when DPAMT is supported.
Couldn't we optimize it so that it only acquires it when it has to keep
the refcount stable at zero?

Roughly:

	slow_path = atomic_dec_and_lock(fine-grained-refcount,
					pamt_lock)
	if (!slow_path)
		goto out;

	// fine-grained-refcount==0 and must stay that way with
	// pamt_lock held. Remove the DPAMT pages:
	tdh_phymem_pamt_remove(page, pamt_pa_array)
out:	
	spin_unlock(pamt_lock)

On the acquire side, you do:

	fast_path = atomic_inc_not_zero(fine-grained-refcount)
	if (fast_path)
		return;

	// slow path:
	spin_lock(pamt_lock)

	// Was the race lost with another 0=>1 increment?
	if (atomic_read(fine-grained-refcount) > 0)
		goto out_inc

	tdh_phymem_pamt_add(page, pamt_pa_array)
	// Inc after the TDCALL so another thread won't race ahead of us
	// and try to use a non-existent PAMT entry
out_inc:
	atomic_inc(fine-grained-refcount)
	spin_unlock(pamt_lock)

Then, at least only the 0=>1 and 1=>0 transitions need the global lock.
The fast paths only touch the refcount which isn't shared nearly as much
as the global lock.

BTW, this probably still needs to be spin_lock_irq(), not what I wrote
above, but that's not a big deal to add.

I've stared at this for a bit and don't see any holes. Does anyone else
see any?