[PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Dmytro Maluka]

vCPU creation in kvm_vm_ioctl_create_vcpu() may fail after
kvm_arch_vcpu_create() -> vmx_vcpu_create() already succeeded. In such
case kvm_vm_ioctl_create_vcpu() destroys the newly created vCPU in the
failure path. However, that leaves a side effect: the IPIv pid_table
entry remains configured with this vCPU's pi_desc address. As a result,
when another vCPU sends an IPI to the APIC ID of this failed-to-create
vCPU, it will cause HW to write to this (freed!) pi_desc memory. [*]

An easy fix would be to clear the pid_table entry in vmx_vcpu_free().
However that would be still problematic, for the following reason:
userspace may try to create a vCPU with the same vcpu_id as an existing
one; vmx_vcpu_create() will succeed, and only after that
kvm_vm_ioctl_create_vcpu() will check for the duplicate vcpu_id and
fail with -EEXIST and then free the vCPU in the failure path. So in this
failure path, vmx_vcpu_free() would clear the pid_table entry for that
already existing good vCPU, i.e. effectively disable IPIv for that vCPU.

So instead fix the issue by moving the pid_table entry setup from
.vcpu_create() to the newly introduced .vcpu_postcreate() callback
called at the end of kvm_vm_ioctl_create_vcpu(), when we are sure that
the vCPU creation succeeded.

[*] Although, since this memory is freed into the kvm_vcpu_cache kmem
    cache which is only used for allocating kvm_vcpus, _maybe_ this
    memory will only be reused for pi_desc of another vCPU, not for
    anything else. So _maybe_ this will only result in delivering the
    IPI to a wrong vCPU (possibly of another VM) in the worst case, not
    in a random corruption of kernel memory.

Fixes: d588bb9be1da ("KVM: VMX: enable IPI virtualization")
Signed-off-by: Dmytro Maluka <dmaluka@chromium.org>
---
 arch/x86/include/asm/kvm-x86-ops.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  1 +
 arch/x86/kvm/vmx/main.c            |  9 +++++++++
 arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
 arch/x86/kvm/vmx/x86_ops.h         |  1 +
 arch/x86/kvm/x86.c                 |  1 +
 6 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 83dc5086138b..aecbe9c54004 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -25,6 +25,7 @@ KVM_X86_OP_OPTIONAL(vm_destroy)
 KVM_X86_OP_OPTIONAL(vm_pre_destroy)
 KVM_X86_OP_OPTIONAL_RET0(vcpu_precreate)
 KVM_X86_OP(vcpu_create)
+KVM_X86_OP_OPTIONAL(vcpu_postcreate)
 KVM_X86_OP(vcpu_free)
 KVM_X86_OP(vcpu_reset)
 KVM_X86_OP(prepare_switch_to_guest)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5f6c1ce9673b..3ed9d8fc3b96 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1803,6 +1803,7 @@ struct kvm_x86_ops {
 	/* Create, but do not attach this VCPU */
 	int (*vcpu_precreate)(struct kvm *kvm);
 	int (*vcpu_create)(struct kvm_vcpu *vcpu);
+	void (*vcpu_postcreate)(struct kvm_vcpu *vcpu);
 	void (*vcpu_free)(struct kvm_vcpu *vcpu);
 	void (*vcpu_reset)(struct kvm_vcpu *vcpu, bool init_event);
 
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 83d9921277ea..bdf5ba3155f6 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -78,6 +78,14 @@ static int vt_vcpu_create(struct kvm_vcpu *vcpu)
 	return vmx_vcpu_create(vcpu);
 }
 
+static void vt_vcpu_postcreate(struct kvm_vcpu *vcpu)
+{
+	if (is_td_vcpu(vcpu))
+		return;
+
+	vmx_vcpu_postcreate(vcpu);
+}
+
 static void vt_vcpu_free(struct kvm_vcpu *vcpu)
 {
 	if (is_td_vcpu(vcpu)) {
@@ -898,6 +906,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
 
 	.vcpu_precreate = vt_op(vcpu_precreate),
 	.vcpu_create = vt_op(vcpu_create),
+	.vcpu_postcreate = vt_op(vcpu_postcreate),
 	.vcpu_free = vt_op(vcpu_free),
 	.vcpu_reset = vt_op(vcpu_reset),
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index cc75feec05da..ae6cef9c13e0 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7752,10 +7752,6 @@ int vmx_vcpu_create(struct kvm_vcpu *vcpu)
 		vmx->ve_info = page_to_virt(page);
 	}
 
-	if (vmx_can_use_ipiv(vcpu))
-		WRITE_ONCE(to_kvm_vmx(vcpu->kvm)->pid_table[vcpu->vcpu_id],
-			   __pa(&vmx->vt.pi_desc) | PID_TABLE_ENTRY_VALID);
-
 	return 0;
 
 free_vmcs:
@@ -7767,6 +7763,13 @@ int vmx_vcpu_create(struct kvm_vcpu *vcpu)
 	return err;
 }
 
