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

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

[Tomas Krcka]

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

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>
---
 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);
 
 	/* Unmap request? */
 	if (!info)
-- 
2.40.1

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

[Marc Zyngier]

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.

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@kernel.org> 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@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:

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


> 
> Reviewed-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
> 
> 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

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

On Mon, 2024-12-30 at 14:28 +0000, Marc Zyngier wrote:
> 
> 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.

The latter isn't needed if you have the former, surely?

I've lost count of the number of patches I've posted over the decades
from my function non-corporate email address, just using a From: and
Signed-off-by: in the body for my work address. We've always accepted
that, and git-am does the right thing (discarding the actual From:
address from the headers of the email).


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

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

[Marc Zyngier]

On Mon, 30 Dec 2024 15:21:38 +0000,
David Woodhouse <dwmw2@infradead.org> wrote:
> 
> [1  <text/plain; UTF-8 (quoted-printable)>]
> On Mon, 2024-12-30 at 14:28 +0000, Marc Zyngier wrote:
> > 
> > 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.
> 
> The latter isn't needed if you have the former, surely?

I don't see why we shouldn't have it. AFAIC, this is a different
sender, and I'm pretty sure tglx applies the same policy.

> I've lost count of the number of patches I've posted over the decades
> from my function non-corporate email address, just using a From: and
> Signed-off-by: in the body for my work address. We've always accepted
> that, and git-am does the right thing (discarding the actual From:
> address from the headers of the email).

Is that the royal 'We'?

	M.

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

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]

On Mon, 2024-12-30 at 17:09 +0000, Marc Zyngier wrote:
> On Mon, 30 Dec 2024 15:21:38 +0000,
> David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > [1  <text/plain; UTF-8 (quoted-printable)>]
> > On Mon, 2024-12-30 at 14:28 +0000, Marc Zyngier wrote:
> > > 
> > > 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.
> > 
> > The latter isn't needed if you have the former, surely?
> 
> I don't see why we shouldn't have it. AFAIC, this is a different
> sender, and I'm pretty sure tglx applies the same policy.

Not in my experience. I send patches like this all the time and don't
recall anyone ever complaining. People often have to work around broken
corporate email but still want to have the authorship correctly
attributed.

In the git log you can find plenty of commits containing
'Link:.*lore.kernel.org.*dwmw2@infradead.org' which I sent from my own
address, for which the Author and SoB are both @amazon.

Let's see if I can find a tglx one... 

https://lore.kernel.org/all/20240802135555.564941-2-dwmw2@infradead.org
which became https://git.kernel.org/torvalds/c/70e6b7d9ae3c6 for
example?

The point of the From: line at the top of the email body is to
*replace* the one in the header. Or put another way, the one in the
header is used as a fallback if there is no explicit From: in the body
of the message.

Documentation/process/submitting-patches.rst phrases it the second way:

The ``from`` line must be the very first line in the message body,
and has the form:

        From: Patch Author <author@example.com>

The ``from`` line specifies who will be credited as the author of the
patch in the permanent changelog.  If the ``from`` line is missing,
then the ``From:`` line from the email header will be used to determine
the patch author in the changelog.

> > I've lost count of the number of patches I've posted over the decades
> > from my function non-corporate email address, just using a From: and
> > Signed-off-by: in the body for my work address. We've always accepted
> > that, and git-am does the right thing (discarding the actual From:
> > address from the headers of the email).
> 
> Is that the royal 'We'?


Nah, it's been a while since I've been an active maintainer of anything
and applying patches from email. :)

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]