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

[Krcka, Tomas]

> On 30. Dec 2024, at 15:28, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Hi Tomas,
> 
> On Mon, 30 Dec 2024 13:49:03 +0000,
> Tomas Krcka <tomas.krcka@xxxxxxxxx> 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@xxxxxxxxx>
> 
> Two problems here:
> 
> - there is no "From Tomas Krcka <krckatom@xxxxxxxxx>" 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:

Ack, reposted as v2 - https://lore.kernel.org/all/20241230150825.62894-1-krckatom@xxxxxxxxx/


> 
> Reviewed-by: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> 
> Thanks,
> 
>        M.
> 
> --
> Without deviation from the norm, progress is not possible.





Amazon Web Services Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597