+void vmx_vcpu_postcreate(struct kvm_vcpu *vcpu)
+{
+	if (vmx_can_use_ipiv(vcpu))
+		WRITE_ONCE(to_kvm_vmx(vcpu->kvm)->pid_table[vcpu->vcpu_id],
+			   __pa(&to_vmx(vcpu)->vt.pi_desc) | PID_TABLE_ENTRY_VALID);
+}
+
 #define L1TF_MSG_SMT "L1TF CPU bug present and SMT on, data leak possible. See CVE-2018-3646 and https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/l1tf.html for details.\n"
 #define L1TF_MSG_L1D "L1TF CPU bug present and virtualization mitigation disabled, data leak possible. See CVE-2018-3646 and https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/l1tf.html for details.\n"
 
diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
index 409858074246..defc2c3a37c8 100644
--- a/arch/x86/kvm/vmx/x86_ops.h
+++ b/arch/x86/kvm/vmx/x86_ops.h
@@ -21,6 +21,7 @@ int vmx_vm_init(struct kvm *kvm);
 void vmx_vm_destroy(struct kvm *kvm);
 int vmx_vcpu_precreate(struct kvm *kvm);
 int vmx_vcpu_create(struct kvm_vcpu *vcpu);
+void vmx_vcpu_postcreate(struct kvm_vcpu *vcpu);
 int vmx_vcpu_pre_run(struct kvm_vcpu *vcpu);
 fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags);
 void vmx_vcpu_free(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index afcac1042947..c9f512a30fec 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12873,6 +12873,7 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 		return;
 	vcpu_load(vcpu);
 	kvm_synchronize_tsc(vcpu, NULL);
+	kvm_x86_call(vcpu_postcreate)(vcpu);
 	vcpu_put(vcpu);
 
 	/* poll control enabled by default */
-- 
2.55.0.141.g00534a21ce-goog

Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Huang, Kai]

> 
> An easy fix would be to clear the pid_table entry in vmx_vcpu_free().
> However that would be still problematic, for the following reason:
> userspace may try to create a vCPU with the same vcpu_id as an existing
> one; vmx_vcpu_create() will succeed, and only after that
> kvm_vm_ioctl_create_vcpu() will check for the duplicate vcpu_id and
> fail with -EEXIST and then free the vCPU in the failure path. So in this
> failure path, vmx_vcpu_free() would clear the pid_table entry for that
> already existing good vCPU, i.e. effectively disable IPIv for that vCPU.

IMHO this seems a bit fragile?  If something similar to pid_table coming up in
the future, we could end up with a similar problem.

The "duplicated vcpu_id check" seems the ones that should happen as early as
possible.  Is it better to move the "duplicated vcpu_id check" earlier before
vmx_vcpu_create(), e.g., even before the kvm_arch_vcpu_precreate()?  In this
case we can do the pid_table cleanup in vmx_vcpu_free() I think.

The downside is this is common KVM code change for all archs, but it doesn't
seem this will bring problem to other archs.

Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Dmytro Maluka]

On Fri, Jul 17, 2026 at 12:59:38PM +0000, Huang, Kai wrote:
> > 
> > An easy fix would be to clear the pid_table entry in vmx_vcpu_free().
> > However that would be still problematic, for the following reason:
> > userspace may try to create a vCPU with the same vcpu_id as an existing
> > one; vmx_vcpu_create() will succeed, and only after that
> > kvm_vm_ioctl_create_vcpu() will check for the duplicate vcpu_id and
> > fail with -EEXIST and then free the vCPU in the failure path. So in this
> > failure path, vmx_vcpu_free() would clear the pid_table entry for that
> > already existing good vCPU, i.e. effectively disable IPIv for that vCPU.
> 
> IMHO this seems a bit fragile?  If something similar to pid_table coming up in
> the future, we could end up with a similar problem.
> 
> The "duplicated vcpu_id check" seems the ones that should happen as early as
> possible.  Is it better to move the "duplicated vcpu_id check" earlier before
> vmx_vcpu_create(), e.g., even before the kvm_arch_vcpu_precreate()?  In this
> case we can do the pid_table cleanup in vmx_vcpu_free() I think.

