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);
