Message-ID: <aLa34QCJCXGLk/fl@yzhao56-desk.sh.intel.com>
Date: Tue, 2 Sep 2025 17:24:49 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Rick P Edgecombe <rick.p.edgecombe@...el.com>, Kai Huang
	<kai.huang@...el.com>, "ackerleytng@...gle.com" <ackerleytng@...gle.com>,
	Vishal Annapurve <vannapurve@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Ira Weiny <ira.weiny@...el.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "michael.roth@....com"
	<michael.roth@....com>, "pbonzini@...hat.com" <pbonzini@...hat.com>
Subject: Re: [RFC PATCH v2 12/18] KVM: TDX: Bug the VM if extended the
 initial measurement fails

On Fri, Aug 29, 2025 at 01:11:35PM -0700, Sean Christopherson wrote:
> On Fri, Aug 29, 2025, Rick P Edgecombe wrote:
> > On Fri, 2025-08-29 at 16:18 +0800, Yan Zhao wrote:
> > > > +	/*
> > > > +	 * Note, MR.EXTEND can fail if the S-EPT mapping is somehow removed
> > > > +	 * between mapping the pfn and now, but slots_lock prevents memslot
> > > > +	 * updates, filemap_invalidate_lock() prevents guest_memfd updates,
> > > > +	 * mmu_notifier events can't reach S-EPT entries, and KVM's
> > > > internal
> > > > +	 * zapping flows are mutually exclusive with S-EPT mappings.
> > > > +	 */
> > > > +	for (i = 0; i < PAGE_SIZE; i += TDX_EXTENDMR_CHUNKSIZE) {
> > > > +		err = tdh_mr_extend(&kvm_tdx->td, gpa + i, &entry,
> > > > &level_state);
> > > > +		if (KVM_BUG_ON(err, kvm)) {
> > > I suspect tdh_mr_extend() running on one vCPU may contend with
> > > tdh_vp_create()/tdh_vp_addcx()/tdh_vp_init*()/tdh_vp_rd()/tdh_vp_wr()/
> > > tdh_mng_rd()/tdh_vp_flush() on other vCPUs, if userspace invokes ioctl
> > > KVM_TDX_INIT_MEM_REGION on one vCPU while initializing other vCPUs.
> > > 
> > > It's similar to the analysis of contention of tdh_mem_page_add() [1], as
> > > both tdh_mr_extend() and tdh_mem_page_add() acquire exclusive lock on
> > > resource TDR.
> > > 
> > > I'll try to write a test to verify it and come back to you.
I've written a selftest and proved the contention between tdh_mr_extend() and
tdh_vp_create().

The KVM_BUG_ON() after tdh_mr_extend() now is not hittable with Sean's newly
provided 2 fixes.


But during writing another concurrency test, I found a sad news :

SEAMCALL TDH_VP_INIT requires to hold exclusive lock for resource TDR when its
leaf_opcode.version > 0. So, when I use v1 (which is the current value in
upstream, for x2apic?) to test executing ioctl KVM_TDX_INIT_VCPU on different
vCPUs concurrently, the TDX_BUG_ON() following tdh_vp_init() will print error
"SEAMCALL TDH_VP_INIT failed: 0x8000020000000080".

If I switch to using v0 version of TDH_VP_INIT, the contention will be gone.

Note: this acquiring of exclusive lock was not previously present in the public
repo https://github.com/intel/tdx-module.git, branch tdx_1.5.
(The branch has been force-updated to new implementation now).


> > I'm seeing the same thing in the TDX module. It could fail because of contention
> > controllable from userspace. So the KVM_BUG_ON() is not appropriate.
> > 
> > Today though if tdh_mr_extend() fails because of contention then the TD is
> > essentially dead anyway. Trying to redo KVM_TDX_INIT_MEM_REGION will fail. The
> > M-EPT fault could be spurious but the second tdh_mem_page_add() would return an
> > error and never get back to the tdh_mr_extend().
> > 
> > The version in this patch can't recover for a different reason. That is 
> > kvm_tdp_mmu_map_private_pfn() doesn't handle spurious faults, so I'd say just
> > drop the KVM_BUG_ON(), and try to handle the contention in a separate effort.
> > 
> > I guess the two approaches could be to make KVM_TDX_INIT_MEM_REGION more robust,
> 
> This.  First and foremost, KVM's ordering and locking rules need to be explicit
> (ideally documented, but at the very least apparent in the code), *especially*
> when the locking (or lack thereof) impacts userspace.  Even if effectively relying
> on the TDX-module to provide ordering "works", it's all but impossible to follow.
> 
> And it doesn't truly work, as everything in the TDX-Module is a trylock, and that
> in turn prevents KVM from asserting success.  Sometimes KVM has better option than
> to rely on hardware to detect failure, but it really should be a last resort,
> because not being able to expect success makes debugging no fun.  Even worse, it
> bleeds hard-to-document, specific ordering requirements into userspace, e.g. in
> this case, it sounds like userspace can't do _anything_ on vCPUs while doing
> KVM_TDX_INIT_MEM_REGION.  Which might not be a burden for userspace, but oof is
> it nasty from an ABI perspective.
> 
> > or prevent the contention. For the latter case:
> > tdh_vp_create()/tdh_vp_addcx()/tdh_vp_init*()/tdh_vp_rd()/tdh_vp_wr()
> > ...I think we could just take slots_lock during KVM_TDX_INIT_VCPU and
> > KVM_TDX_GET_CPUID.
> > 
> > For tdh_vp_flush() the vcpu_load() in kvm_arch_vcpu_ioctl() could be hard to
> > handle.
> > 
> > So I'd think maybe to look towards making KVM_TDX_INIT_MEM_REGION more robust,
> > which would mean the eventual solution wouldn't have ABI concerns by later
> > blocking things that used to be allowed.
> > 
> > Maybe having kvm_tdp_mmu_map_private_pfn() return success for spurious faults is
> > enough. But this is all for a case that userspace isn't expected to actually
> > hit, so seems like something that could be kicked down the road easily.
> 
> You're trying to be too "nice", just smack 'em with a big hammer.  For all intents
> and purposes, the paths in question are fully serialized, there's no reason to try
> and allow anything remotely interesting to happen.
This big hammer looks good to me :)

> 
> Acquire kvm->lock to prevent VM-wide things from happening, slots_lock to prevent
> kvm_mmu_zap_all_fast(), and _all_ vCPU mutexes to prevent vCPUs from interefering.
Nit: we should have no worry to kvm_mmu_zap_all_fast(), since it only zaps
!mirror roots. The slots_lock should be for slots deletion.

> 
> Doing that for a vCPU ioctl is a bit awkward, but not awful.  E.g. we can abuse
> kvm_arch_vcpu_async_ioctl().  In hindsight, a more clever approach would have
> been to make KVM_TDX_INIT_MEM_REGION a VM-scoped ioctl that takes a vCPU fd.  Oh
> well.
> 
> Anyways, I think we need to avoid the "synchronous" ioctl path anyways, because
> taking kvm->slots_lock inside vcpu->mutex is gross.  AFAICT it's not actively
> problematic today, but it feels like a deadlock waiting to happen.
Note: Looks kvm_inhibit_apic_access_page() also takes kvm->slots_lock inside
vcpu->mutex.

 
> The other oddity I see is the handling of kvm_tdx->state.  I don't see how this
> check in tdx_vcpu_create() is safe:
> 
> 	if (kvm_tdx->state != TD_STATE_INITIALIZED)
> 		return -EIO;

Right, if tdh_vp_create() contends with tdh_mr_finalize(), KVM_BUG_ON() will be
triggered.
I previously overlooked the KVM_BUG_ON() after tdh_vp_create(), thinking that
it's ok to have it return error once tdh_vp_create() is invoked after
tdh_mr_finalize().

...
>  int tdx_vcpu_ioctl(struct kvm_vcpu *vcpu, void __user *argp)
>  {
>  	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> @@ -3146,19 +3211,14 @@ int tdx_vcpu_ioctl(struct kvm_vcpu *vcpu, void __user *argp)
>  	if (!is_hkid_assigned(kvm_tdx) || kvm_tdx->state == TD_STATE_RUNNABLE)
>  		return -EINVAL;
>  
> -	if (copy_from_user(&cmd, argp, sizeof(cmd)))
> -		return -EFAULT;
> -
> -	if (cmd.hw_error)
> -		return -EINVAL;
> +	ret = tdx_get_cmd(argp, &cmd);
> +	if (ret)
> +		return ret;
>  
>  	switch (cmd.id) {
>  	case KVM_TDX_INIT_VCPU:
>  		ret = tdx_vcpu_init(vcpu, &cmd);
>  		break;
So, do we need to move KVM_TDX_INIT_VCPU to tdx_vcpu_async_ioctl() as well?

> -	case KVM_TDX_INIT_MEM_REGION:
> -		ret = tdx_vcpu_init_mem_region(vcpu, &cmd);
> -		break;
>  	case KVM_TDX_GET_CPUID:
>  		ret = tdx_vcpu_get_cpuid(vcpu, &cmd);

Powered by blists - more mailing lists