Unfortunately it is not that simple. As I understand, the reason why the
"duplicated vcpu_id check" is done later is that it needs to be done
atomically together with inserting the vCPU into kvm->vcpu_array after
it, i.e. both the check and the insertion need to be done with kvm->lock
held (rather than releasing kvm->lock and taking it again between the
two operations).

IOW, if we just move the "duplicated vcpu_id check" earlier (before the
first unlock of kvm->lock), we have a race:

1. vCPU A is being created but not installed in kvm->vcpu_array yet.
2. vCPU B with the same vcpu_id is being created. It passes the
   duplicated vcpu_id check, since the check doesn't find vCPU A in
   kvm->vcpu_array.
3. vCPU A is installed in kvm->vcpu_array, vCPU creation succeeds.
4. vCPU B with the same vcpu_id is installed in kvm->vcpu_array, vCPU
   creation succeeds.

And we cannot just move the kvm->vcpu_array insertion earlier (before
the first unlock of kvm->lock), at least because the vCPU is not even
allocated at that point.

I guess we could track the used vcpu_ids separately in another xarray
(or a bitmap) which could be checked and updated by the early
"duplicated vcpu_id check" before the first unlock of kvm->lock. But do
we want to pay the memory price for that?

Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Huang, Kai]

On Fri, 2026-07-17 at 18:20 +0200, Dmytro Maluka wrote:
> On Fri, Jul 17, 2026 at 12:59:38PM +0000, Huang, Kai wrote:
> > > 
> > > An easy fix would be to clear the pid_table entry in vmx_vcpu_free().
> > > However that would be still problematic, for the following reason:
> > > userspace may try to create a vCPU with the same vcpu_id as an existing
> > > one; vmx_vcpu_create() will succeed, and only after that
> > > kvm_vm_ioctl_create_vcpu() will check for the duplicate vcpu_id and
> > > fail with -EEXIST and then free the vCPU in the failure path. So in this
> > > failure path, vmx_vcpu_free() would clear the pid_table entry for that
> > > already existing good vCPU, i.e. effectively disable IPIv for that vCPU.
> > 
> > IMHO this seems a bit fragile?  If something similar to pid_table coming up in
> > the future, we could end up with a similar problem.
> > 
> > The "duplicated vcpu_id check" seems the ones that should happen as early as
> > possible.  Is it better to move the "duplicated vcpu_id check" earlier before
> > vmx_vcpu_create(), e.g., even before the kvm_arch_vcpu_precreate()?  In this
> > case we can do the pid_table cleanup in vmx_vcpu_free() I think.
> 
> Unfortunately it is not that simple. As I understand, the reason why the
> "duplicated vcpu_id check" is done later is that it needs to be done
> atomically together with inserting the vCPU into kvm->vcpu_array after
> it, i.e. both the check and the insertion need to be done with kvm->lock
> held (rather than releasing kvm->lock and taking it again between the
> two operations).
> 
> IOW, if we just move the "duplicated vcpu_id check" earlier (before the
> first unlock of kvm->lock), we have a race:
> 
> 1. vCPU A is being created but not installed in kvm->vcpu_array yet.
> 2. vCPU B with the same vcpu_id is being created. It passes the
>    duplicated vcpu_id check, since the check doesn't find vCPU A in
>    kvm->vcpu_array.
> 3. vCPU A is installed in kvm->vcpu_array, vCPU creation succeeds.
> 4. vCPU B with the same vcpu_id is installed in kvm->vcpu_array, vCPU
>    creation succeeds.
> 
> And we cannot just move the kvm->vcpu_array insertion earlier (before
> the first unlock of kvm->lock), at least because the vCPU is not even
> allocated at that point.

Hmm right, and AFACIT vCPU A needs to actually bump up the kvm->online_vcpus in
order for the vCPU B to detect the vCPU of same vcpu_id has been created because
kvm_get_vcpu_by_id() will only scan online vCPUs.

