[PATCH v2 1/2] MIPS: OCTEON: fix kernel crash when offlining a CPU

[PATCH v2 1/2] MIPS: OCTEON: fix kernel crash when offlining a CPU

[Aaro Koskinen]

octeon_cpu_disable() will unconditionally enable interrupts when called.
We can assume that the routine is always called with interrupts disabled,
so just delete the incorrect local_irq_disable/enable().

The patch fixes the following crash when offlining a CPU:

[   93.818785] ------------[ cut here ]------------
[   93.823421] WARNING: CPU: 1 PID: 10 at kernel/smp.c:231 flush_smp_call_function_queue+0x1c4/0x1d0()
[   93.836215] Modules linked in:
[   93.839287] CPU: 1 PID: 10 Comm: migration/1 Not tainted 3.19.0-rc4-octeon-los_b5f0 #1
[   93.847212] Stack : 0000000000000001 ffffffff81b2cf90 0000000000000004 ffffffff81630000
	  0000000000000000 0000000000000000 0000000000000000 000000000000004a
	  0000000000000006 ffffffff8117e550 0000000000000000 0000000000000000
	  ffffffff81b30000 ffffffff81b26808 8000000032c77748 ffffffff81627e07
	  ffffffff81595ec8 ffffffff81b26808 000000000000000a 0000000000000001
	  0000000000000001 0000000000000003 0000000010008ce1 ffffffff815030c8
	  8000000032cbbb38 ffffffff8113d42c 0000000010008ce1 ffffffff8117f36c
	  8000000032c77300 8000000032cbba50 0000000000000001 ffffffff81503984
	  0000000000000000 0000000000000000 0000000000000000 0000000000000000
	  0000000000000000 ffffffff81121668 0000000000000000 0000000000000000
	  ...
