| Message ID | 20241230134903.84613-1-krckatom@amazon.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | irqchip/gic-v3-its: fix raw_local_irq_restore() called with IRQs enabled | expand |
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.
> 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
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).
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.
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. :)
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)
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(-)