Thanks for pointing out!

> 
> I guess we could track the used vcpu_ids separately in another xarray
> (or a bitmap) which could be checked and updated by the early
> "duplicated vcpu_id check" before the first unlock of kvm->lock. But do
> we want to pay the memory price for that?

I don't think we should do that, and your approach is much safer:

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Sean Christopherson]

On Fri, Jul 17, 2026, Kai Huang wrote:
> On Fri, 2026-07-17 at 18:20 +0200, Dmytro Maluka wrote:
> > On Fri, Jul 17, 2026 at 12:59:38PM +0000, Huang, Kai wrote:
> > > > 
> > > > An easy fix would be to clear the pid_table entry in vmx_vcpu_free().
> > > > However that would be still problematic, for the following reason:
> > > > userspace may try to create a vCPU with the same vcpu_id as an existing
> > > > one; vmx_vcpu_create() will succeed, and only after that
> > > > kvm_vm_ioctl_create_vcpu() will check for the duplicate vcpu_id and
> > > > fail with -EEXIST and then free the vCPU in the failure path. So in this
> > > > failure path, vmx_vcpu_free() would clear the pid_table entry for that
> > > > already existing good vCPU, i.e. effectively disable IPIv for that vCPU.
> > > 
> > > IMHO this seems a bit fragile?  If something similar to pid_table coming up in
> > > the future, we could end up with a similar problem.
> > > 
> > > The "duplicated vcpu_id check" seems the ones that should happen as early as
> > > possible.  Is it better to move the "duplicated vcpu_id check" earlier before
> > > vmx_vcpu_create(), e.g., even before the kvm_arch_vcpu_precreate()?  In this
> > > case we can do the pid_table cleanup in vmx_vcpu_free() I think.
> > 
> > Unfortunately it is not that simple. As I understand, the reason why the
> > "duplicated vcpu_id check" is done later is that it needs to be done
> > atomically together with inserting the vCPU into kvm->vcpu_array after
> > it, i.e. both the check and the insertion need to be done with kvm->lock
> > held (rather than releasing kvm->lock and taking it again between the
> > two operations).
> > 
> > IOW, if we just move the "duplicated vcpu_id check" earlier (before the
> > first unlock of kvm->lock), we have a race:
> > 
> > 1. vCPU A is being created but not installed in kvm->vcpu_array yet.
> > 2. vCPU B with the same vcpu_id is being created. It passes the
> >    duplicated vcpu_id check, since the check doesn't find vCPU A in
> >    kvm->vcpu_array.
> > 3. vCPU A is installed in kvm->vcpu_array, vCPU creation succeeds.
> > 4. vCPU B with the same vcpu_id is installed in kvm->vcpu_array, vCPU
> >    creation succeeds.
> > 
> > And we cannot just move the kvm->vcpu_array insertion earlier (before
> > the first unlock of kvm->lock), at least because the vCPU is not even
> > allocated at that point.
> 
> Hmm right, and AFACIT vCPU A needs to actually bump up the kvm->online_vcpus in
> order for the vCPU B to detect the vCPU of same vcpu_id has been created because
> kvm_get_vcpu_by_id() will only scan online vCPUs.
> 
> Thanks for pointing out!
> 
> > I guess we could track the used vcpu_ids separately in another xarray
> > (or a bitmap) which could be checked and updated by the early
> > "duplicated vcpu_id check" before the first unlock of kvm->lock. But do
> > we want to pay the memory price for that?

I'm comfortable paying the cost for a bitmap.  If a system can actually support
thousands of vCPU IDs, then burning a few hundred bytes per VM is all but
guaranteed to be a non-issue.  And for smaller systems, e.g. on arm64 where a
VM can have at most 512 vCPU IDs, the bitmap costs a measely 64 bytes.

And if the wastefulness of a persistent bitmap is a concern, we could easly use
an xarray to track only "pending" vCPUs, so that the steady state cost is ~zero.
Actually, given how easy that is (famous last words), unless removing from an
xarray doesn't free memory soon-ish, I'd say go straight to a semi-permanent
xarray to avoid bikeshedding over the cost of the bitmap.

> I don't think we should do that, and your approach is much safer:

I disagree.  I actually arrived at the bitmap solution before reading this.  My
concern isn't so much about IPI virtualization, it's about the lurking danger of
an unverified vcpu_id.  E.g. see Naveen's suggestion in this thread of simply
letting vcpu_load() initialize the per-VM table, which doesn't work because x86
calls vcpu_load() as part of vCPU creation.  I wouldn't be all that surprised if
there's another bug or two in KVM where colliding 

Ha!  Case in point, s390 had what is effectively the *exact* same bug and fixed
it in the *exact* same way (hooking vcpu_postcreate()) over a decade ago in
commit 255088244929 ("KVM: s390: fix SCA related races and double use"), and that
obviously did nothing to help x86 from repeating the same mistake.

In other words, I want to fix this entire class of bugs, not play a game of
whack-a-mole with bugs that humans are all but guaranteed to overlook.

My other concern with the proposed change is that the vCPU becomes reachable
before long before kvm_arch_vcpu_postcreate().  Which should be fine" for IPI
virtualization, but sets a precedence I'd rather not exist, because initializing
vCPU state _after_ it's reachable is rarely correct.  E.g. msr_kvm_poll_control
is initialized in postcreate for some reason, and that's technically buggy
because it's possible, albeit extremely unlikely, that MSR_KVM_POLL_CONTROL could
be written by userspace before postcreate() runs.

E.g. for the xarray approacy (sketch only, completely untested):

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index fdbd697d0337..e34ca5920393 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -791,6 +791,7 @@ struct kvm {
        /* The current active memslot set for each address space */
        struct kvm_memslots __rcu *memslots[KVM_MAX_NR_ADDRESS_SPACES];
        struct xarray vcpu_array;
+       struct xarray pending_vcpu_ids;
        /*
         * Protected by slots_lock, but can be read outside if an
         * incorrect answer is acceptable.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2df8ee9ecf6c..8bc4f7ffd76d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4179,6 +4179,12 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
                return r;
        }
 
+       r = xa_insert(&kvm->pending_vcpu_ids, id, xa_mk_value(id), GFP_KERNEL);
+       if (r) {
+               mutex_unlock(&kvm->lock);
+               return r;
+       }
+
        kvm->created_vcpus++;
        mutex_unlock(&kvm->lock);
 
@@ -4249,6 +4255,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
         */
        smp_wmb();
        atomic_inc(&kvm->online_vcpus);
+       xa_erase(&kvm->pending_vcpu_ids, id);
        mutex_unlock(&vcpu->mutex);
 
        mutex_unlock(&kvm->lock);
@@ -4273,6 +4280,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
 vcpu_decrement:
        mutex_lock(&kvm->lock);
        kvm->created_vcpus--;
+       xa_erase(&kvm->pending_vcpu_ids, id);
        mutex_unlock(&kvm->lock);
        return r;
 }