[   93.912819] Call Trace:
[   93.915273] [<ffffffff81121668>] show_stack+0x68/0x80
[   93.920335] [<ffffffff81503984>] dump_stack+0x6c/0x90
[   93.925395] [<ffffffff8113d58c>] warn_slowpath_common+0x94/0xd8
[   93.931324] [<ffffffff811a402c>] flush_smp_call_function_queue+0x1c4/0x1d0
[   93.938208] [<ffffffff811a4128>] hotplug_cfd+0xf0/0x108
[   93.943444] [<ffffffff8115bacc>] notifier_call_chain+0x5c/0xb8
[   93.949286] [<ffffffff8113d704>] cpu_notify+0x24/0x60
[   93.954348] [<ffffffff81501738>] take_cpu_down+0x38/0x58
[   93.959670] [<ffffffff811b343c>] multi_cpu_stop+0x154/0x180
[   93.965250] [<ffffffff811b3768>] cpu_stopper_thread+0xd8/0x160
[   93.971093] [<ffffffff8115ea4c>] smpboot_thread_fn+0x1ec/0x1f8
[   93.976936] [<ffffffff8115ab04>] kthread+0xd4/0xf0
[   93.981735] [<ffffffff8111c4f0>] ret_from_kernel_thread+0x14/0x1c
[   93.987835]
[   93.989326] ---[ end trace c9e3815ee655bda9 ]---
[   93.993951] Kernel bug detected[#1]:
[   93.997533] CPU: 1 PID: 10 Comm: migration/1 Tainted: G        W      3.19.0-rc4-octeon-los_b5f0 #1
[   94.006591] task: 8000000032c77300 ti: 8000000032cb8000 task.ti: 8000000032cb8000
[   94.014081] $ 0   : 0000000000000000 0000000010000ce1 0000000000000001 ffffffff81620000
[   94.022146] $ 4   : 8000000002c72ac0 0000000000000000 00000000000001a7 ffffffff813b06f0
[   94.030210] $ 8   : ffffffff813b20d8 0000000000000000 0000000000000000 ffffffff81630000
[   94.038275] $12   : 0000000000000087 0000000000000000 0000000000000086 0000000000000000
[   94.046339] $16   : ffffffff81623168 0000000000000001 0000000000000000 0000000000000008
[   94.054405] $20   : 0000000000000001 0000000000000001 0000000000000001 0000000000000003
[   94.062470] $24   : 0000000000000038 ffffffff813b7f10
[   94.070536] $28   : 8000000032cb8000 8000000032cbbc20 0000000010008ce1 ffffffff811bcaf4
[   94.078601] Hi    : 0000000000f188e8
[   94.082179] Lo    : d4fdf3b646c09d55
[   94.085760] epc   : ffffffff811bc9d0 irq_work_run_list+0x8/0xf8
[   94.091686]     Tainted: G        W
[   94.095613] ra    : ffffffff811bcaf4 irq_work_run+0x34/0x60
[   94.101192] Status: 10000ce3	KX SX UX KERNEL EXL IE
[   94.106235] Cause : 40808034
[   94.109119] PrId  : 000d9301 (Cavium Octeon II)
[   94.113653] Modules linked in:
[   94.116721] Process migration/1 (pid: 10, threadinfo=8000000032cb8000, task=8000000032c77300, tls=0000000000000000)
[   94.127168] Stack : 8000000002c74c80 ffffffff811a4128 0000000000000001 ffffffff81635720
	  fffffffffffffff2 ffffffff8115bacc 80000000320fbce0 80000000320fbca4
	  80000000320fbc80 0000000000000002 0000000000000004 ffffffff8113d704
	  80000000320fbce0 ffffffff81501738 0000000000000003 ffffffff811b343c
	  8000000002c72aa0 8000000002c72aa8 ffffffff8159cae8 ffffffff8159caa0
	  ffffffff81650000 80000000320fbbf0 80000000320fbc80 ffffffff811b32e8
	  0000000000000000 ffffffff811b3768 ffffffff81622b80 ffffffff815148a8
	  8000000032c77300 8000000002c73e80 ffffffff815148a8 8000000032c77300
	  ffffffff81622b80 ffffffff815148a8 8000000032c77300 ffffffff81503f48
	  ffffffff8115ea0c ffffffff81620000 0000000000000000 ffffffff81174d64
	  ...
[   94.192771] Call Trace:
[   94.195222] [<ffffffff811bc9d0>] irq_work_run_list+0x8/0xf8
[   94.200802] [<ffffffff811bcaf4>] irq_work_run+0x34/0x60
[   94.206036] [<ffffffff811a4128>] hotplug_cfd+0xf0/0x108
[   94.211269] [<ffffffff8115bacc>] notifier_call_chain+0x5c/0xb8
[   94.217111] [<ffffffff8113d704>] cpu_notify+0x24/0x60
[   94.222171] [<ffffffff81501738>] take_cpu_down+0x38/0x58
[   94.227491] [<ffffffff811b343c>] multi_cpu_stop+0x154/0x180
[   94.233072] [<ffffffff811b3768>] cpu_stopper_thread+0xd8/0x160
[   94.238914] [<ffffffff8115ea4c>] smpboot_thread_fn+0x1ec/0x1f8
[   94.244757] [<ffffffff8115ab04>] kthread+0xd4/0xf0
[   94.249555] [<ffffffff8111c4f0>] ret_from_kernel_thread+0x14/0x1c
[   94.255654]
[   94.257146]
Code: a2423c40  40026000  30420001 <00020336> dc820000  10400037  00000000  0000010f  0000010f
[   94.267183] ---[ end trace c9e3815ee655bdaa ]---
[   94.271804] Fatal exception: panic in 5 seconds

Reported-by: Hemmo Nieminen <hemmo.nieminen@iki.fi>
Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Acked-by: David Daney <david.daney@cavium.com>
Cc: stable@vger.kernel.org # v3.18+
---

	v2: Remove local_irq_disable/local_irq_enable altogether, instead of
            saving and restoring the old context.

        v1: http://marc.info/?l=linux-mips&m=142134781818170&w=2

 arch/mips/cavium-octeon/smp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c
index ecd903d..8b1eeff 100644
--- a/arch/mips/cavium-octeon/smp.c
+++ b/arch/mips/cavium-octeon/smp.c
@@ -240,9 +240,7 @@ static int octeon_cpu_disable(void)
 
 	set_cpu_online(cpu, false);
 	cpu_clear(cpu, cpu_callin_map);
-	local_irq_disable();
 	octeon_fixup_irqs();
-	local_irq_enable();
 
 	flush_cache_all();
 	local_flush_tlb_all();
-- 
2.2.0

[PATCH v2 2/2] MIPS: fix kernel lockup or crash after CPU offline/online

[Aaro Koskinen]

From: Hemmo Nieminen <hemmo.nieminen@iki.fi>

As printk() invocation can cause e.g. a TLB miss, printk() cannot be
called before the exception handlers have been properly initialized.
This can happen e.g. when netconsole has been loaded as a kernel module
and the TLB table has been cleared when a CPU was offline.

Call cpu_report() in start_secondary() only after the exception handlers
have been initialized to fix this.

Without the patch the kernel will randomly either lockup or crash
after a CPU is onlined and the console driver is a module.

Signed-off-by: Hemmo Nieminen <hemmo.nieminen@iki.fi>
Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Cc: stable@vger.kernel.org
---

	v2: No changes in this patch.

	v1: http://marc.info/?l=linux-mips&m=142134783418176&w=2

 arch/mips/kernel/smp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index c94c4e9..1c0d8c5 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -123,10 +123,10 @@ asmlinkage void start_secondary(void)
 	unsigned int cpu;
 
 	cpu_probe();
-	cpu_report();
 	per_cpu_trap_init(false);
 	mips_clockevent_init();
 	mp_ops->init_secondary();
+	cpu_report();
 
 	/*
 	 * XXX parity protection should be folded in here when it's converted
-- 
2.2.0

Re: [PATCH v2 2/2] MIPS: fix kernel lockup or crash after CPU offline/online

[Maciej W. Rozycki]

On Thu, 15 Jan 2015, Aaro Koskinen wrote:

> As printk() invocation can cause e.g. a TLB miss, printk() cannot be
> called before the exception handlers have been properly initialized.
> This can happen e.g. when netconsole has been loaded as a kernel module
> and the TLB table has been cleared when a CPU was offline.

 Hmm, why can a call to `printk' cause a TLB miss, what's so special about 
this function?  Does it use kernel mapped addresses for any purpose such 
as `vmalloc'?

  Maciej

Re: [PATCH v2 2/2] MIPS: fix kernel lockup or crash after CPU offline/online

[James Hogan]

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

Hi Maciej,

On 30/01/15 09:25, Maciej W. Rozycki wrote:
> On Thu, 15 Jan 2015, Aaro Koskinen wrote:
> 
>> As printk() invocation can cause e.g. a TLB miss, printk() cannot be
>> called before the exception handlers have been properly initialized.
>> This can happen e.g. when netconsole has been loaded as a kernel module
>> and the TLB table has been cleared when a CPU was offline.
> 
>  Hmm, why can a call to `printk' cause a TLB miss, what's so special about 
> this function?  Does it use kernel mapped addresses for any purpose such 
> as `vmalloc'?

It would be the fact netconsole (or whatever other console is in use) is
built as a kernel module, memory for which is allocated from the vmalloc
area.

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/2] MIPS: fix kernel lockup or crash after CPU offline/online

[Maciej W. Rozycki]

On Fri, 30 Jan 2015, James Hogan wrote:

> >  Hmm, why can a call to `printk' cause a TLB miss, what's so special about 
> > this function?  Does it use kernel mapped addresses for any purpose such 
> > as `vmalloc'?
> 
> It would be the fact netconsole (or whatever other console is in use) is
> built as a kernel module, memory for which is allocated from the vmalloc
> area.

 Ah, I see, thanks for enlightening me.  But in that case wouldn't it be 
possible to postpone console output from `printk' until it is safe to 
access the device?  In a manner similar to how for example we handle calls 
to `printk' made from the hardirq context.  That would make things less 
fragile.

  Maciej

Re: [PATCH v2 2/2] MIPS: fix kernel lockup or crash after CPU offline/online

[James Hogan]

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

On 30/01/15 12:47, Maciej W. Rozycki wrote:
> On Fri, 30 Jan 2015, James Hogan wrote:
> 
>>>  Hmm, why can a call to `printk' cause a TLB miss, what's so special about 
>>> this function?  Does it use kernel mapped addresses for any purpose such 
>>> as `vmalloc'?
>>
>> It would be the fact netconsole (or whatever other console is in use) is
>> built as a kernel module, memory for which is allocated from the vmalloc
>> area.
> 
>  Ah, I see, thanks for enlightening me.  But in that case wouldn't it be 
> possible to postpone console output from `printk' until it is safe to 
> access the device?  In a manner similar to how for example we handle calls 
> to `printk' made from the hardirq context.  That would make things less 
> fragile.

Hmm, kernel/printk/printk.c does have:

static inline int can_use_console(unsigned int cpu)
{
	return cpu_online(cpu) || have_callable_console();
}

which should prevent it dumping printk buffer to console. CPU shouldn't
be marked online that early, which suggests that the console has the
CON_ANYTIME flag set, which it probably shouldn't if it depends on
module code. call_console_drivers() seems to ensure the CPU is online or
has CON_ANYTIME before calling the console write callback.

A quick glance and I can't see any evidence of netconsole being able to
get CON_ANYTIME.

serial8250_console does appear to set that flag *and* is tristate
though, which is slightly worrying.

Aaro, what is the content of your /proc/consoles?

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/2] MIPS: fix kernel lockup or crash after CPU offline/online

[Aaro Koskinen]

Hi,

On Fri, Jan 30, 2015 at 02:59:57PM +0000, James Hogan wrote:
> On 30/01/15 12:47, Maciej W. Rozycki wrote:
> > On Fri, 30 Jan 2015, James Hogan wrote:
> > 
> >>>  Hmm, why can a call to `printk' cause a TLB miss, what's so special about 
> >>> this function?  Does it use kernel mapped addresses for any purpose such 
> >>> as `vmalloc'?
> >>
> >> It would be the fact netconsole (or whatever other console is in use) is
> >> built as a kernel module, memory for which is allocated from the vmalloc
> >> area.
> > 
> >  Ah, I see, thanks for enlightening me.  But in that case wouldn't it be 
> > possible to postpone console output from `printk' until it is safe to 
> > access the device?  In a manner similar to how for example we handle calls 
> > to `printk' made from the hardirq context.  That would make things less 
> > fragile.
> 
> Hmm, kernel/printk/printk.c does have:
> 
> static inline int can_use_console(unsigned int cpu)
> {
> 	return cpu_online(cpu) || have_callable_console();
> }
> 
> which should prevent it dumping printk buffer to console. CPU shouldn't
> be marked online that early, which suggests that the console has the
> CON_ANYTIME flag set, which it probably shouldn't if it depends on
> module code. call_console_drivers() seems to ensure the CPU is online or
> has CON_ANYTIME before calling the console write callback.
> 
> A quick glance and I can't see any evidence of netconsole being able to
> get CON_ANYTIME.

It does not set the flag. But flags are kept in module's static data,
so the original problem stays.

A.

Re: [PATCH v2 2/2] MIPS: fix kernel lockup or crash after CPU offline/online

[James Hogan]

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

On Fri, Jan 30, 2015 at 07:55:32PM +0200, Aaro Koskinen wrote:
> Hi,
> 
> On Fri, Jan 30, 2015 at 02:59:57PM +0000, James Hogan wrote:
> > On 30/01/15 12:47, Maciej W. Rozycki wrote:
> > > On Fri, 30 Jan 2015, James Hogan wrote:
> > > 
> > >>>  Hmm, why can a call to `printk' cause a TLB miss, what's so special about 
> > >>> this function?  Does it use kernel mapped addresses for any purpose such 
> > >>> as `vmalloc'?
> > >>
> > >> It would be the fact netconsole (or whatever other console is in use) is
> > >> built as a kernel module, memory for which is allocated from the vmalloc
> > >> area.
> > > 
> > >  Ah, I see, thanks for enlightening me.  But in that case wouldn't it be 
> > > possible to postpone console output from `printk' until it is safe to 
> > > access the device?  In a manner similar to how for example we handle calls 
> > > to `printk' made from the hardirq context.  That would make things less 
> > > fragile.
> > 
> > Hmm, kernel/printk/printk.c does have:
> > 
> > static inline int can_use_console(unsigned int cpu)
> > {
> > 	return cpu_online(cpu) || have_callable_console();
> > }
> > 
> > which should prevent it dumping printk buffer to console. CPU shouldn't
> > be marked online that early, which suggests that the console has the
> > CON_ANYTIME flag set, which it probably shouldn't if it depends on
> > module code. call_console_drivers() seems to ensure the CPU is online or
> > has CON_ANYTIME before calling the console write callback.
> > 
> > A quick glance and I can't see any evidence of netconsole being able to
> > get CON_ANYTIME.
> 
> It does not set the flag. But flags are kept in module's static data,
> so the original problem stays.
> 
> A.

Ah yes, of course. This approach looks correct to me then.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/2] MIPS: fix kernel lockup or crash after CPU offline/online

[Maciej W. Rozycki]

On Fri, 30 Jan 2015, James Hogan wrote:

> > > Hmm, kernel/printk/printk.c does have:
> > > 
> > > static inline int can_use_console(unsigned int cpu)
> > > {
> > > 	return cpu_online(cpu) || have_callable_console();
> > > }
> > > 
> > > which should prevent it dumping printk buffer to console. CPU shouldn't
> > > be marked online that early, which suggests that the console has the
> > > CON_ANYTIME flag set, which it probably shouldn't if it depends on
> > > module code. call_console_drivers() seems to ensure the CPU is online or
> > > has CON_ANYTIME before calling the console write callback.
> > > 
> > > A quick glance and I can't see any evidence of netconsole being able to
> > > get CON_ANYTIME.
> > 
> > It does not set the flag. But flags are kept in module's static data,
> > so the original problem stays.
> > 
> > A.
> 
> Ah yes, of course. This approach looks correct to me then.

 In such a case shouldn't the flags be copied out on console registration 
to a structure that is guaranteed to be accessible at all times?

  Maciej