Re: [PATCH] KVM: arm64: vgic: Check the interrupt is still ours before migrating it

[Hyunwoo Kim <imv4bel@gmail.com>]

On Fri, Jun 05, 2026 at 01:43:32AM -0700, Oliver Upton wrote:
> On Fri, Jun 05, 2026 at 08:42:52AM +0100, Marc Zyngier wrote:
> > On Fri, 05 Jun 2026 07:00:37 +0100,
> > Oliver Upton <oupton@kernel.org> wrote:
> > > 
> > > On Fri, Jun 05, 2026 at 05:59:15AM +0900, Hyunwoo Kim wrote:
> > > > vgic_prune_ap_list() drops both ap_list_lock and irq_lock while migrating
> > > > an interrupt to another vCPU. After reacquiring the locks it only checks
> > > > that the affinity is unchanged (target_vcpu == vgic_target_oracle(irq))
> > > > before moving the interrupt, which assumes that an interrupt whose affinity
> > > > is preserved is still queued on this vCPU's ap_list.
> > > > 
> > > > That assumption no longer holds if the interrupt is taken off the ap_list
> > > > while the locks are dropped. vgic_flush_pending_lpis() removes the
> > > > interrupt from the list and sets irq->vcpu to NULL, but leaves
> > > > enabled/pending/target_vcpu untouched. As the interrupt is still enabled
> > > > and pending, vgic_target_oracle() returns the same target_vcpu, so the
> > > > affinity check passes and list_del() is run a second time on an entry that
> > > > has already been removed.
> > > > 
> > > > Also check that the interrupt is still assigned to this vCPU
> > > > (irq->vcpu == vcpu) before moving it.
> > > > 
> > > > Fixes: 0919e84c0fc1 ("KVM: arm/arm64: vgic-new: Add IRQ sync/flush framework")
> > > > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > > 
> > > Looking at this and the other VGIC patch you sent (which should've been
> > > a combined series), are you trying to deal with a vCPU writing to
> > > another vCPU's redistributor? I.e. vCPU B setting GICR_CTLR.EnableLPIs=0
> > > behind the back of vCPU A?
> > > 
> > > That is extremely relevant information as the off-the-cuff reaction is
> > > that no race exists. But since the GIC architecture is awesome and
> > > allows for this sort of insanity, it obviously does....
> > > 
> > > Anyway, for LPIs resident on a particular RD, there's zero expectation
> > > that the pending state is preserved when EnableLPIs=0. So I'd rather
> > > vgic_flush_pending_lpis() just invalidate the pending state.
> > 
> > Just clearing the pending state introduces a potential problem as we
> > now have an interrupt that is neither active nor pending on the AP
> > list. It is not impossible to solve (we now have similar behaviours
> > with SPI deactivation from another vcpu), but that requires posting a
> > KVM_REQ_VGIC_PROCESS_UPDATE to the target vcpu.
> 
> Right, I was suggesting that in addition to deleting the LPI from the AP
> list we actually invalidate the pending state so that someone sitting on
> a pointer to a to-be-freed LPI sees vgic_target_oracle() returning
> NULL
> 
> > > Beyond that, I see two other fixes for lifetime issues around the
> > > vgic_irq in the middle of migration. I'd like to see explicit RCU
> > > protection around the release && reacquire of the ap_list_lock rather
> > > than depending on the precondition that IRQs are disabled.
> > 
> > I'm not sure I follow. Are you suggesting turning the AP list into an
> > RCU protected list?
> 
> No, sorry, I should expand a little.
> 
> We store a reference on the vgic_irq struct in the AP list, which is
> stable so long as the ap_list_lock is held. It should be possible for
> the refcount to drop to 0 between releasing the ap_list_lock and
> reacquiring it.
> 
> So either vgic_prune_ap_list() takes an additional reference on the
> vgic_irq before dropping the ap_list_lock or rely on RCU to protect
> vgic_irq structs observed with a non-zero refcount.

What are your thoughts on this approach?


Best regards,
Hyunwoo Kim

---

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 933983bb2005..7fb871c3ccd8 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -523,7 +523,7 @@ static void __kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
 	 * Retire all pending LPIs on this vcpu anyway as we're
 	 * going to destroy it.
 	 */
-	vgic_flush_pending_lpis(vcpu);
+	vgic_flush_pending_lpis(vcpu, true);

 	INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
 	kfree(vgic_cpu->private_irqs);
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 5913a20d8301..f85d63f17af0 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -303,7 +303,7 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 		if (ctlr != GICR_CTLR_ENABLE_LPIS)
 			return;

-		vgic_flush_pending_lpis(vcpu);
+		vgic_flush_pending_lpis(vcpu, false);
 		vgic_its_invalidate_all_caches(vcpu->kvm);
 		atomic_set_release(&vgic_cpu->ctlr, 0);
 	} else {
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 1e9fe8764584..09629a38fc0a 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -192,7 +192,7 @@ static void vgic_release_deleted_lpis(struct kvm *kvm)
 	xa_unlock_irqrestore(&dist->lpi_xa, flags);
 }

-void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
+void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu, bool destroy)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_irq *irq, *tmp;
@@ -204,6 +204,13 @@ void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu)
 	list_for_each_entry_safe(irq, tmp, &vgic_cpu->ap_list_head, ap_list) {
 		if (irq_is_lpi(vcpu->kvm, irq->intid)) {
 			raw_spin_lock(&irq->irq_lock);
+			/* Leave interrupts pending a migration for prune. */
+			if (!destroy && irq->vcpu != vgic_target_oracle(irq)) {
+				raw_spin_unlock(&irq->irq_lock);
+				continue;
+			}
+			/* Pending state is not preserved across EnableLPIs=0. */
+			irq->pending_latch = false;
 			list_del(&irq->ap_list);
 			irq->vcpu = NULL;
 			raw_spin_unlock(&irq->irq_lock);
@@ -797,6 +804,9 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)

 		/* This interrupt looks like it has to be migrated. */

+		/* Keep the interrupt alive while the locks are dropped. */
+		vgic_get_irq_ref(irq);
+
 		raw_spin_unlock(&irq->irq_lock);
 		raw_spin_unlock(&vgic_cpu->ap_list_lock);

@@ -839,6 +849,8 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
 		raw_spin_unlock(&vcpuB->arch.vgic_cpu.ap_list_lock);
 		raw_spin_unlock(&vcpuA->arch.vgic_cpu.ap_list_lock);

+		deleted_lpis |= vgic_put_irq_norelease(vcpu->kvm, irq);
+
 		if (target_vcpu_needs_kick) {
 			kvm_make_request(KVM_REQ_IRQ_PENDING, target_vcpu);
 			kvm_vcpu_kick(target_vcpu);
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 9d941241c8a2..c1ac24ede899 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -341,7 +341,7 @@ void vgic_v3_put(struct kvm_vcpu *vcpu);
 bool vgic_has_its(struct kvm *kvm);
 int kvm_vgic_register_its_device(void);
 void vgic_enable_lpis(struct kvm_vcpu *vcpu);
-void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu);
+void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu, bool destroy);
 int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi);
 int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
 int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,