or if that doesn't work, the more naive bitmap approach:

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index fdbd697d0337..eadde580e27f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -791,6 +791,8 @@ struct kvm {
        /* The current active memslot set for each address space */
        struct kvm_memslots __rcu *memslots[KVM_MAX_NR_ADDRESS_SPACES];
        struct xarray vcpu_array;
+       DECLARE_BITMAP(vcpu_ids, KVM_MAX_VCPU_IDS);
+
        /*
         * Protected by slots_lock, but can be read outside if an
         * incorrect answer is acceptable.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2df8ee9ecf6c..c735698e76cf 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4179,6 +4179,11 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
                return r;
        }
 
+       if (__test_and_set_bit(id, kvm->vcpu_ids)) {
+               mutex_unlock(&kvm->lock);
+               return -EEXIST;
+       }
+
        kvm->created_vcpus++;
        mutex_unlock(&kvm->lock);
 
@@ -4213,7 +4218,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
 
        mutex_lock(&kvm->lock);
 
-       if (kvm_get_vcpu_by_id(kvm, id)) {
+       if (WARN_ON_ONCE(kvm_get_vcpu_by_id(kvm, id))) {
                r = -EEXIST;
                goto unlock_vcpu_destroy;
        }
@@ -4273,6 +4278,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
 vcpu_decrement:
        mutex_lock(&kvm->lock);
        kvm->created_vcpus--;
+       __clear_bit(id, kvm->vcpu_ids);
        mutex_unlock(&kvm->lock);
        return r;
 }

Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Huang, Kai]

> 
> In other words, I want to fix this entire class of bugs, not play a game of
> whack-a-mole with bugs that humans are all but guaranteed to overlook.

I was thinking yet another xarray could make the overall logic more complicated,
but yeah, agreed that rejecting the duplicated vcpu_id asap is the right
solution to avoid such bugs (my first glance too in the first reply).

> 
> My other concern with the proposed change is that the vCPU becomes reachable
> before long before kvm_arch_vcpu_postcreate().  Which should be fine" for IPI
> virtualization, but sets a precedence I'd rather not exist, because initializing
> vCPU state _after_ it's reachable is rarely correct.  E.g. msr_kvm_poll_control
> is initialized in postcreate for some reason, and that's technically buggy
> because it's possible, albeit extremely unlikely, that MSR_KVM_POLL_CONTROL could
> be written by userspace before postcreate() runs.

Fair enough.  This didn't ring a bell to me.  Thanks for mentioning it.

> 
> E.g. for the xarray approacy (sketch only, completely untested):
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index fdbd697d0337..e34ca5920393 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -791,6 +791,7 @@ struct kvm {
>         /* The current active memslot set for each address space */
>         struct kvm_memslots __rcu *memslots[KVM_MAX_NR_ADDRESS_SPACES];
>         struct xarray vcpu_array;
> +       struct xarray pending_vcpu_ids;
>         /*
>          * Protected by slots_lock, but can be read outside if an
>          * incorrect answer is acceptable.
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2df8ee9ecf6c..8bc4f7ffd76d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4179,6 +4179,12 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>                 return r;
>         }
>  
> +       r = xa_insert(&kvm->pending_vcpu_ids, id, xa_mk_value(id), GFP_KERNEL);
> +       if (r) {
> +               mutex_unlock(&kvm->lock);
> +               return r;
> +       }
> +

(No preference between xarray vs bitmap)

Should we move this before kvm_arch_vcpu_precreate() to avoid any "precreate"
for duplicated vcpu_id?

Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Sean Christopherson]

On Mon, Jul 20, 2026, Kai Huang wrote:
> > E.g. for the xarray approacy (sketch only, completely untested):
> > 
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index fdbd697d0337..e34ca5920393 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -791,6 +791,7 @@ struct kvm {
> >         /* The current active memslot set for each address space */
> >         struct kvm_memslots __rcu *memslots[KVM_MAX_NR_ADDRESS_SPACES];
> >         struct xarray vcpu_array;
> > +       struct xarray pending_vcpu_ids;
> >         /*
> >          * Protected by slots_lock, but can be read outside if an
> >          * incorrect answer is acceptable.
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 2df8ee9ecf6c..8bc4f7ffd76d 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -4179,6 +4179,12 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
> >                 return r;
> >         }
> >  
> > +       r = xa_insert(&kvm->pending_vcpu_ids, id, xa_mk_value(id), GFP_KERNEL);
> > +       if (r) {
> > +               mutex_unlock(&kvm->lock);
> > +               return r;
> > +       }
> > +
> 
> (No preference between xarray vs bitmap)
> 
> Should we move this before kvm_arch_vcpu_precreate() to avoid any "precreate"
> for duplicated vcpu_id?

Hmm, I'm not sure.  I'm leaning "yes".  At a glance, I like the idea.  My only
concern is that we could consume a garbage @id if kvm_arch_vcpu_precreate()
performs additional checks on @id, as x86's implementation does.  However, I
don't think that's a meaningful concern since kvm_vm_ioctl_create_vcpu() *must*
perform at least basic sanity checks.

So yeah, I think moving the check before precreate() makes sense.

Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Huang, Kai]

On Mon, 2026-07-20 at 16:15 -0700, Sean Christopherson wrote:
> My only
> concern is that we could consume a garbage @id if kvm_arch_vcpu_precreate()
> performs additional checks on @id, as x86's implementation does.  However, I
> don't think that's a meaningful concern since kvm_vm_ioctl_create_vcpu() *must*
> perform at least basic sanity checks.

Agreed.

Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Dmytro Maluka]

On Mon, Jul 20, 2026 at 03:17:39PM -0700, Sean Christopherson wrote:
> On Fri, Jul 17, 2026, Kai Huang wrote:
> > On Fri, 2026-07-17 at 18:20 +0200, Dmytro Maluka wrote:
> > > I guess we could track the used vcpu_ids separately in another xarray
> > > (or a bitmap) which could be checked and updated by the early
> > > "duplicated vcpu_id check" before the first unlock of kvm->lock. But do
> > > we want to pay the memory price for that?
> 
> I'm comfortable paying the cost for a bitmap.  If a system can actually support
> thousands of vCPU IDs, then burning a few hundred bytes per VM is all but
> guaranteed to be a non-issue.  And for smaller systems, e.g. on arm64 where a
> VM can have at most 512 vCPU IDs, the bitmap costs a measely 64 bytes.

I was just a bit concerned that, for example, on my laptop, even though
there is only a dozen of physical CPUs, there is still
CONFIG_KVM_MAX_NR_VCPUS=4096 in my Debian's default kernel config, and
KVM_VCPU_ID_RATIO is still hardcoded to 4, so the bitmap size would be
4096 * 4 / 8 = 2KB which just feels "disproportionate" for such a corner
case as this sanity check.

But yeah, that doesn't seem to be a real problem (it's not like we
routinely run thousands of VMs on such systems).

> And if the wastefulness of a persistent bitmap is a concern, we could easly use
> an xarray to track only "pending" vCPUs, so that the steady state cost is ~zero.
> Actually, given how easy that is (famous last words), unless removing from an
> xarray doesn't free memory soon-ish, I'd say go straight to a semi-permanent
> xarray to avoid bikeshedding over the cost of the bitmap.

After some thinking, I think I prefer the bitmap. The pending_vcpu_ids
xarray feels like a premature optimization.

> > I don't think we should do that, and your approach is much safer:
> 
> I disagree.  I actually arrived at the bitmap solution before reading this.  My
> concern isn't so much about IPI virtualization, it's about the lurking danger of
> an unverified vcpu_id.  E.g. see Naveen's suggestion in this thread of simply
> letting vcpu_load() initialize the per-VM table, which doesn't work because x86
> calls vcpu_load() as part of vCPU creation.  I wouldn't be all that surprised if
> there's another bug or two in KVM where colliding 
> 
> Ha!  Case in point, s390 had what is effectively the *exact* same bug and fixed
> it in the *exact* same way (hooking vcpu_postcreate()) over a decade ago in
> commit 255088244929 ("KVM: s390: fix SCA related races and double use"), and that
> obviously did nothing to help x86 from repeating the same mistake.
> 
> In other words, I want to fix this entire class of bugs, not play a game of
> whack-a-mole with bugs that humans are all but guaranteed to overlook.
> 
> My other concern with the proposed change is that the vCPU becomes reachable
> before long before kvm_arch_vcpu_postcreate().  Which should be fine" for IPI
> virtualization, but sets a precedence I'd rather not exist, because initializing
> vCPU state _after_ it's reachable is rarely correct.  E.g. msr_kvm_poll_control
> is initialized in postcreate for some reason, and that's technically buggy
> because it's possible, albeit extremely unlikely, that MSR_KVM_POLL_CONTROL could
> be written by userspace before postcreate() runs.

Yeah, agree on both points. I myself was a bit uncomfortable with my
vcpu_postcreate approach implying some unwritten ill-defined rules on
what things _must_ be done in vcpu_postcreate (rather than in
vcpu_create) and what things _must not_ be done in it.

> E.g. for the xarray approacy (sketch only, completely untested):
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index fdbd697d0337..e34ca5920393 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -791,6 +791,7 @@ struct kvm {
>         /* The current active memslot set for each address space */
>         struct kvm_memslots __rcu *memslots[KVM_MAX_NR_ADDRESS_SPACES];
>         struct xarray vcpu_array;
> +       struct xarray pending_vcpu_ids;
>         /*
>          * Protected by slots_lock, but can be read outside if an
>          * incorrect answer is acceptable.
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2df8ee9ecf6c..8bc4f7ffd76d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4179,6 +4179,12 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>                 return r;
>         }
>  
> +       r = xa_insert(&kvm->pending_vcpu_ids, id, xa_mk_value(id), GFP_KERNEL);
> +       if (r) {
> +               mutex_unlock(&kvm->lock);
> +               return r;
> +       }
> +
>         kvm->created_vcpus++;
>         mutex_unlock(&kvm->lock);
>  
> @@ -4249,6 +4255,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>          */
>         smp_wmb();
>         atomic_inc(&kvm->online_vcpus);
> +       xa_erase(&kvm->pending_vcpu_ids, id);
>         mutex_unlock(&vcpu->mutex);
>  
>         mutex_unlock(&kvm->lock);
> @@ -4273,6 +4280,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  vcpu_decrement:
>         mutex_lock(&kvm->lock);
>         kvm->created_vcpus--;
> +       xa_erase(&kvm->pending_vcpu_ids, id);
>         mutex_unlock(&kvm->lock);
>         return r;
>  }
> 
> 
> or if that doesn't work, the more naive bitmap approach:
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index fdbd697d0337..eadde580e27f 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -791,6 +791,8 @@ struct kvm {
>         /* The current active memslot set for each address space */
>         struct kvm_memslots __rcu *memslots[KVM_MAX_NR_ADDRESS_SPACES];
>         struct xarray vcpu_array;
> +       DECLARE_BITMAP(vcpu_ids, KVM_MAX_VCPU_IDS);
> +
>         /*
>          * Protected by slots_lock, but can be read outside if an
>          * incorrect answer is acceptable.
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2df8ee9ecf6c..c735698e76cf 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4179,6 +4179,11 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>                 return r;
>         }
>  
> +       if (__test_and_set_bit(id, kvm->vcpu_ids)) {
> +               mutex_unlock(&kvm->lock);
> +               return -EEXIST;
> +       }
> +
>         kvm->created_vcpus++;
>         mutex_unlock(&kvm->lock);
>  
> @@ -4213,7 +4218,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  
>         mutex_lock(&kvm->lock);
>  
> -       if (kvm_get_vcpu_by_id(kvm, id)) {
> +       if (WARN_ON_ONCE(kvm_get_vcpu_by_id(kvm, id))) {
>                 r = -EEXIST;
>                 goto unlock_vcpu_destroy;
>         }
> @@ -4273,6 +4278,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  vcpu_decrement:
>         mutex_lock(&kvm->lock);
>         kvm->created_vcpus--;
> +       __clear_bit(id, kvm->vcpu_ids);
>         mutex_unlock(&kvm->lock);
>         return r;
>  }

Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Chao Gao]

On Thu, Jul 16, 2026 at 04:08:01PM +0000, Dmytro Maluka wrote:
>vCPU creation in kvm_vm_ioctl_create_vcpu() may fail after
>kvm_arch_vcpu_create() -> vmx_vcpu_create() already succeeded. In such
>case kvm_vm_ioctl_create_vcpu() destroys the newly created vCPU in the
>failure path. However, that leaves a side effect: the IPIv pid_table
>entry remains configured with this vCPU's pi_desc address. As a result,
>when another vCPU sends an IPI to the APIC ID of this failed-to-create
>vCPU, it will cause HW to write to this (freed!) pi_desc memory. [*]
>
>An easy fix would be to clear the pid_table entry in vmx_vcpu_free().
>However that would be still problematic, for the following reason:
>userspace may try to create a vCPU with the same vcpu_id as an existing
>one; vmx_vcpu_create() will succeed, and only after that
>kvm_vm_ioctl_create_vcpu() will check for the duplicate vcpu_id and
>fail with -EEXIST and then free the vCPU in the failure path. So in this
>failure path, vmx_vcpu_free() would clear the pid_table entry for that
>already existing good vCPU, i.e. effectively disable IPIv for that vCPU.
>
>So instead fix the issue by moving the pid_table entry setup from
>.vcpu_create() to the newly introduced .vcpu_postcreate() callback
>called at the end of kvm_vm_ioctl_create_vcpu(), when we are sure that
>the vCPU creation succeeded.
>
>[*] Although, since this memory is freed into the kvm_vcpu_cache kmem
>    cache which is only used for allocating kvm_vcpus, _maybe_ this
>    memory will only be reused for pi_desc of another vCPU, not for
>    anything else. So _maybe_ this will only result in delivering the
>    IPI to a wrong vCPU (possibly of another VM) in the worst case, not
>    in a random corruption of kernel memory.
>
>Fixes: d588bb9be1da ("KVM: VMX: enable IPI virtualization")
>Signed-off-by: Dmytro Maluka <dmaluka@chromium.org>

Reviewed-by: Chao Gao <chao.gao@intel.com>