[PATCH] KVM: arm64: vgic: Fix race between LPI release and re-registration

[PATCH] KVM: arm64: vgic: Fix race between LPI release and re-registration

[Carlos López]

Fix a potential race between decrementing an LPI's reference count and
evicting that structure from the LPI xarray.

LPI structures are maintained in the VGIC LPI xarray (dist->lpi_xa).
When the reference count of an LPI structure drops to zero,
vgic_release_lpi_locked() removes the structure from the xarray and
frees it under the xarray lock.

However, the release of an LPI can race with a concurrent LPI
re-registration with the same INTID via vgic_add_lpi() on another CPU,
since the reference count drop and the xarray eviction are not performed
in a single atomic step. This can happen e.g. if the guest issues a
DISCARD while the LPI is still referenced from a vCPU's active-pending
list (ap_list), and the same INTID is re-mapped via MAPTI.

Particularly, vgic_release_lpi_locked() is called from two distinct
paths: direct release via vgic_put_irq(), and deferred release via
vgic_release_deleted_lpis(). During direct release, the issue can result
in deleting a newly registered LPI from the xarray:

  CPU0 (Releasing LPI)                    CPU1 (Adding new LPI)
  ====================                    =====================
  vgic_put_irq()
      __vgic_put_irq()
          refcount_dec_and_test()
                                          vgic_add_lpi()
                                              xa_lock_irqsave(..);
                                              old_irq = xa_load(&dist->lpi_xa, intid);
                                              vgic_try_get_irq_ref(old_irq) == false
                        new IRQ inserted -->  __xa_store(&dist->lpi_xa, intid, ..)
                                              xa_unlock_irqrestore(..);
  xa_lock_irqsave(..);
  vgic_release_lpi_locked()
      __xa_erase(&dist->lpi_xa, irq->intid);   <-- BUG: new IRQ is erased
      kfree_rcu(old_irq)

During the deferred release path, the old IRQ can be leaked:

  CPU0 (Releasing LPI)                    CPU1 (Adding new LPI)
  ====================                    =====================
  vgic_put_irq_norelease()
      __vgic_put_irq()
          refcount_dec_and_test()
      irq->pending_release = true
                                          vgic_add_lpi()
                                              xa_lock_irqsave(..);
                                              old_irq = xa_load(&dist->lpi_xa, intid);
                                              vgic_try_get_irq_ref() == false
                 BUG: old IRQ overwritten --> __xa_store(&dist->lpi_xa, intid, ..)
                                              xa_unlock_irqrestore(..);

  vgic_release_deleted_lpis()
      xa_lock_irqsave(..);
      xa_for_each() { .. } <-- old IRQ with pending_release = true
                               is gone, so it cannot be released

Fix both issues with two coordinated changes. On the release path,
erase and free the IRQ only if the xarray entry still contains the IRQ
being released. On the registration path, if a dead LPI is encountered
(non-NULL entry with refcount=0), release the old structure after
storing the new one. This ensures that whichever CPU acquires the
xarray lock first, it will take on the responsibility of releasing a
to-be-cleaned-up LPI.

Reported-by: Claude:claude-opus-4-6
Fixes: 3a08a6ca7c37 ("KVM: arm64: vgic-v3: Use bare refcount for VGIC LPIs")
Fixes: d54594accf73 ("KVM: arm64: vgic-v3: Erase LPIs from xarray outside of raw spinlocks")
Signed-off-by: Carlos López <clopez@suse.de>
---
 arch/arm64/kvm/vgic/vgic-its.c | 10 +++++++++-
 arch/arm64/kvm/vgic/vgic.c     | 14 ++++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 67d107e9a77d..42983d5211cc 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -116,7 +116,15 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
 		kfree(irq);
 		irq = oldirq;
 	} else {
-		ret = xa_err(__xa_store(&dist->lpi_xa, intid, irq, 0));
+		/*
+		 * The entry is either empty or contains an IRQ whose refcount is 0
+		 * but has not yet been freed by vgic_release_lpi_locked(). Evict the
+		 * IRQ and free it, if there is one.
+		 */
+		oldirq = __xa_store(&dist->lpi_xa, intid, irq, 0);
+		ret = xa_err(oldirq);
+		if (!ret && oldirq)
+			kfree_rcu(oldirq, rcu);
 	}
 
 	xa_unlock_irqrestore(&dist->lpi_xa, flags);
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 5a4768d8cd4f..b7f6b6c4dcf2 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -131,9 +131,19 @@ struct vgic_irq *vgic_get_vcpu_irq(struct kvm_vcpu *vcpu, u32 intid)
 
 static void vgic_release_lpi_locked(struct vgic_dist *dist, struct vgic_irq *irq)
 {
+	struct vgic_irq *old;
+
 	lockdep_assert_held(&dist->lpi_xa.xa_lock);
-	__xa_erase(&dist->lpi_xa, irq->intid);
-	kfree_rcu(irq, rcu);
+
+	/*
+	 * Free the IRQ if it is still present in the xarray, as its entry could have
+	 * been overwritten after the refcount was dropped, but before the xarray lock
+	 * was acquired. If the cmpxchg fails, vgic_add_lpi() grabbed the lock first
+	 * and freed the old IRQ along the way.
+	 */
+	old = __xa_cmpxchg(&dist->lpi_xa, irq->intid, irq, NULL, 0);
+	if (old == irq)
+		kfree_rcu(irq, rcu);
 }
 
 static __must_check bool __vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)

base-commit: 1ee27dacbe5dc4def481794d899d67b0d4570094
-- 
2.51.0