* Re: [PATCH] x86/irq: Plug vector setup race
[not found] <draft-87ikjhrhhh.ffs@tglx>
@ 2025-07-31 12:45 ` Thomas Gleixner
2025-08-01 14:56 ` Hogan Wang
2025-08-02 12:18 ` [tip: x86/urgent] " tip-bot2 for Thomas Gleixner
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Thomas Gleixner @ 2025-07-31 12:45 UTC (permalink / raw)
To: Hogan Wang, x86, dave.hansen, kvm, alex.williamson
Cc: weidong.huang, yechuan, hogan.wang, wangxinxin.wang, jianjay.zhou,
wangjie88, Marc Zyngier, LKML
On Thu, Jul 24 2025 at 12:49, Thomas Gleixner wrote:
Hogan!
> Hogan reported a vector setup race, which overwrites the interrupt
> descriptor in the per CPU vector array resulting in a disfunctional device.
>
> CPU0 CPU1
> interrupt is raised in APIC IRR
> but not handled
> free_irq()
> per_cpu(vector_irq, CPU1)[vector] = VECTOR_SHUTDOWN;
>
> request_irq() common_interrupt()
> d = this_cpu_read(vector_irq[vector]);
>
> per_cpu(vector_irq, CPU1)[vector] = desc;
>
> if (d == VECTOR_SHUTDOWN)
> this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
>
> free_irq() cannot observe the pending vector in the CPU1 APIC as there is
> no way to query the remote CPUs APIC IRR.
>
> This requires that request_irq() uses the same vector/CPU as the one which
> was freed, but this also can be triggered by a spurious interrupt.
>
> Prevent this by reevaluating vector_irq under the vector lock, which is
> held by the interrupt activation code when vector_irq is updated.
Does this fix your problem?
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/irq: Plug vector setup race
2025-07-31 12:45 ` [PATCH] x86/irq: Plug vector setup race Thomas Gleixner
@ 2025-08-01 14:56 ` Hogan Wang
2025-08-02 11:54 ` Thomas Gleixner
0 siblings, 1 reply; 6+ messages in thread
From: Hogan Wang @ 2025-08-01 14:56 UTC (permalink / raw)
To: tglx, x86, dave.hansen, kvm, alex.williamson
Cc: weidong.huang, yechuan, hogan.wang, wangxinxin.wang, jianjay.zhou,
wangjie88, maz, linux-kernel
> On Thu, Jul 24 2025 at 12:49, Thomas Gleixner wrote:
>
Thank you very much for your professional, friendly, and detailed response.
Based on the clear modification suggestions you provided, I conducted
retesting and validation using the following methods:
1) Start a virtual machine with 192U384G specification, and configure
one VirtioNet network card and one VirtioSCSI disk.
2) After the virtual machine starts successfully, execute the following
script inside the virtual machine. The interrupt number 30 is the
VirtioNet MSI-x interrupt.
for((;;))
do (for((i=0;i<192;i++))
do echo $i > /proc/irq/30/smp_affinity_list
sleep 0.1
done)
done
After a 7x24-hour test, no error logs of the type "No irq handler for
vector" were found, I believe this issue should have already been
resolved.
As you said, this fix cannot solve the problem of lost interrupts.
I believe an effective solution to the issue of lost interrupts
might be to modify the vifo module to avoid un-plug/plug irq,
and instead use a more lightweight method to switch interrupt
modes. Just like:
vfio_irq_handler()
if kvm_mode
vfio_send_eventfd(kvm_irq_fd);
else
vfio_send_eventfd(qemu_irq_fd);
However, this will bring about some troubles:
1) The kvm_mode variable should be protected, leading to performance loss.
2) The VFIO interface requires the passing of two eventfds.
3) Add another interface to implement mode switching.
Do you have a better solution to fix this interrupt loss issue?
There is a question that has been troubling me: Why are interrupts
still reported after they have been masked and the interrupt remapping
table entries have been disabled? Is this interrupt cached somewhere?
> Hogan!
>
> > Hogan reported a vector setup race, which overwrites the interrupt
> > descriptor in the per CPU vector array resulting in a disfunctional device.
> >
> > CPU0 CPU1
> > interrupt is raised in APIC IRR
> > but not handled
> > free_irq()
> > per_cpu(vector_irq, CPU1)[vector] = VECTOR_SHUTDOWN;
> >
> > request_irq() common_interrupt()
> > d = this_cpu_read(vector_irq[vector]);
> >
> > per_cpu(vector_irq, CPU1)[vector] = desc;
> >
> > if (d == VECTOR_SHUTDOWN)
> > this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
> >
> > free_irq() cannot observe the pending vector in the CPU1 APIC as there
> > is no way to query the remote CPUs APIC IRR.
> >
> > This requires that request_irq() uses the same vector/CPU as the one
> > which was freed, but this also can be triggered by a spurious interrupt.
> >
> > Prevent this by reevaluating vector_irq under the vector lock, which
> > is held by the interrupt activation code when vector_irq is updated.
>
> Does this fix your problem?
Thanks,
Hogan
--
2.45.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/irq: Plug vector setup race
2025-08-01 14:56 ` Hogan Wang
@ 2025-08-02 11:54 ` Thomas Gleixner
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2025-08-02 11:54 UTC (permalink / raw)
To: Hogan Wang, x86, dave.hansen, kvm, alex.williamson
Cc: weidong.huang, yechuan, hogan.wang, wangxinxin.wang, jianjay.zhou,
wangjie88, maz, linux-kernel
On Fri, Aug 01 2025 at 22:56, Hogan Wang wrote:
> I believe an effective solution to the issue of lost interrupts
> might be to modify the vifo module to avoid un-plug/plug irq,
> and instead use a more lightweight method to switch interrupt
> modes. Just like:
>
> vfio_irq_handler()
> if kvm_mode
> vfio_send_eventfd(kvm_irq_fd);
> else
> vfio_send_eventfd(qemu_irq_fd);
>
> However, this will bring about some troubles:
> 1) The kvm_mode variable should be protected, leading to performance loss.
> 2) The VFIO interface requires the passing of two eventfds.
> 3) Add another interface to implement mode switching.
>
> Do you have a better solution to fix this interrupt loss issue?
Interesting. I looked at vfio_irq_handler(), which is in the platform/
part of VFIO. The corresponding vfio_set_trigger(), which switches eventfds
does the right thing:
disable_irq();
update(trigger);
enable_irq();
disable_irq() ensures that there is no interrupt handler in progress, so
it becomes safe to switch the trigger in the data structure which is has
been handed to request_irq() as @dev_id argument. For edge type
interupts this ensures that a interrupt which arrives while disabled is
retriggered on enable, so that no interrupt can get lost.
The PCI variant is using the trigger itself as the @dev_id argument and
therefore has to do the free_irq()/request_irq() dance. It shouldn't be
hard to convert the PCI implementation over to the disable/enable scheme.
> There is a question that has been troubling me: Why are interrupts
> still reported after they have been masked and the interrupt remapping
> table entries have been disabled? Is this interrupt cached somewhere?
Let me bring back the picture I used before:
CPU0 CPU1
vmenter(vCPU0)
.... local_irq_disable()
msi_set_affinity()
#1 mask(MSI-X)
vmexit()
#2 ... interrupt is raised in APIC
but not handled
#3 really_mask(MSI-X)
free_irq()
mask();
#4 __synchronize_irq()
msi_domain_deactivate()
write_msg(0);
x86_vector_deactivate()
#5 per_cpu(vector_irq, cpu)[vector] = VECTOR_SHUTDOWN;
#6 local_irq_enable()
interrupt is handled and
observes VECTOR_SHUTDOWN
writes VECTOR_UNUSED
request_irq()
x86_vector_activate()
per_cpu(vector_irq, cpu)[vector] = desc;
msi_domain_deactivate()
write_msg(msg);
unmask();
#1 is the mask operation in the VM, which is trapped, i.e. the interrupt
is not yet masked at the MSIX level.
#2 The device raises the interupt _before_ the host can mask the
interrupt at the PCI-MSIX level (#3).
The interrupt is sent to the APIC of the target CPU 1, which sets the
corresponding IRR bit in the APIC if the CPU cannot handle it at that
point, because it has interrupts disabled.
#4 cannot observe the pending IRR bit on CPU1's APIC and therefore
concludes that there is no interrupt in flight.
If the host side VMM manages to shut down the interrupt completely (#5)
_before_ CPU1 reenables interrupts (#6), then CPU1 will observe
VECTOR_SHUTDOWN and treats it as a spurious interrupt.
The same problem exists on bare metal, when a driver leaves the device
interrupts enabled and then does a free/request dance:
CPU0 CPU1
.... local_irq_disable()
#1 free_irq()
#2 ... interrupt is raised in APIC
but not handled
#3 really_mask(MSI-X)
#4 __synchronize_irq()
msi_domain_deactivate()
write_msg(0);
x86_vector_deactivate()
#5 per_cpu(vector_irq, cpu)[vector] = VECTOR_SHUTDOWN;
#6 local_irq_enable()
interrupt is handled and
observes VECTOR_SHUTDOWN
writes VECTOR_UNUSED
request_irq()
x86_vector_activate()
per_cpu(vector_irq, cpu)[vector] = desc;
msi_domain_deactivate()
write_msg(msg);
unmask();
See?
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: x86/urgent] x86/irq: Plug vector setup race
[not found] <draft-87ikjhrhhh.ffs@tglx>
2025-07-31 12:45 ` [PATCH] x86/irq: Plug vector setup race Thomas Gleixner
@ 2025-08-02 12:18 ` tip-bot2 for Thomas Gleixner
2025-08-02 20:08 ` tip-bot2 for Thomas Gleixner
2025-08-04 22:42 ` tip-bot2 for Thomas Gleixner
3 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2025-08-02 12:18 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Hogan Wang, Thomas Gleixner, x86, linux-kernel
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 69adc077da4c247dd39a8a0e3a898a25924e98d0
Gitweb: https://git.kernel.org/tip/69adc077da4c247dd39a8a0e3a898a25924e98d0
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 24 Jul 2025 12:49:30 +02:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sat, 02 Aug 2025 14:09:38 +02:00
x86/irq: Plug vector setup race
Hogan reported a vector setup race, which overwrites the interrupt
descriptor in the per CPU vector array resulting in a disfunctional device.
CPU0 CPU1
interrupt is raised in APIC IRR
but not handled
free_irq()
per_cpu(vector_irq, CPU1)[vector] = VECTOR_SHUTDOWN;
request_irq() common_interrupt()
d = this_cpu_read(vector_irq[vector]);
per_cpu(vector_irq, CPU1)[vector] = desc;
if (d == VECTOR_SHUTDOWN)
this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
free_irq() cannot observe the pending vector in the CPU1 APIC as there is
no way to query the remote CPUs APIC IRR.
This requires that request_irq() uses the same vector/CPU as the one which
was freed, but this also can be triggered by a spurious interrupt.
Prevent this by reevaluating vector_irq under the vector lock, which is
held by the interrupt activation code when vector_irq is updated.
Interestingly enough this problem managed to be hidden for more than a
decade.
Fixes: 9345005f4eed ("x86/irq: Fix do_IRQ() interrupt warning for cpu hotplug retriggered irqs")
Reported-by: Hogan Wang <hogan.wang@huawei.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Hogan Wang <hogan.wang@huawei.com>
Link: https://lore.kernel.org/all/draft-87ikjhrhhh.ffs@tglx
---
arch/x86/kernel/irq.c | 63 ++++++++++++++++++++++++++++++++----------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 9ed29ff..10721a1 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -256,26 +256,59 @@ static __always_inline void handle_irq(struct irq_desc *desc,
__handle_irq(desc, regs);
}
-static __always_inline int call_irq_handler(int vector, struct pt_regs *regs)
+static struct irq_desc *reevaluate_vector(int vector)
{
- struct irq_desc *desc;
- int ret = 0;
+ struct irq_desc *desc = __this_cpu_read(vector_irq[vector]);
+
+ if (!IS_ERR_OR_NULL(desc))
+ return desc;
+
+ if (desc == VECTOR_UNUSED)
+ pr_emerg_ratelimited("No irq handler for %d.%u\n", smp_processor_id(), vector);
+ else
+ __this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
+ return NULL;
+}
+
+static __always_inline bool call_irq_handler(int vector, struct pt_regs *regs)
+{
+ struct irq_desc *desc = __this_cpu_read(vector_irq[vector]);
- desc = __this_cpu_read(vector_irq[vector]);
if (likely(!IS_ERR_OR_NULL(desc))) {
handle_irq(desc, regs);
- } else {
- ret = -EINVAL;
- if (desc == VECTOR_UNUSED) {
- pr_emerg_ratelimited("%s: %d.%u No irq handler for vector\n",
- __func__, smp_processor_id(),
- vector);
- } else {
- __this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
- }
+ return true;
}
- return ret;
+ /*
+ * Reevaluate with vector_lock held to prevent a race against
+ * request_irq() setting up the vector:
+ *
+ * CPU0 CPU1
+ * interrupt is raised in APIC IRR
+ * but not handled
+ * free_irq()
+ * per_cpu(vector_irq, CPU1)[vector] = VECTOR_SHUTDOWN;
+ *
+ * request_irq() common_interrupt()
+ * d = this_cpu_read(vector_irq[vector]);
+ *
+ * per_cpu(vector_irq, CPU1)[vector] = desc;
+ *
+ * if (d == VECTOR_SHUTDOWN)
+ * this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
+ *
+ * This requires that the same vector on the same target CPU is
+ * handed out or that a spurious interrupt hits that CPU/vector.
+ */
+ lock_vector_lock();
+ desc = reevaluate_vector(vector);
+ unlock_vector_lock();
+
+ if (!desc)
+ return false;
+
+ handle_irq(desc, regs);
+ return true;
}
/*
@@ -289,7 +322,7 @@ DEFINE_IDTENTRY_IRQ(common_interrupt)
/* entry code tells RCU that we're not quiescent. Check it. */
RCU_LOCKDEP_WARN(!rcu_is_watching(), "IRQ failed to wake up RCU");
- if (unlikely(call_irq_handler(vector, regs)))
+ if (unlikely(!call_irq_handler(vector, regs)))
apic_eoi();
set_irq_regs(old_regs);
^ permalink raw reply related [flat|nested] 6+ messages in thread* [tip: x86/urgent] x86/irq: Plug vector setup race
[not found] <draft-87ikjhrhhh.ffs@tglx>
2025-07-31 12:45 ` [PATCH] x86/irq: Plug vector setup race Thomas Gleixner
2025-08-02 12:18 ` [tip: x86/urgent] " tip-bot2 for Thomas Gleixner
@ 2025-08-02 20:08 ` tip-bot2 for Thomas Gleixner
2025-08-04 22:42 ` tip-bot2 for Thomas Gleixner
3 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2025-08-02 20:08 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Hogan Wang, Thomas Gleixner, x86, linux-kernel
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: e3079ac6cf4213dd46a7a292150b2ba7e6e85bac
Gitweb: https://git.kernel.org/tip/e3079ac6cf4213dd46a7a292150b2ba7e6e85bac
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 24 Jul 2025 12:49:30 +02:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sat, 02 Aug 2025 21:56:35 +02:00
x86/irq: Plug vector setup race
Hogan reported a vector setup race, which overwrites the interrupt
descriptor in the per CPU vector array resulting in a disfunctional device.
CPU0 CPU1
interrupt is raised in APIC IRR
but not handled
free_irq()
per_cpu(vector_irq, CPU1)[vector] = VECTOR_SHUTDOWN;
request_irq() common_interrupt()
d = this_cpu_read(vector_irq[vector]);
per_cpu(vector_irq, CPU1)[vector] = desc;
if (d == VECTOR_SHUTDOWN)
this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
free_irq() cannot observe the pending vector in the CPU1 APIC as there is
no way to query the remote CPUs APIC IRR.
This requires that request_irq() uses the same vector/CPU as the one which
was freed, but this also can be triggered by a spurious interrupt.
Interestingly enough this problem managed to be hidden for more than a
decade.
Prevent this by reevaluating vector_irq under the vector lock, which is
held by the interrupt activation code when vector_irq is updated.
To avoid ifdeffery or IS_ENABLED() nonsense, repair the broken config
dependency in hw_irq.h, which covers [un]lock_vector_lock().
It has to depend on CONFIG_X86_LOCAL_APIC as that's what makes the
functions available or stubbed out. The current CONFIG_IRQ_DOMAIN_HIERARCHY
guard is selected by CONFIG_X86_LOCAL_APIC, but can also be selected by
other parts of the Kconfig system, which makes 32-bit UP builds with
CONFIG_X86_LOCAL_APIC=n fail.
Can we just get rid of this !APIC nonsense once and forever?
Fixes: 9345005f4eed ("x86/irq: Fix do_IRQ() interrupt warning for cpu hotplug retriggered irqs")
Reported-by: Hogan Wang <hogan.wang@huawei.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Hogan Wang <hogan.wang@huawei.com>
Link: https://lore.kernel.org/all/draft-87ikjhrhhh.ffs@tglx
---
arch/x86/include/asm/hw_irq.h | 6 +--
arch/x86/kernel/irq.c | 63 +++++++++++++++++++++++++---------
2 files changed, 51 insertions(+), 18 deletions(-)
diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
index 162ebd7..99f7bf9 100644
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -26,7 +26,7 @@
#include <asm/irq.h>
#include <asm/sections.h>
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+#ifdef CONFIG_X86_LOCAL_APIC
struct irq_data;
struct pci_dev;
struct msi_desc;
@@ -103,10 +103,10 @@ static inline void irq_complete_move(struct irq_cfg *c) { }
#endif
extern void apic_ack_edge(struct irq_data *data);
-#else /* CONFIG_IRQ_DOMAIN_HIERARCHY */
+#else /* CONFIG_X86_LOCAL_APIC */
static inline void lock_vector_lock(void) {}
static inline void unlock_vector_lock(void) {}
-#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
+#endif /* !CONFIG_X86_LOCAL_APIC */
/* Statistics */
extern atomic_t irq_err_count;
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 9ed29ff..10721a1 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -256,26 +256,59 @@ static __always_inline void handle_irq(struct irq_desc *desc,
__handle_irq(desc, regs);
}
-static __always_inline int call_irq_handler(int vector, struct pt_regs *regs)
+static struct irq_desc *reevaluate_vector(int vector)
{
- struct irq_desc *desc;
- int ret = 0;
+ struct irq_desc *desc = __this_cpu_read(vector_irq[vector]);
+
+ if (!IS_ERR_OR_NULL(desc))
+ return desc;
+
+ if (desc == VECTOR_UNUSED)
+ pr_emerg_ratelimited("No irq handler for %d.%u\n", smp_processor_id(), vector);
+ else
+ __this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
+ return NULL;
+}
+
+static __always_inline bool call_irq_handler(int vector, struct pt_regs *regs)
+{
+ struct irq_desc *desc = __this_cpu_read(vector_irq[vector]);
- desc = __this_cpu_read(vector_irq[vector]);
if (likely(!IS_ERR_OR_NULL(desc))) {
handle_irq(desc, regs);
- } else {
- ret = -EINVAL;
- if (desc == VECTOR_UNUSED) {
- pr_emerg_ratelimited("%s: %d.%u No irq handler for vector\n",
- __func__, smp_processor_id(),
- vector);
- } else {
- __this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
- }
+ return true;
}
- return ret;
+ /*
+ * Reevaluate with vector_lock held to prevent a race against
+ * request_irq() setting up the vector:
+ *
+ * CPU0 CPU1
+ * interrupt is raised in APIC IRR
+ * but not handled
+ * free_irq()
+ * per_cpu(vector_irq, CPU1)[vector] = VECTOR_SHUTDOWN;
+ *
+ * request_irq() common_interrupt()
+ * d = this_cpu_read(vector_irq[vector]);
+ *
+ * per_cpu(vector_irq, CPU1)[vector] = desc;
+ *
+ * if (d == VECTOR_SHUTDOWN)
+ * this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
+ *
+ * This requires that the same vector on the same target CPU is
+ * handed out or that a spurious interrupt hits that CPU/vector.
+ */
+ lock_vector_lock();
+ desc = reevaluate_vector(vector);
+ unlock_vector_lock();
+
+ if (!desc)
+ return false;
+
+ handle_irq(desc, regs);
+ return true;
}
/*
@@ -289,7 +322,7 @@ DEFINE_IDTENTRY_IRQ(common_interrupt)
/* entry code tells RCU that we're not quiescent. Check it. */
RCU_LOCKDEP_WARN(!rcu_is_watching(), "IRQ failed to wake up RCU");
- if (unlikely(call_irq_handler(vector, regs)))
+ if (unlikely(!call_irq_handler(vector, regs)))
apic_eoi();
set_irq_regs(old_regs);
^ permalink raw reply related [flat|nested] 6+ messages in thread* [tip: x86/urgent] x86/irq: Plug vector setup race
[not found] <draft-87ikjhrhhh.ffs@tglx>
` (2 preceding siblings ...)
2025-08-02 20:08 ` tip-bot2 for Thomas Gleixner
@ 2025-08-04 22:42 ` tip-bot2 for Thomas Gleixner
3 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2025-08-04 22:42 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Hogan Wang, Thomas Gleixner, x86, linux-kernel
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: ce0b5eedcb753697d43f61dd2e27d68eb5d3150f
Gitweb: https://git.kernel.org/tip/ce0b5eedcb753697d43f61dd2e27d68eb5d3150f
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 24 Jul 2025 12:49:30 +02:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 04 Aug 2025 23:34:03 +02:00
x86/irq: Plug vector setup race
Hogan reported a vector setup race, which overwrites the interrupt
descriptor in the per CPU vector array resulting in a disfunctional device.
CPU0 CPU1
interrupt is raised in APIC IRR
but not handled
free_irq()
per_cpu(vector_irq, CPU1)[vector] = VECTOR_SHUTDOWN;
request_irq() common_interrupt()
d = this_cpu_read(vector_irq[vector]);
per_cpu(vector_irq, CPU1)[vector] = desc;
if (d == VECTOR_SHUTDOWN)
this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
free_irq() cannot observe the pending vector in the CPU1 APIC as there is
no way to query the remote CPUs APIC IRR.
This requires that request_irq() uses the same vector/CPU as the one which
was freed, but this also can be triggered by a spurious interrupt.
Interestingly enough this problem managed to be hidden for more than a
decade.
Prevent this by reevaluating vector_irq under the vector lock, which is
held by the interrupt activation code when vector_irq is updated.
To avoid ifdeffery or IS_ENABLED() nonsense, move the
[un]lock_vector_lock() declarations out under the
CONFIG_IRQ_DOMAIN_HIERARCHY guard as it's only provided when
CONFIG_X86_LOCAL_APIC=y.
The current CONFIG_IRQ_DOMAIN_HIERARCHY guard is selected by
CONFIG_X86_LOCAL_APIC, but can also be selected by other parts of the
Kconfig system, which makes 32-bit UP builds with CONFIG_X86_LOCAL_APIC=n
fail.
Can we just get rid of this !APIC nonsense once and forever?
Fixes: 9345005f4eed ("x86/irq: Fix do_IRQ() interrupt warning for cpu hotplug retriggered irqs")
Reported-by: Hogan Wang <hogan.wang@huawei.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Hogan Wang <hogan.wang@huawei.com>
Link: https://lore.kernel.org/all/draft-87ikjhrhhh.ffs@tglx
---
arch/x86/include/asm/hw_irq.h | 12 +++---
arch/x86/kernel/irq.c | 63 +++++++++++++++++++++++++---------
2 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
index 162ebd7..cbe19e6 100644
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -92,8 +92,6 @@ struct irq_cfg {
extern struct irq_cfg *irq_cfg(unsigned int irq);
extern struct irq_cfg *irqd_cfg(struct irq_data *irq_data);
-extern void lock_vector_lock(void);
-extern void unlock_vector_lock(void);
#ifdef CONFIG_SMP
extern void vector_schedule_cleanup(struct irq_cfg *);
extern void irq_complete_move(struct irq_cfg *cfg);
@@ -101,12 +99,16 @@ extern void irq_complete_move(struct irq_cfg *cfg);
static inline void vector_schedule_cleanup(struct irq_cfg *c) { }
static inline void irq_complete_move(struct irq_cfg *c) { }
#endif
-
extern void apic_ack_edge(struct irq_data *data);
-#else /* CONFIG_IRQ_DOMAIN_HIERARCHY */
+#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
+
+#ifdef CONFIG_X86_LOCAL_APIC
+extern void lock_vector_lock(void);
+extern void unlock_vector_lock(void);
+#else
static inline void lock_vector_lock(void) {}
static inline void unlock_vector_lock(void) {}
-#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
+#endif
/* Statistics */
extern atomic_t irq_err_count;
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 9ed29ff..10721a1 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -256,26 +256,59 @@ static __always_inline void handle_irq(struct irq_desc *desc,
__handle_irq(desc, regs);
}
-static __always_inline int call_irq_handler(int vector, struct pt_regs *regs)
+static struct irq_desc *reevaluate_vector(int vector)
{
- struct irq_desc *desc;
- int ret = 0;
+ struct irq_desc *desc = __this_cpu_read(vector_irq[vector]);
+
+ if (!IS_ERR_OR_NULL(desc))
+ return desc;
+
+ if (desc == VECTOR_UNUSED)
+ pr_emerg_ratelimited("No irq handler for %d.%u\n", smp_processor_id(), vector);
+ else
+ __this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
+ return NULL;
+}
+
+static __always_inline bool call_irq_handler(int vector, struct pt_regs *regs)
+{
+ struct irq_desc *desc = __this_cpu_read(vector_irq[vector]);
- desc = __this_cpu_read(vector_irq[vector]);
if (likely(!IS_ERR_OR_NULL(desc))) {
handle_irq(desc, regs);
- } else {
- ret = -EINVAL;
- if (desc == VECTOR_UNUSED) {
- pr_emerg_ratelimited("%s: %d.%u No irq handler for vector\n",
- __func__, smp_processor_id(),
- vector);
- } else {
- __this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
- }
+ return true;
}
- return ret;
+ /*
+ * Reevaluate with vector_lock held to prevent a race against
+ * request_irq() setting up the vector:
+ *
+ * CPU0 CPU1
+ * interrupt is raised in APIC IRR
+ * but not handled
+ * free_irq()
+ * per_cpu(vector_irq, CPU1)[vector] = VECTOR_SHUTDOWN;
+ *
+ * request_irq() common_interrupt()
+ * d = this_cpu_read(vector_irq[vector]);
+ *
+ * per_cpu(vector_irq, CPU1)[vector] = desc;
+ *
+ * if (d == VECTOR_SHUTDOWN)
+ * this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
+ *
+ * This requires that the same vector on the same target CPU is
+ * handed out or that a spurious interrupt hits that CPU/vector.
+ */
+ lock_vector_lock();
+ desc = reevaluate_vector(vector);
+ unlock_vector_lock();
+
+ if (!desc)
+ return false;
+
+ handle_irq(desc, regs);
+ return true;
}
/*
@@ -289,7 +322,7 @@ DEFINE_IDTENTRY_IRQ(common_interrupt)
/* entry code tells RCU that we're not quiescent. Check it. */
RCU_LOCKDEP_WARN(!rcu_is_watching(), "IRQ failed to wake up RCU");
- if (unlikely(call_irq_handler(vector, regs)))
+ if (unlikely(!call_irq_handler(vector, regs)))
apic_eoi();
set_irq_regs(old_regs);
^ permalink raw reply related [flat|nested] 6+ messages in thread