Re: [PATCH v5 2/2] KVM: arm64: vgic: Mitigate potential LPI registration failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The error path in `vgic_add_lpi()` calls `xa_release()`, which acquires the `lpi_xa` lock without disabling interrupts, leading to a potential deadlock.
--

--- Patch [2]: [PATCH v5 2/2] KVM: arm64: vgic: Mitigate potential LPI registration failure ---
commit c5f231166837f986fca0d6f0a35dc7e8c46a61d1
Author: Carlos López <clopez@suse.de>
Subject: KVM: arm64: vgic: Mitigate potential LPI registration failure

This commit mitigates a potential allocation failure when inserting a new LPI 
into the VGIC LPI xarray. It updates vgic_add_lpi() to pass GFP_NOWAIT and 
__GFP_ACCOUNT to __xa_store(), allowing allocation to occur under the lock if 
the reserved entry was removed concurrently.

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -124,7 +124,8 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid,
>  		 * holds before freeing it, as otherwise someone else could still
>  		 * hold a pointer to the evicted struct.
>  		 */
> -		oldirq = __xa_store(&dist->lpi_xa, intid, irq, 0);
> +		oldirq = __xa_store(&dist->lpi_xa, intid, irq,
> +				    GFP_NOWAIT | __GFP_ACCOUNT);
>  		ret = xa_err(oldirq);
>  		if (!ret && oldirq &&
>  		    !WARN_ON_ONCE(refcount_read(&oldirq->refcount) ||
>  				  !oldirq->pending_release))
>  			kfree_rcu(oldirq, rcu);
>  	}
>  
>  	xa_unlock_irqrestore(&dist->lpi_xa, flags);
>  
>  	if (ret) {
>  		xa_release(&dist->lpi_xa, intid);

[Severity: High]
This is a pre-existing issue, but does calling xa_release() here introduce a
potential lock inversion deadlock with hardirqs?

The xa_release() function internally uses xa_cmpxchg(), which acquires the 
lpi_xa.xa_lock using spin_lock() without disabling local interrupts.

If a hardware interrupt fires on this CPU while the lock is held (for example,
an eventfd signal triggering irqfd_wakeup() in hardirq context for fast-path 
MSIs), it can inject an LPI and eventually call vgic_put_irq().

Because vgic_put_irq() acquires lpi_xa.xa_lock using spin_lock_irqsave(), the 
interrupt handler would spin forever waiting for the lock held by the preempted
vgic_add_lpi() thread.

Since this patch makes it more likely for the error path to be hit (if the 
GFP_NOWAIT allocation fails), should this path be updated to manually erase 
the entry while holding the xarray lock with interrupts disabled?

>  		kfree(irq);
>  
>  		return ERR_PTR(ret);

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