* [PATCH 1/3] powerpc: kdump: Fix NULL pointer dereference in irq disable code @ 2010-05-11 2:23 Anton Blanchard 2010-05-11 2:25 ` [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU Anton Blanchard 2010-05-11 4:14 ` [PATCH 1/3] powerpc: kdump: Fix NULL pointer dereference in irq disable code Michael Ellerman 0 siblings, 2 replies; 6+ messages in thread From: Anton Blanchard @ 2010-05-11 2:23 UTC (permalink / raw) To: benh, mikey, michael, miltonm; +Cc: linuxppc-dev With sparse irqs we have to check if we have a descriptor before dereferencing it. Signed-off-by: Anton Blanchard <anton@samba.org> --- diff --git a/arch/powerpc/kernel/crash.c b/arch/powerpc/kernel/crash.c index 6f4613d..5182439 100644 --- a/arch/powerpc/kernel/crash.c +++ b/arch/powerpc/kernel/crash.c @@ -375,6 +375,9 @@ void default_machine_crash_shutdown(struct pt_regs *regs) for_each_irq(i) { struct irq_desc *desc = irq_to_desc(i); + if (!desc) + continue; + if (desc->status & IRQ_INPROGRESS) desc->chip->eoi(i); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU 2010-05-11 2:23 [PATCH 1/3] powerpc: kdump: Fix NULL pointer dereference in irq disable code Anton Blanchard @ 2010-05-11 2:25 ` Anton Blanchard 2010-05-11 2:27 ` [PATCH 3/3] powerpc: kdump: Use chip->shutdown to disable IRQs Anton Blanchard 2010-05-11 4:14 ` [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU Michael Ellerman 2010-05-11 4:14 ` [PATCH 1/3] powerpc: kdump: Fix NULL pointer dereference in irq disable code Michael Ellerman 1 sibling, 2 replies; 6+ messages in thread From: Anton Blanchard @ 2010-05-11 2:25 UTC (permalink / raw) To: benh, mikey, miltonm, michael, paulus; +Cc: linuxppc-dev We wrap the crash_shutdown_handles[] calls with longjmp/setjmp, so if any of them fault we can recover. The problem is we add a hook to the debugger fault handler hook which calls longjmp unconditionally. This first part of kdump is run before we marshall the other CPUs, so there is a very good chance some CPU on the box is going to page fault. And when it does it hits the longjmp code and assumes the context of the oopsing CPU. The machine gets very confused when it has 10 CPUs all with the same stack, all thinking they have the same CPU id. I get even more confused trying to debug it. The patch below adds crash_shutdown_cpu and uses it to specify which cpu is in the protected region. Since it can only be -1 or the oopsing CPU, we don't need to use memory barriers since it is only valid on the local CPU - no other CPU will ever see a value that matches it's local CPU id. Eventually we should switch the order and marshall all CPUs before doing the crash_shutdown_handles[] calls, but that is a bigger fix. Signed-off-by: Anton Blanchard <anton@samba.org> --- Index: linux-2.6/arch/powerpc/kernel/crash.c =================================================================== --- linux-2.6.orig/arch/powerpc/kernel/crash.c 2010-05-10 23:48:35.775954185 +1000 +++ linux-2.6/arch/powerpc/kernel/crash.c 2010-05-11 08:39:14.423453660 +1000 @@ -281,10 +281,12 @@ int crash_shutdown_unregister(crash_shut EXPORT_SYMBOL(crash_shutdown_unregister); static unsigned long crash_shutdown_buf[JMP_BUF_LEN]; +static int crash_shutdown_cpu = -1; static int handle_fault(struct pt_regs *regs) { - longjmp(crash_shutdown_buf, 1); + if (crash_shutdown_cpu == smp_processor_id()) + longjmp(crash_shutdown_buf, 1); return 0; } @@ -325,6 +327,7 @@ void default_machine_crash_shutdown(stru */ old_handler = __debugger_fault_handler; __debugger_fault_handler = handle_fault; + crash_shutdown_cpu = smp_processor_id(); for (i = 0; crash_shutdown_handles[i]; i++) { if (setjmp(crash_shutdown_buf) == 0) { /* @@ -338,6 +341,7 @@ void default_machine_crash_shutdown(stru asm volatile("sync; isync"); } } + crash_shutdown_cpu = -1; __debugger_fault_handler = old_handler; /* ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] powerpc: kdump: Use chip->shutdown to disable IRQs 2010-05-11 2:25 ` [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU Anton Blanchard @ 2010-05-11 2:27 ` Anton Blanchard 2010-05-11 4:11 ` Michael Ellerman 2010-05-11 4:14 ` [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU Michael Ellerman 1 sibling, 1 reply; 6+ messages in thread From: Anton Blanchard @ 2010-05-11 2:27 UTC (permalink / raw) To: benh, mikey, miltonm, michael, paulus; +Cc: linuxppc-dev I saw this in a kdump kernel: IOMMU table initialized, virtual merging enabled Interrupt 155954 (real) is invalid, disabling it. Interrupt 155953 (real) is invalid, disabling it. ie we took some spurious interrupts. default_machine_crash_shutdown tries to disable all interrupt sources but uses chip->disable which maps to the default action of: static void default_disable(unsigned int irq) { } If we use chip->shutdown, then we actually mask the IRQ: static void default_shutdown(unsigned int irq) { struct irq_desc *desc = irq_to_desc(irq); desc->chip->mask(irq); desc->status |= IRQ_MASKED; } Not sure why we don't implement a ->disable action for xics.c, or why default_disable doesn't mask the interrupt. Signed-off-by: Anton Blanchard <anton@samba.org> --- Index: linux-2.6/arch/powerpc/kernel/crash.c =================================================================== --- linux-2.6.orig/arch/powerpc/kernel/crash.c 2010-05-10 23:43:10.445953883 +1000 +++ linux-2.6/arch/powerpc/kernel/crash.c 2010-05-10 23:43:21.223454012 +1000 @@ -315,7 +315,7 @@ void default_machine_crash_shutdown(stru desc->chip->eoi(i); if (!(desc->status & IRQ_DISABLED)) - desc->chip->disable(i); + desc->chip->shutdown(i); } /* ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] powerpc: kdump: Use chip->shutdown to disable IRQs 2010-05-11 2:27 ` [PATCH 3/3] powerpc: kdump: Use chip->shutdown to disable IRQs Anton Blanchard @ 2010-05-11 4:11 ` Michael Ellerman 0 siblings, 0 replies; 6+ messages in thread From: Michael Ellerman @ 2010-05-11 4:11 UTC (permalink / raw) To: Anton Blanchard; +Cc: mikey, paulus, miltonm, linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 1120 bytes --] On Tue, 2010-05-11 at 12:27 +1000, Anton Blanchard wrote: > I saw this in a kdump kernel: > > IOMMU table initialized, virtual merging enabled > Interrupt 155954 (real) is invalid, disabling it. > Interrupt 155953 (real) is invalid, disabling it. > > ie we took some spurious interrupts. OK, but it should have still worked OK? > default_machine_crash_shutdown tries > to disable all interrupt sources but uses chip->disable which maps to > the default action of: > > static void default_disable(unsigned int irq) > { > } > > If we use chip->shutdown, then we actually mask the IRQ: > > static void default_shutdown(unsigned int irq) > { > struct irq_desc *desc = irq_to_desc(irq); > > desc->chip->mask(irq); > desc->status |= IRQ_MASKED; > } > > Not sure why we don't implement a ->disable action for xics.c, or why > default_disable doesn't mask the interrupt. It used to mask, see 76d21601, I knew that would bite us somewhere. Not 100% sure about the change to use shutdown, but it's probably sane and you've tested it so cool :) cheers [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU 2010-05-11 2:25 ` [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU Anton Blanchard 2010-05-11 2:27 ` [PATCH 3/3] powerpc: kdump: Use chip->shutdown to disable IRQs Anton Blanchard @ 2010-05-11 4:14 ` Michael Ellerman 1 sibling, 0 replies; 6+ messages in thread From: Michael Ellerman @ 2010-05-11 4:14 UTC (permalink / raw) To: Anton Blanchard; +Cc: mikey, paulus, miltonm, linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 771 bytes --] On Tue, 2010-05-11 at 12:25 +1000, Anton Blanchard wrote: > We wrap the crash_shutdown_handles[] calls with longjmp/setjmp, so if any > of them fault we can recover. The problem is we add a hook to the debugger > fault handler hook which calls longjmp unconditionally. > > This first part of kdump is run before we marshall the other CPUs, so there > is a very good chance some CPU on the box is going to page fault. And when > it does it hits the longjmp code and assumes the context of the oopsing CPU. > The machine gets very confused when it has 10 CPUs all with the same stack, > all thinking they have the same CPU id. I get even more confused trying > to debug it. Lol, guess that one didn't get tested that well :) Fix looks good. cheers [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] powerpc: kdump: Fix NULL pointer dereference in irq disable code 2010-05-11 2:23 [PATCH 1/3] powerpc: kdump: Fix NULL pointer dereference in irq disable code Anton Blanchard 2010-05-11 2:25 ` [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU Anton Blanchard @ 2010-05-11 4:14 ` Michael Ellerman 1 sibling, 0 replies; 6+ messages in thread From: Michael Ellerman @ 2010-05-11 4:14 UTC (permalink / raw) To: Anton Blanchard; +Cc: mikey, miltonm, linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 690 bytes --] On Tue, 2010-05-11 at 12:23 +1000, Anton Blanchard wrote: > With sparse irqs we have to check if we have a descriptor before dereferencing > it. > > Signed-off-by: Anton Blanchard <anton@samba.org> > --- > > diff --git a/arch/powerpc/kernel/crash.c b/arch/powerpc/kernel/crash.c > index 6f4613d..5182439 100644 > --- a/arch/powerpc/kernel/crash.c > +++ b/arch/powerpc/kernel/crash.c > @@ -375,6 +375,9 @@ void default_machine_crash_shutdown(struct pt_regs *regs) > for_each_irq(i) { > struct irq_desc *desc = irq_to_desc(i); > > + if (!desc) > + continue; > + > if (desc->status & IRQ_INPROGRESS) > desc->chip->eoi(i); > Ouch, my bad. cheers [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-05-11 4:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-05-11 2:23 [PATCH 1/3] powerpc: kdump: Fix NULL pointer dereference in irq disable code Anton Blanchard 2010-05-11 2:25 ` [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU Anton Blanchard 2010-05-11 2:27 ` [PATCH 3/3] powerpc: kdump: Use chip->shutdown to disable IRQs Anton Blanchard 2010-05-11 4:11 ` Michael Ellerman 2010-05-11 4:14 ` [PATCH 2/3] powerpc: kdump: CPUs assume the context of the oopsing CPU Michael Ellerman 2010-05-11 4:14 ` [PATCH 1/3] powerpc: kdump: Fix NULL pointer dereference in irq disable code Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox