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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Massive lock contention due to unconditional acquisition of the global lpi_xa lock in the hot path.
- [High] Spurious -ENOMEM failures in vgic_add_lpi() due to a TOCTOU race with xarray node freeing.
--

commit ab3411f56aa5d7fb40c5a79e25df87d1b602fdeb
Author: Carlos López <clopez@suse.de>

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

This commit attempts to fix a race condition between LPI release and
re-registration by moving the reference count decrement inside the xarray
lock in vgic_put_irq(), and by evicting dead LPIs from the xarray during
vgic_add_lpi().

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 67d107e9a77d1..577286069368d 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 a dead LPI (refcount=0)
> +		 * from the deferred release path, pending cleanup by
> +		 * vgic_release_deleted_lpis(). Evict and free it if present.
> +		 */
> +		oldirq = __xa_store(&dist->lpi_xa, intid, irq, 0);

[Severity: High]
Can this __xa_store() fail with -ENOMEM due to a TOCTOU race with xarray node
freeing?

Earlier in vgic_add_lpi(), xa_reserve_irq() is called to pre-allocate memory.
If the slot is already occupied by a dead LPI with refcount=0,
xa_reserve_irq() returns 0 without actually allocating a new node.

If a concurrent thread (e.g. vgic_put_irq()) removes this dead LPI and frees
the underlying xarray node before vgic_add_lpi() acquires the xa_lock_irqsave,
the slot becomes completely empty.

When vgic_add_lpi() resumes, xa_load() returns NULL, causing
vgic_try_get_irq_ref() to return false. The code then falls into this else
branch and calls __xa_store(..., 0) with a gfp mask of 0. Since the node was
concurrently freed and gfp=0 prevents memory allocations, __xa_store() will
fail and return -ENOMEM, potentially breaking guest device initialization.

> +		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 5a4768d8cd4f3..c32e6c9777e56 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
[ ... ]
> @@ -167,11 +167,12 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>  		guard(spinlock_irqsave)(&dist->lpi_xa.xa_lock);
>  	}
>  
> -	if (!__vgic_put_irq(kvm, irq))
> +	if (!irq_is_lpi(kvm, irq->intid))
>  		return;
>  
>  	xa_lock_irqsave(&dist->lpi_xa, flags);
> -	vgic_release_lpi_locked(dist, irq);
> +	if (refcount_dec_and_test(&irq->refcount))
> +		vgic_release_lpi_locked(dist, irq);

[Severity: High]
Does this cause massive lock contention on the global lpi_xa lock in the hot
path?

Previously, the refcount was decremented locklessly and the global xarray lock
(&dist->lpi_xa.xa_lock) was only taken if the refcount dropped to zero.

By replacing this with an unconditional xa_lock_irqsave() for all LPIs before
checking the refcount, every LPI delivery (e.g. via
vgic_its_inject_cached_translation) that acquires and drops an IRQ reference
will now serialize on this single global spinlock across all vCPUs.

This unconditional lock acquisition also renders the preceding
guard(spinlock_irqsave) block completely redundant.

>  	xa_unlock_irqrestore(&dist->lpi_xa, flags);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703021508.2593100-2-clopez@suse.de?part=1