| 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
| ||
|
Message-ID: <aYmoIaFwgR6+hnGp@yzhao56-desk.sh.intel.com>
Date: Mon, 9 Feb 2026 17:25:53 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: 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>, <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>, "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 20/45] KVM: x86/mmu: Allocate/free S-EPT pages
using tdx_{alloc,free}_control_page()
On Fri, Feb 06, 2026 at 07:01:14AM -0800, Sean Christopherson wrote:
> > (1) alloc
> > tdx_alloc_control_page
> > __tdx_alloc_control_page
> > __tdx_pamt_get
> > spin_lock(&pamt_lock) ==> under process context
> > spin_unlock(&pamt_lock)
> >
> > (2) free
> > tdp_mmu_free_sp_rcu_callback
> > tdp_mmu_free_sp
> > kvm_x86_call(free_external_sp)
> > tdx_free_control_page
> > __tdx_free_control_page
> > __tdx_pamt_put
> > spin_lock(&pamt_lock) ==> under softirq context
> > spin_unlock(&pamt_lock)
> >
> > So, invoking __tdx_pamt_put() in the RCU callback triggers deadlock warning
> > (see the bottom for details).
>
> Hrm. I can think of two options. Option #1 would be to use a raw spinlock and
> disable IRQs:
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 823ec092b4e4..6348085d7dcb 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -2246,7 +2246,7 @@ static u64 tdh_phymem_pamt_remove(u64 pfn, u64 *pamt_pa_array)
> }
>
> /* Serializes adding/removing PAMT memory */
> -static DEFINE_SPINLOCK(pamt_lock);
> +static DEFINE_RAW_SPINLOCK(pamt_lock);
>
> /* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> int __tdx_pamt_get(u64 pfn, struct tdx_pamt_cache *cache)
> @@ -2272,7 +2272,7 @@ int __tdx_pamt_get(u64 pfn, struct tdx_pamt_cache *cache)
> if (ret)
> goto out_free;
>
> - scoped_guard(spinlock, &pamt_lock) {
> + scoped_guard(raw_spinlock_irqsave, &pamt_lock) {
> /*
> * Lost race to other tdx_pamt_add(). Other task has already allocated
> * PAMT memory for the HPA.
> @@ -2348,7 +2348,7 @@ void __tdx_pamt_put(u64 pfn)
> if (!atomic_dec_and_test(pamt_refcount))
> return;
>
> - scoped_guard(spinlock, &pamt_lock) {
> + scoped_guard(raw_spinlock_irqsave, &pamt_lock) {
> /* Lost race with tdx_pamt_get(). */
> if (atomic_read(pamt_refcount))
> return;
This option can get rid of the warning.
However, given the pamt_lock is a global lock, which may be acquired even in the
softirq context, not sure if this irq disabled version is good.
For your reference, I measured some test data by concurrently launching and
destroying 4 TDs for 3 rounds:
t0 ---------------------
scoped_guard(spinlock, &pamt_lock) { |->T1=t1-t0 |
t1 ---------- |
... |
t2 ---------- |->T3=t4-t0
tdh_phymem_pamt_add/remove() |->T2=t3-t2 |
t3 ---------- |
... |
t4 ---------------------
}
(1) for __tdx_pamt_get()
avg us min us max us
------|---------------------------
T1 | 4 0 69
T2 | 4 2 18
T3 | 10 3 83
(2) for__tdx_pamt_put()
avg us min us max us
------|---------------------------
T1 | 0 0 5
T2 | 2 1 11
T3 | 3 2 15
> Option #2 would be to immediately free the page in tdx_sept_reclaim_private_sp(),
> so that pages that freed via handle_removed_pt() don't defer freeing the S-EPT
> page table (which, IIUC, is safe since the TDX-Module forces TLB flushes and exits).
>
> I really, really don't like this option (if it even works).
I don't like its asymmetry with tdx_sept_link_private_spt().
However, do you think it would be good to have the PAMT pages of the sept pages
allocated from (*topup_private_mapping_cache) [1]?
private_mapping could also include non-leaf mappings.
So, we could invoke tdx_pamt_get() in tdx_sept_link_private_spt() for symmetry.
[1] https://lore.kernel.org/all/aYZ2qft-akOYwkOk@google.com/
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index ae7b9beb3249..4726011ad624 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -2014,7 +2014,15 @@ static void tdx_sept_reclaim_private_sp(struct kvm *kvm, gfn_t gfn,
> */
> if (KVM_BUG_ON(is_hkid_assigned(to_kvm_tdx(kvm)), kvm) ||
> tdx_reclaim_page(virt_to_page(sp->external_spt)))
> - sp->external_spt = NULL;
> + goto out;
> +
> + /*
> + * Immediately free the control page, as the TDX subsystem doesn't
> + * support freeing pages from RCU callbacks.
> + */
> + tdx_free_control_page((unsigned long)sp->external_spt);
> +out:
> + sp->external_spt = NULL;
> }
>
> void tdx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
> --
Powered by blists - more mailing lists