* x86: Spurious vectors not handled robustly @ 2019-06-24 10:00 Jan Kiszka 2019-06-24 10:09 ` Thomas Gleixner 0 siblings, 1 reply; 6+ messages in thread From: Jan Kiszka @ 2019-06-24 10:00 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86 Cc: Linux Kernel Mailing List Hi all, probably since "x86: Avoid building unused IRQ entry stubs" (2414e021ac8d), the kernel can no longer tell spurious IRQs by the APIC apart from spuriously triggered unused vectors. We've managed to trigger such a cause with the Jailhouse hypervisor (incorrectly injected MANAGED_IRQ_SHUTDOWN_VECTOR), and the result was not only a misreport of the vector number (0xff instead of 0xef - took me a while...), but also stalled interrupts of equal and lower priority because a spurious interrupt is not (and must not be) acknowledged. How to address that best? I would say we should at least have separate entry paths for APIC interrupt vs. vectors, to improve robustness in the faulty case. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: x86: Spurious vectors not handled robustly 2019-06-24 10:00 x86: Spurious vectors not handled robustly Jan Kiszka @ 2019-06-24 10:09 ` Thomas Gleixner 2019-06-24 10:21 ` Jan Kiszka 0 siblings, 1 reply; 6+ messages in thread From: Thomas Gleixner @ 2019-06-24 10:09 UTC (permalink / raw) To: Jan Kiszka; +Cc: Ingo Molnar, Borislav Petkov, x86, Linux Kernel Mailing List Jan, On Mon, 24 Jun 2019, Jan Kiszka wrote: > probably since "x86: Avoid building unused IRQ entry stubs" (2414e021ac8d), > the kernel can no longer tell spurious IRQs by the APIC apart from spuriously > triggered unused vectors. Err. It does. > We've managed to trigger such a cause with the Jailhouse hypervisor > (incorrectly injected MANAGED_IRQ_SHUTDOWN_VECTOR), and the result was > not only a misreport of the vector number (0xff instead of 0xef - took me > a while...), but also stalled interrupts of equal and lower priority > because a spurious interrupt is not (and must not be) acknowledged. That does not make sense. __visible void __irq_entry smp_spurious_interrupt(struct pt_regs *regs) { u8 vector = ~regs->orig_ax; u32 v; entering_irq(); trace_spurious_apic_entry(vector); /* * Check if this really is a spurious interrupt and ACK it * if it is a vectored one. Just in case... */ v = apic_read(APIC_ISR + ((vector & ~0x1f) >> 1)); if (v & (1 << (vector & 0x1f))) ack_APIC_irq(); If it is a vectored one it _IS_ acked. inc_irq_stat(irq_spurious_count); /* see sw-dev-man vol 3, chapter 7.4.13.5 */ pr_info("spurious APIC interrupt through vector %02x on CPU#%d, " "should never happen.\n", vector, smp_processor_id()); and the vector through which that comes is printed correctly, unless regs->orig_ax is hosed. Thanks, tglx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: x86: Spurious vectors not handled robustly 2019-06-24 10:09 ` Thomas Gleixner @ 2019-06-24 10:21 ` Jan Kiszka 2019-06-24 10:37 ` Thomas Gleixner 0 siblings, 1 reply; 6+ messages in thread From: Jan Kiszka @ 2019-06-24 10:21 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo Molnar, Borislav Petkov, x86, Linux Kernel Mailing List On 24.06.19 12:09, Thomas Gleixner wrote: > Jan, > > On Mon, 24 Jun 2019, Jan Kiszka wrote: >> probably since "x86: Avoid building unused IRQ entry stubs" (2414e021ac8d), >> the kernel can no longer tell spurious IRQs by the APIC apart from spuriously >> triggered unused vectors. > > Err. It does. > >> We've managed to trigger such a cause with the Jailhouse hypervisor >> (incorrectly injected MANAGED_IRQ_SHUTDOWN_VECTOR), and the result was >> not only a misreport of the vector number (0xff instead of 0xef - took me >> a while...), but also stalled interrupts of equal and lower priority >> because a spurious interrupt is not (and must not be) acknowledged. > > That does not make sense. > > __visible void __irq_entry smp_spurious_interrupt(struct pt_regs *regs) > { > u8 vector = ~regs->orig_ax; > u32 v; > > entering_irq(); > trace_spurious_apic_entry(vector); > /* > * Check if this really is a spurious interrupt and ACK it > * if it is a vectored one. Just in case... > */ > v = apic_read(APIC_ISR + ((vector & ~0x1f) >> 1)); > if (v & (1 << (vector & 0x1f))) > ack_APIC_irq(); > > If it is a vectored one it _IS_ acked. > > inc_irq_stat(irq_spurious_count); > > /* see sw-dev-man vol 3, chapter 7.4.13.5 */ > pr_info("spurious APIC interrupt through vector %02x on CPU#%d, " > "should never happen.\n", vector, smp_processor_id()); > > and the vector through which that comes is printed correctly, unless > regs->orig_ax is hosed. ...which is exactly the case: Since that commit, all unused vectors share the same entry point, spurious_interrupt, see idt_setup_apic_and_irq_gates(). And that entry point sets orig_ax to ~0xff. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: x86: Spurious vectors not handled robustly 2019-06-24 10:21 ` Jan Kiszka @ 2019-06-24 10:37 ` Thomas Gleixner 2019-06-24 13:46 ` Thomas Gleixner 0 siblings, 1 reply; 6+ messages in thread From: Thomas Gleixner @ 2019-06-24 10:37 UTC (permalink / raw) To: Jan Kiszka; +Cc: Ingo Molnar, Borislav Petkov, x86, Linux Kernel Mailing List On Mon, 24 Jun 2019, Jan Kiszka wrote: > On 24.06.19 12:09, Thomas Gleixner wrote: > > If it is a vectored one it _IS_ acked. > > > > inc_irq_stat(irq_spurious_count); > > > > /* see sw-dev-man vol 3, chapter 7.4.13.5 */ > > pr_info("spurious APIC interrupt through vector %02x on CPU#%d, " > > "should never happen.\n", vector, smp_processor_id()); > > > > and the vector through which that comes is printed correctly, unless > > regs->orig_ax is hosed. > > ...which is exactly the case: Since that commit, all unused vectors share the > same entry point, spurious_interrupt, see idt_setup_apic_and_irq_gates(). And > that entry point sets orig_ax to ~0xff. Bah. Of course I did not look at that ... /me goes back to stare at the code. Thanks, tglx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: x86: Spurious vectors not handled robustly 2019-06-24 10:37 ` Thomas Gleixner @ 2019-06-24 13:46 ` Thomas Gleixner 2019-06-24 15:26 ` Thomas Gleixner 0 siblings, 1 reply; 6+ messages in thread From: Thomas Gleixner @ 2019-06-24 13:46 UTC (permalink / raw) To: Jan Kiszka Cc: Ingo Molnar, Borislav Petkov, x86, Linux Kernel Mailing List, Jan Beulich On Mon, 24 Jun 2019, Thomas Gleixner wrote: > On Mon, 24 Jun 2019, Jan Kiszka wrote: > > On 24.06.19 12:09, Thomas Gleixner wrote: > > > If it is a vectored one it _IS_ acked. > > > > > > inc_irq_stat(irq_spurious_count); > > > > > > /* see sw-dev-man vol 3, chapter 7.4.13.5 */ > > > pr_info("spurious APIC interrupt through vector %02x on CPU#%d, " > > > "should never happen.\n", vector, smp_processor_id()); > > > > > > and the vector through which that comes is printed correctly, unless > > > regs->orig_ax is hosed. > > > > ...which is exactly the case: Since that commit, all unused vectors share the > > same entry point, spurious_interrupt, see idt_setup_apic_and_irq_gates(). And > > that entry point sets orig_ax to ~0xff. > > Bah. Of course I did not look at that ... > > /me goes back to stare at the code. The patch below should fix it. It's quick tested on 64bit (without inducing a spurious vector) and compile tested on 32bit. Can you please give it a try with your failure case? Thanks, tglx 8<---------------- Subject: x86/irq: Seperate unused system vectors from spurious entry again From: Thomas Gleixner <tglx@linutronix.de> Date: Mon, 24 Jun 2019 13:34:06 +0200 Quite some time ago the interrupt entry stubs for unused vectors in the system vector range got removed and directly mapped to the spurious interrupt vector entry point. Sounds reasonable, but it's subtly broken. The spurious interrupt vector entry point pushes vector number 0xFF on the stack which makes the whole logic in smp_spurious_interrupt() pointless. As a consequence any spurious interrupt which comes from a vector != 0xFF is treated as a real spurious interrupt (vector 0xFF) and not acknowledged. That subsequently stalls all interrupt vectors of equal and lower priority, which brings the system to a grinding halt. This can happen because even on 64-bit the system vector space is not guaranteed to be fully populated. A full compile time handling of the unused vectors is not possible because quite some of them are conditonally populated at runtime. Bring the entry stubs back, which wastes 160 bytes if all stubs are unused, but gains the proper handling back. There is no point to selectively spare some of the stubs which are known at compile time as the required code in the IDT management would be way larger and convoluted. The array is straight forward and simple. Do not route the spurious entries through common_interrupt and do_IRQ() as the original code did. Route it to smp_spurious_interrupt() which evaluates the vector number and acts accordingly now that the real vector numbers are handed in. Fixes: 2414e021ac8d ("x86: Avoid building unused IRQ entry stubs") Reported-by: Jan Kiszka <jan.kiszka@siemens.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: Jan Beulich <jbeulich@suse.com> --- arch/x86/entry/entry_32.S | 24 ++++++++++++++++++++++++ arch/x86/entry/entry_64.S | 30 ++++++++++++++++++++++++++---- arch/x86/include/asm/hw_irq.h | 2 ++ arch/x86/kernel/idt.c | 3 ++- 4 files changed, 54 insertions(+), 5 deletions(-) --- a/arch/x86/entry/entry_32.S +++ b/arch/x86/entry/entry_32.S @@ -1104,6 +1104,30 @@ ENTRY(irq_entries_start) .endr END(irq_entries_start) +#ifdef CONFIG_X86_LOCAL_APIC + .align 8 +ENTRY(spurious_entries_start) + vector=FIRST_SYSTEM_VECTOR + .rept (NR_VECTORS - FIRST_SYSTEM_VECTOR) + pushl $(~vector+0x80) /* Note: always in signed byte range */ + vector=vector+1 + jmp common_spurious_vector + .align 8 + .endr +END(spurious_entries_start) + +common_spurious: + ASM_CLAC + addl $-0x80, (%esp) /* Adjust vector into the [-256, -1] range */ + SAVE_ALL switch_stacks=1 + ENCODE_FRAME_POINTER + TRACE_IRQS_OFF + movl %esp, %eax + call smp_spurious_interrupt + jmp ret_from_intr +ENDPROC(common_interrupt) +#endif + /* * the CPU automatically disables interrupts when executing an IRQ vector, * so IRQ-flags tracing has to follow that: --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -375,6 +375,18 @@ ENTRY(irq_entries_start) .endr END(irq_entries_start) + .align 8 +ENTRY(spurious_entries_start) + vector=FIRST_SYSTEM_VECTOR + .rept (NR_VECTORS - FIRST_SYSTEM_VECTOR) + UNWIND_HINT_IRET_REGS + pushq $(~vector+0x80) /* Note: always in signed byte range */ + jmp common_spurious + .align 8 + vector=vector+1 + .endr +END(spurious_entries_start) + .macro DEBUG_ENTRY_ASSERT_IRQS_OFF #ifdef CONFIG_DEBUG_ENTRY pushq %rax @@ -571,10 +583,20 @@ END(interrupt_entry) /* Interrupt entry/exit. */ - /* - * The interrupt stubs push (~vector+0x80) onto the stack and - * then jump to common_interrupt. - */ +/* + * The interrupt stubs push (~vector+0x80) onto the stack and + * then jump to common_spurious/interrupt. + */ +common_spurious: + addq $-0x80, (%rsp) /* Adjust vector to [-256, -1] range */ + call interrupt_entry + UNWIND_HINT_REGS indirect=1 + call smp_spurious_interrupt /* rdi points to pt_regs */ + jmp ret_from_intr +END(common_spurious) +_ASM_NOKPROBE(common_spurious) + +/* common_interrupt is a hotpath. Align it */ .p2align CONFIG_X86_L1_CACHE_SHIFT common_interrupt: addq $-0x80, (%rsp) /* Adjust vector to [-256, -1] range */ --- a/arch/x86/include/asm/hw_irq.h +++ b/arch/x86/include/asm/hw_irq.h @@ -150,6 +150,8 @@ extern char irq_entries_start[]; #define trace_irq_entries_start irq_entries_start #endif +extern char spurious_entries_start[]; + #define VECTOR_UNUSED NULL #define VECTOR_RETRIGGERED ((void *)~0UL) --- a/arch/x86/kernel/idt.c +++ b/arch/x86/kernel/idt.c @@ -319,7 +319,8 @@ void __init idt_setup_apic_and_irq_gates #ifdef CONFIG_X86_LOCAL_APIC for_each_clear_bit_from(i, system_vectors, NR_VECTORS) { set_bit(i, system_vectors); - set_intr_gate(i, spurious_interrupt); + entry = spurious_entries_start + 8 * (i - FIRST_SYSTEM_VECTOR); + set_intr_gate(i, entry); } #endif } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: x86: Spurious vectors not handled robustly 2019-06-24 13:46 ` Thomas Gleixner @ 2019-06-24 15:26 ` Thomas Gleixner 0 siblings, 0 replies; 6+ messages in thread From: Thomas Gleixner @ 2019-06-24 15:26 UTC (permalink / raw) To: Jan Kiszka Cc: Ingo Molnar, Borislav Petkov, x86, Linux Kernel Mailing List, Jan Beulich On Mon, 24 Jun 2019, Thomas Gleixner wrote: > > +#ifdef CONFIG_X86_LOCAL_APIC > + .align 8 > +ENTRY(spurious_entries_start) > + vector=FIRST_SYSTEM_VECTOR > + .rept (NR_VECTORS - FIRST_SYSTEM_VECTOR) > + pushl $(~vector+0x80) /* Note: always in signed byte range */ > + vector=vector+1 > + jmp common_spurious_vector Moo. Not syncing the compile machine and the laptop! That should obviously be + jmp common_spurious > + .align 8 > + .endr > +END(spurious_entries_start) > + > +common_spurious: > + ASM_CLAC > + addl $-0x80, (%esp) /* Adjust vector into the [-256, -1] range */ > + SAVE_ALL switch_stacks=1 > + ENCODE_FRAME_POINTER > + TRACE_IRQS_OFF > + movl %esp, %eax > + call smp_spurious_interrupt > + jmp ret_from_intr > +ENDPROC(common_interrupt) > +#endif > + > /* > * the CPU automatically disables interrupts when executing an IRQ vector, > * so IRQ-flags tracing has to follow that: > --- a/arch/x86/entry/entry_64.S > +++ b/arch/x86/entry/entry_64.S > @@ -375,6 +375,18 @@ ENTRY(irq_entries_start) > .endr > END(irq_entries_start) > > + .align 8 > +ENTRY(spurious_entries_start) > + vector=FIRST_SYSTEM_VECTOR > + .rept (NR_VECTORS - FIRST_SYSTEM_VECTOR) > + UNWIND_HINT_IRET_REGS > + pushq $(~vector+0x80) /* Note: always in signed byte range */ > + jmp common_spurious > + .align 8 > + vector=vector+1 > + .endr > +END(spurious_entries_start) > + > .macro DEBUG_ENTRY_ASSERT_IRQS_OFF > #ifdef CONFIG_DEBUG_ENTRY > pushq %rax > @@ -571,10 +583,20 @@ END(interrupt_entry) > > /* Interrupt entry/exit. */ > > - /* > - * The interrupt stubs push (~vector+0x80) onto the stack and > - * then jump to common_interrupt. > - */ > +/* > + * The interrupt stubs push (~vector+0x80) onto the stack and > + * then jump to common_spurious/interrupt. > + */ > +common_spurious: > + addq $-0x80, (%rsp) /* Adjust vector to [-256, -1] range */ > + call interrupt_entry > + UNWIND_HINT_REGS indirect=1 > + call smp_spurious_interrupt /* rdi points to pt_regs */ > + jmp ret_from_intr > +END(common_spurious) > +_ASM_NOKPROBE(common_spurious) > + > +/* common_interrupt is a hotpath. Align it */ > .p2align CONFIG_X86_L1_CACHE_SHIFT > common_interrupt: > addq $-0x80, (%rsp) /* Adjust vector to [-256, -1] range */ > --- a/arch/x86/include/asm/hw_irq.h > +++ b/arch/x86/include/asm/hw_irq.h > @@ -150,6 +150,8 @@ extern char irq_entries_start[]; > #define trace_irq_entries_start irq_entries_start > #endif > > +extern char spurious_entries_start[]; > + > #define VECTOR_UNUSED NULL > #define VECTOR_RETRIGGERED ((void *)~0UL) > > --- a/arch/x86/kernel/idt.c > +++ b/arch/x86/kernel/idt.c > @@ -319,7 +319,8 @@ void __init idt_setup_apic_and_irq_gates > #ifdef CONFIG_X86_LOCAL_APIC > for_each_clear_bit_from(i, system_vectors, NR_VECTORS) { > set_bit(i, system_vectors); > - set_intr_gate(i, spurious_interrupt); > + entry = spurious_entries_start + 8 * (i - FIRST_SYSTEM_VECTOR); > + set_intr_gate(i, entry); > } > #endif > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-24 15:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-24 10:00 x86: Spurious vectors not handled robustly Jan Kiszka 2019-06-24 10:09 ` Thomas Gleixner 2019-06-24 10:21 ` Jan Kiszka 2019-06-24 10:37 ` Thomas Gleixner 2019-06-24 13:46 ` Thomas Gleixner 2019-06-24 15:26 ` Thomas Gleixner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).