Re: [RFC PATCH v2 12/18] KVM: TDX: Bug the VM if extended the initial measurement fails

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

On Fri, 2025-08-29 at 13:11 -0700, Sean Christopherson wrote:
> > 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.

I could see that. I didn't think of the below.

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

Yea.

> 
> 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.
> 
> 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;
> 
> kvm_arch_vcpu_create() runs without any locks held, and so TDX effectively has
> the same bug that SEV intra-host migration had, where an in-flight vCPU
> creation could race with a VM-wide state transition (see commit ecf371f8b02d
> ("KVM: SVM: Reject SEV{-ES} intra host migration if vCPU creation is in-
> flight").  To fix that, kvm->lock needs to be taken and KVM needs to verify
> there's no in-flight vCPU creation, e.g. so that a vCPU doesn't pop up and
> contend a TDX-Module lock.
> 
> We an even define a fancy new CLASS to handle the lock+check => unlock logic
> with guard()-like syntax:
> 
> 	CLASS(tdx_vm_state_guard, guard)(kvm);
> 	if (IS_ERR(guard))
> 		return PTR_ERR(guard);
> 
> IIUC, with all of those locks, KVM can KVM_BUG_ON() both TDH_MEM_PAGE_ADD and
> TDH_MR_EXTEND, with no exceptions given for -EBUSY.  Attached patches are very
> lightly tested as usual and need to be chunked up, but seem do to what I want.

Ok, the direction seem clear. The patch has an issue, need to debug.