Re: [PATCH] irqchip/gic-v3-its: fix raw_local_irq_restore() called with IRQs enabled

[Marc Zyngier <maz@kernel.org>]

Hi Tomas,

On Mon, 30 Dec 2024 13:49:03 +0000,
Tomas Krcka <tomas.krcka@gmail.com> wrote:
> 
> The following call-chain leads to misuse of spinlock_irq
> when spinlock_irqsave was hold.
> 
> irq_set_vcpu_affinity
>   -> irq_get_desc_lock (spinlock_irqsave)
>    -> its_irq_set_vcpu_affinity
>     -> guard(raw_spin_lock_irq) <--- this enables interrupts
>   -> irq_put_desc_unlock // <--- WARN IRQs enabled

Urgh. Nice catch. It really should have been either raw_spin_lock or
the _irqsave variant, but not the _irq variant which should really
never be used outside of the core code. I guess this was never really
tested when rewritten at commit time...

> Fix the issue by using guard(raw_spinlock), since the function is
> already called with irqsave and raw_spin_lock was used before the commit
> b97e8a2f7130 ("irqchip/gic-v3-its: Fix potential race condition in its_vlpi_prop_update()")
> introducing the guard as well.
> 
> This was discovered through the lock debugging, and the corresponding
> log is as follows:
> 
> raw_local_irq_restore() called with IRQs enabled
> WARNING: CPU: 38 PID: 444 at kernel/locking/irqflag-debug.c:10 warn_bogus_irq_restore+0x2c/0x38
>  Call trace:
>   warn_bogus_irq_restore+0x2c/0x38
>    _raw_spin_unlock_irqrestore+0x68/0x88
>    __irq_put_desc_unlock+0x1c/0x48
>    irq_set_vcpu_affinity+0x74/0xc0
>    its_map_vlpi+0x44/0x88
>    kvm_vgic_v4_set_forwarding+0x148/0x230
>    kvm_arch_irq_bypass_add_producer+0x20/0x28
>    __connect+0x98/0xb8
>    irq_bypass_register_consumer+0x150/0x178
>    kvm_irqfd+0x6dc/0x744
>    kvm_vm_ioctl+0xe44/0x16b0
> 
> Fixes: b97e8a2f7130 ("irqchip/gic-v3-its: Fix potential race condition in its_vlpi_prop_update()")
> Signed-off-by: Tomas Krcka <krckatom@amazon.de>

Two problems here:

- there is no "From Tomas Krcka <krckatom@amazon.de>" at the beginning
  of the patch, which is needed since you are posting from a gmail.com
  address

- there is no SoB using your gmail.com address, which is needed since
  this patch appears to be from your Amazon doppelganger.

So either you post this patch from your amazon.de email, or you add
the two missing pieces of information that are required.

> ---
>  drivers/irqchip/irq-gic-v3-its.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 92244cfa0464..8c3ec5734f1e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2045,7 +2045,7 @@ static int its_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu_info)
>  	if (!is_v4(its_dev->its))
>  		return -EINVAL;
>  
> -	guard(raw_spinlock_irq)(&its_dev->event_map.vlpi_lock);
> +	guard(raw_spinlock)(&its_dev->event_map.vlpi_lock);

This otherwise looks good to me. Please repost this with the above
issues fixed, and the following tags:

Reviewed-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.