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

["Carlos López" <clopez@suse.de>]

On 7/9/26 5:09 PM, sashiko-bot@kernel.org wrote:
> 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?

I clearly did not read Oliver's diff carefully enough, this does not
happen with those changes.

> 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);
>