* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-24 8:06 ` Zhang, Yanmin
@ 2008-06-24 8:37 ` Vegard Nossum
2008-06-24 13:14 ` Rusty Russell
` (2 subsequent siblings)
3 siblings, 0 replies; 24+ messages in thread
From: Vegard Nossum @ 2008-06-24 8:37 UTC (permalink / raw)
To: Zhang, Yanmin
Cc: Rusty Russell, Mike Travis, Adrian Bunk, Srivatsa Vaddagiri,
linux-kernel, Gautham R Shenoy, Rafael J. Wysocki, Zhang, Yanmin,
Heiko Carstens
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1837 bytes --]
On Tue, Jun 24, 2008 at 10:06 AM, Zhang, Yanmin<yanmin_zhang@linux.intel.com> wrote:> In function _cpu_up, the panic happens when calling __raw_notifier_call_chain> at the second time. Kernel doesn't panic when calling it at the first time. If> just say because of nr_cpu_ids, that's not right.>> By checking source codes, I find function do_boot_cpu is the culprit.> Consider below call chain:> _cpu_up=>__cpu_up=>smp_ops.cpu_up=>native_cpu_up=>do_boot_cpu.>> So do_boot_cpu is called in the end. In do_boot_cpu, if boot_error==true,> cpu_clear(cpu, cpu_possible_map) is executed. So later on, when _cpu_up> calls __raw_notifier_call_chain at the second time to report CPU_UP_CANCELED,> because this cpu is already cleared from cpu_possible_map, get_cpu_sysdev returns> NULL.
Ahhha! Well done!
(Whew, I have a lot to learn :-D)
>> Many resources are related to cpu_possible_map, so it's better not to change it.>> Below patch against 2.6.26-rc7 fixes it by removing the bit clearing in cpu_possible_map.>> Vegard, would you like to help test it?
Sure, but it can take a while. 1) I have no idea why the processorfailed to initialize in the first place. So far, it only ever happenedthis one time. 2) There seems to be a couple of other failure casesrelated to cpu hotplug (usually the machine freezes hard), so it's notcertain that we hit this (failed to initialize) first. But I will try!
Thanks for solving the mystery.
Vegard
-- "The animistic metaphor of the bug that maliciously sneaked in whilethe programmer was not looking is intellectually dishonest as itdisguises that the error is the programmer's own creation." -- E. W. Dijkstra, EWD1036ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-24 8:06 ` Zhang, Yanmin
2008-06-24 8:37 ` Vegard Nossum
@ 2008-06-24 13:14 ` Rusty Russell
2008-06-24 14:44 ` Mike Travis
` (2 more replies)
2008-06-26 0:59 ` Zhang, Yanmin
2008-07-10 19:10 ` Vegard Nossum
3 siblings, 3 replies; 24+ messages in thread
From: Rusty Russell @ 2008-06-24 13:14 UTC (permalink / raw)
To: Zhang, Yanmin
Cc: Mike Travis, Vegard Nossum, Adrian Bunk, Srivatsa Vaddagiri,
linux-kernel, Gautham R Shenoy, Rafael J. Wysocki, Zhang, Yanmin,
Heiko Carstens
On Tuesday 24 June 2008 18:06:23 Zhang, Yanmin wrote:
> On Tue, 2008-06-24 at 11:36 +1000, Rusty Russell wrote:
> > On Tuesday 24 June 2008 02:58:44 Mike Travis wrote:
> > > Rusty Russell wrote:
> > > > On Monday 23 June 2008 02:29:07 Vegard Nossum wrote:
> > > >> And the (cpu < nr_cpu_ids) fails because the CPU has just been
> > > >> offlined (or failed to initialize, but it's the same thing), while
> > > >> NR_CPUS is the value that was compiled in as CONFIG_NR_CPUS (so the
> > > >> former check will always be true).
> > > >>
> > > >> I don't think it is valid to ask for a per_cpu() variable on a CPU
> > > >> which does not exist, though
> > > >
> > > > Yes it is. As long as cpu_possible(cpu), per_cpu(cpu) is valid.
> > > >
> > > > The number check should be removed: checking cpu_possible() is
> > > > sufficient.
> > > >
> > > > Hope that helps,
> > > > Rusty.
> > >
> > > I don't see a check for index being out of range in cpu_possible().
> >
> > You're right. It assumes cpu is < NR_CPUS. Hmm, I have no idea what's
> > going on. nr_cpu_ids (ignore that it's a horrible name for a bad idea)
> > should be fine to test against.
> >
> > Vegard's analysis is flawed: just because cpu is offline, it still must
> > be < nr_cpu_ids, which is based on possible cpus. Unless something crazy
> > is happening, but a quick grep doesn't reveal anyone manipulating
> > nr_cpu_ids.
> >
> > If changing this fixes the bug, something else is badly wrong...
> > Rusty.
>
> In function _cpu_up, the panic happens when calling
> __raw_notifier_call_chain at the second time. Kernel doesn't panic when
> calling it at the first time. If just say because of nr_cpu_ids, that's
> not right.
>
> By checking source codes, I find function do_boot_cpu is the culprit.
> Consider below call chain:
> _cpu_up=>__cpu_up=>smp_ops.cpu_up=>native_cpu_up=>do_boot_cpu.
>
> So do_boot_cpu is called in the end. In do_boot_cpu, if boot_error==true,
> cpu_clear(cpu, cpu_possible_map) is executed. So later on, when _cpu_up
> calls __raw_notifier_call_chain at the second time to report
> CPU_UP_CANCELED, because this cpu is already cleared from
> cpu_possible_map, get_cpu_sysdev returns NULL.
>
> Many resources are related to cpu_possible_map, so it's better not to
> change it.
>
> Below patch against 2.6.26-rc7 fixes it by removing the bit clearing in
> cpu_possible_map.
>
> Vegard, would you like to help test it?
>
> Signed-off-by: Zhang Yanmin <yanmin_zhang@linux.intel.com>
>
> ---
>
> diff -Nraup linux-2.6.26-rc7/arch/x86/kernel/smpboot.c
> linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c ---
> linux-2.6.26-rc7/arch/x86/kernel/smpboot.c 2008-06-24 09:03:54.000000000
> +0800 +++ linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c 2008-06-24
> 09:04:45.000000000 +0800 @@ -996,7 +996,6 @@ do_rest:
> #endif
> cpu_clear(cpu, cpu_callout_map); /* was set by do_boot_cpu() */
> cpu_clear(cpu, cpu_initialized); /* was set by cpu_init() */
> - cpu_clear(cpu, cpu_possible_map);
> cpu_clear(cpu, cpu_present_map);
> per_cpu(x86_cpu_to_apicid, cpu) = BAD_APICID;
> }
Nice catch. Basically, cpu_possible_map should only be cleared at boot, and
probably not even then.
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Thanks,
Rusty.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-24 13:14 ` Rusty Russell
@ 2008-06-24 14:44 ` Mike Travis
2008-06-25 5:38 ` Rusty Russell
2008-06-26 12:58 ` Gautham R Shenoy
2008-06-30 11:19 ` Ingo Molnar
2 siblings, 1 reply; 24+ messages in thread
From: Mike Travis @ 2008-06-24 14:44 UTC (permalink / raw)
To: Rusty Russell
Cc: Zhang, Yanmin, Vegard Nossum, Adrian Bunk, Srivatsa Vaddagiri,
linux-kernel, Gautham R Shenoy, Rafael J. Wysocki, Zhang, Yanmin,
Heiko Carstens
Rusty Russell wrote:
...
> Nice catch. Basically, cpu_possible_map should only be cleared at boot, and
> probably not even then.
>
One thing that should be avoided, is clearing anything but the last bit in the
cpu_possible_map. This is because num_possible_cpus != nr_cpu_ids when there
are holes in the map. (nr_cpu_ids = highest possible cpu # + 1).
Thanks,
Mike
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-24 14:44 ` Mike Travis
@ 2008-06-25 5:38 ` Rusty Russell
2008-06-25 15:06 ` Mike Travis
0 siblings, 1 reply; 24+ messages in thread
From: Rusty Russell @ 2008-06-25 5:38 UTC (permalink / raw)
To: Mike Travis
Cc: Zhang, Yanmin, Vegard Nossum, Adrian Bunk, Srivatsa Vaddagiri,
linux-kernel, Gautham R Shenoy, Rafael J. Wysocki, Zhang, Yanmin,
Heiko Carstens
On Wednesday 25 June 2008 00:44:45 Mike Travis wrote:
> Rusty Russell wrote:
> ...
>
> > Nice catch. Basically, cpu_possible_map should only be cleared at boot,
> > and probably not even then.
>
> One thing that should be avoided, is clearing anything but the last bit in
> the cpu_possible_map. This is because num_possible_cpus != nr_cpu_ids when
> there are holes in the map. (nr_cpu_ids = highest possible cpu # + 1).
It's ok if nr_cpu_ids is an overestimate, isn't it?
But for this corner case, I think clearing cpu_possible_map is wrong.
Cheers,
Rusty.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-25 5:38 ` Rusty Russell
@ 2008-06-25 15:06 ` Mike Travis
0 siblings, 0 replies; 24+ messages in thread
From: Mike Travis @ 2008-06-25 15:06 UTC (permalink / raw)
To: Rusty Russell
Cc: Zhang, Yanmin, Vegard Nossum, Adrian Bunk, Srivatsa Vaddagiri,
linux-kernel, Gautham R Shenoy, Rafael J. Wysocki, Zhang, Yanmin,
Heiko Carstens
Rusty Russell wrote:
> On Wednesday 25 June 2008 00:44:45 Mike Travis wrote:
>> Rusty Russell wrote:
>> ...
>>
>>> Nice catch. Basically, cpu_possible_map should only be cleared at boot,
>>> and probably not even then.
>> One thing that should be avoided, is clearing anything but the last bit in
>> the cpu_possible_map. This is because num_possible_cpus != nr_cpu_ids when
>> there are holes in the map. (nr_cpu_ids = highest possible cpu # + 1).
>
> It's ok if nr_cpu_ids is an overestimate, isn't it?
Yes. As I see it, nr_cpu_ids is the max index (+1) into anything dealing with
cpu's.
>
> But for this corner case, I think clearing cpu_possible_map is wrong.
Yes, I agree. If for some reason, ACPI discovers a "possible" cpu but it faults
when brought online, then it simply stays offline. It may never come online, or
with some trick hardware, it could be replaced on the running system and then
a new attempt can be made to bring it online.
Thanks,
Mike
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-24 13:14 ` Rusty Russell
2008-06-24 14:44 ` Mike Travis
@ 2008-06-26 12:58 ` Gautham R Shenoy
2008-06-27 3:16 ` Rusty Russell
2008-06-30 11:19 ` Ingo Molnar
2 siblings, 1 reply; 24+ messages in thread
From: Gautham R Shenoy @ 2008-06-26 12:58 UTC (permalink / raw)
To: Rusty Russell
Cc: Zhang, Yanmin, Mike Travis, Vegard Nossum, Adrian Bunk,
Srivatsa Vaddagiri, linux-kernel, Rafael J. Wysocki,
Zhang, Yanmin, Heiko Carstens
On Tue, Jun 24, 2008 at 11:14:51PM +1000, Rusty Russell wrote:
> On Tuesday 24 June 2008 18:06:23 Zhang, Yanmin wrote:
> > On Tue, 2008-06-24 at 11:36 +1000, Rusty Russell wrote:
> > > On Tuesday 24 June 2008 02:58:44 Mike Travis wrote:
> > > > Rusty Russell wrote:
> > > > > On Monday 23 June 2008 02:29:07 Vegard Nossum wrote:
> > > > >> And the (cpu < nr_cpu_ids) fails because the CPU has just been
> > > > >> offlined (or failed to initialize, but it's the same thing), while
> > > > >> NR_CPUS is the value that was compiled in as CONFIG_NR_CPUS (so the
> > > > >> former check will always be true).
> > > > >>
> > > > >> I don't think it is valid to ask for a per_cpu() variable on a CPU
> > > > >> which does not exist, though
> > > > >
> > > > > Yes it is. As long as cpu_possible(cpu), per_cpu(cpu) is valid.
> > > > >
> > > > > The number check should be removed: checking cpu_possible() is
> > > > > sufficient.
> > > > >
> > > > > Hope that helps,
> > > > > Rusty.
> > > >
> > > > I don't see a check for index being out of range in cpu_possible().
> > >
> > > You're right. It assumes cpu is < NR_CPUS. Hmm, I have no idea what's
> > > going on. nr_cpu_ids (ignore that it's a horrible name for a bad idea)
> > > should be fine to test against.
> > >
> > > Vegard's analysis is flawed: just because cpu is offline, it still must
> > > be < nr_cpu_ids, which is based on possible cpus. Unless something crazy
> > > is happening, but a quick grep doesn't reveal anyone manipulating
> > > nr_cpu_ids.
> > >
> > > If changing this fixes the bug, something else is badly wrong...
> > > Rusty.
> >
> > In function _cpu_up, the panic happens when calling
> > __raw_notifier_call_chain at the second time. Kernel doesn't panic when
> > calling it at the first time. If just say because of nr_cpu_ids, that's
> > not right.
> >
> > By checking source codes, I find function do_boot_cpu is the culprit.
> > Consider below call chain:
> > _cpu_up=>__cpu_up=>smp_ops.cpu_up=>native_cpu_up=>do_boot_cpu.
> >
> > So do_boot_cpu is called in the end. In do_boot_cpu, if boot_error==true,
> > cpu_clear(cpu, cpu_possible_map) is executed. So later on, when _cpu_up
> > calls __raw_notifier_call_chain at the second time to report
> > CPU_UP_CANCELED, because this cpu is already cleared from
> > cpu_possible_map, get_cpu_sysdev returns NULL.
> >
> > Many resources are related to cpu_possible_map, so it's better not to
> > change it.
> >
> > Below patch against 2.6.26-rc7 fixes it by removing the bit clearing in
> > cpu_possible_map.
> >
> > Vegard, would you like to help test it?
> >
> > Signed-off-by: Zhang Yanmin <yanmin_zhang@linux.intel.com>
> >
> > ---
> >
> > diff -Nraup linux-2.6.26-rc7/arch/x86/kernel/smpboot.c
> > linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c ---
> > linux-2.6.26-rc7/arch/x86/kernel/smpboot.c 2008-06-24 09:03:54.000000000
> > +0800 +++ linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c 2008-06-24
> > 09:04:45.000000000 +0800 @@ -996,7 +996,6 @@ do_rest:
> > #endif
> > cpu_clear(cpu, cpu_callout_map); /* was set by do_boot_cpu() */
> > cpu_clear(cpu, cpu_initialized); /* was set by cpu_init() */
> > - cpu_clear(cpu, cpu_possible_map);
> > cpu_clear(cpu, cpu_present_map);
Nice catch.
While we're at it, is the clearing of cpu from the cpu_present_map
necessary if cpu_up failed for 'cpu' ?
> > per_cpu(x86_cpu_to_apicid, cpu) = BAD_APICID;
> > }
>
> Nice catch. Basically, cpu_possible_map should only be cleared at boot, and
> probably not even then.
>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Thanks,
> Rusty.
--
Thanks and Regards
gautham
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-26 12:58 ` Gautham R Shenoy
@ 2008-06-27 3:16 ` Rusty Russell
0 siblings, 0 replies; 24+ messages in thread
From: Rusty Russell @ 2008-06-27 3:16 UTC (permalink / raw)
To: ego
Cc: Zhang, Yanmin, Mike Travis, Vegard Nossum, Adrian Bunk,
Srivatsa Vaddagiri, linux-kernel, Rafael J. Wysocki,
Zhang, Yanmin, Heiko Carstens
On Thursday 26 June 2008 22:58:20 Gautham R Shenoy wrote:
> On Tue, Jun 24, 2008 at 11:14:51PM +1000, Rusty Russell wrote:
> > On Tuesday 24 June 2008 18:06:23 Zhang, Yanmin wrote:
> > > On Tue, 2008-06-24 at 11:36 +1000, Rusty Russell wrote:
> > > > On Tuesday 24 June 2008 02:58:44 Mike Travis wrote:
> > > > > Rusty Russell wrote:
> > > > > > On Monday 23 June 2008 02:29:07 Vegard Nossum wrote:
> > > > > >> And the (cpu < nr_cpu_ids) fails because the CPU has just been
> > > > > >> offlined (or failed to initialize, but it's the same thing),
> > > > > >> while NR_CPUS is the value that was compiled in as
> > > > > >> CONFIG_NR_CPUS (so the former check will always be true).
> > > > > >>
> > > > > >> I don't think it is valid to ask for a per_cpu() variable on a
> > > > > >> CPU which does not exist, though
> > > > > >
> > > > > > Yes it is. As long as cpu_possible(cpu), per_cpu(cpu) is valid.
> > > > > >
> > > > > > The number check should be removed: checking cpu_possible() is
> > > > > > sufficient.
> > > > > >
> > > > > > Hope that helps,
> > > > > > Rusty.
> > > > >
> > > > > I don't see a check for index being out of range in cpu_possible().
> > > >
> > > > You're right. It assumes cpu is < NR_CPUS. Hmm, I have no idea
> > > > what's going on. nr_cpu_ids (ignore that it's a horrible name for a
> > > > bad idea) should be fine to test against.
> > > >
> > > > Vegard's analysis is flawed: just because cpu is offline, it still
> > > > must be < nr_cpu_ids, which is based on possible cpus. Unless
> > > > something crazy is happening, but a quick grep doesn't reveal anyone
> > > > manipulating nr_cpu_ids.
> > > >
> > > > If changing this fixes the bug, something else is badly wrong...
> > > > Rusty.
> > >
> > > In function _cpu_up, the panic happens when calling
> > > __raw_notifier_call_chain at the second time. Kernel doesn't panic when
> > > calling it at the first time. If just say because of nr_cpu_ids,
> > > that's not right.
> > >
> > > By checking source codes, I find function do_boot_cpu is the culprit.
> > > Consider below call chain:
> > > _cpu_up=>__cpu_up=>smp_ops.cpu_up=>native_cpu_up=>do_boot_cpu.
> > >
> > > So do_boot_cpu is called in the end. In do_boot_cpu, if
> > > boot_error==true, cpu_clear(cpu, cpu_possible_map) is executed. So
> > > later on, when _cpu_up calls __raw_notifier_call_chain at the second
> > > time to report
> > > CPU_UP_CANCELED, because this cpu is already cleared from
> > > cpu_possible_map, get_cpu_sysdev returns NULL.
> > >
> > > Many resources are related to cpu_possible_map, so it's better not to
> > > change it.
> > >
> > > Below patch against 2.6.26-rc7 fixes it by removing the bit clearing in
> > > cpu_possible_map.
> > >
> > > Vegard, would you like to help test it?
> > >
> > > Signed-off-by: Zhang Yanmin <yanmin_zhang@linux.intel.com>
> > >
> > > ---
> > >
> > > diff -Nraup linux-2.6.26-rc7/arch/x86/kernel/smpboot.c
> > > linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c ---
> > > linux-2.6.26-rc7/arch/x86/kernel/smpboot.c 2008-06-24
> > > 09:03:54.000000000 +0800 +++
> > > linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c 2008-06-24
> > > 09:04:45.000000000 +0800 @@ -996,7 +996,6 @@ do_rest:
> > > #endif
> > > cpu_clear(cpu, cpu_callout_map); /* was set by do_boot_cpu() */
> > > cpu_clear(cpu, cpu_initialized); /* was set by cpu_init() */
> > > - cpu_clear(cpu, cpu_possible_map);
> > > cpu_clear(cpu, cpu_present_map);
>
> Nice catch.
>
> While we're at it, is the clearing of cpu from the cpu_present_map
> necessary if cpu_up failed for 'cpu' ?
It's never necessary, but there there are not many places which cpu_present is
examined. It just prevents it from being hot added again, AFAICT.
Rusty.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-24 13:14 ` Rusty Russell
2008-06-24 14:44 ` Mike Travis
2008-06-26 12:58 ` Gautham R Shenoy
@ 2008-06-30 11:19 ` Ingo Molnar
2 siblings, 0 replies; 24+ messages in thread
From: Ingo Molnar @ 2008-06-30 11:19 UTC (permalink / raw)
To: Rusty Russell
Cc: Zhang, Yanmin, Mike Travis, Vegard Nossum, Adrian Bunk,
Srivatsa Vaddagiri, linux-kernel, Gautham R Shenoy,
Rafael J. Wysocki, Zhang, Yanmin, Heiko Carstens, Andrew Morton
* Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Tuesday 24 June 2008 18:06:23 Zhang, Yanmin wrote:
> > In function _cpu_up, the panic happens when calling
> > __raw_notifier_call_chain at the second time. Kernel doesn't panic
> > when calling it at the first time. If just say because of
> > nr_cpu_ids, that's not right.
> >
> > By checking source codes, I find function do_boot_cpu is the
> > culprit. Consider below call chain:
> > _cpu_up=>__cpu_up=>smp_ops.cpu_up=>native_cpu_up=>do_boot_cpu.
> >
> > So do_boot_cpu is called in the end. In do_boot_cpu, if
> > boot_error==true, cpu_clear(cpu, cpu_possible_map) is executed. So
> > later on, when _cpu_up calls __raw_notifier_call_chain at the second
> > time to report CPU_UP_CANCELED, because this cpu is already cleared
> > from cpu_possible_map, get_cpu_sysdev returns NULL.
> >
> > Many resources are related to cpu_possible_map, so it's better not to
> > change it.
> >
> > Below patch against 2.6.26-rc7 fixes it by removing the bit clearing in
> > cpu_possible_map.
> >
> > Vegard, would you like to help test it?
> >
> > Signed-off-by: Zhang Yanmin <yanmin_zhang@linux.intel.com>
[...]
> Nice catch. Basically, cpu_possible_map should only be cleared at
> boot, and probably not even then.
>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
applied to tip/x86/urgent for v2.6.26 merging - thanks everyone!
Ingo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-24 8:06 ` Zhang, Yanmin
2008-06-24 8:37 ` Vegard Nossum
2008-06-24 13:14 ` Rusty Russell
@ 2008-06-26 0:59 ` Zhang, Yanmin
2008-06-26 2:15 ` Andrew Morton
2008-07-10 19:10 ` Vegard Nossum
3 siblings, 1 reply; 24+ messages in thread
From: Zhang, Yanmin @ 2008-06-26 0:59 UTC (permalink / raw)
To: Andrew Morton, Ingo Molnar
Cc: Mike Travis, Vegard Nossum, Adrian Bunk, Srivatsa Vaddagiri,
linux-kernel, Gautham R Shenoy, Rafael J. Wysocki, Zhang, Yanmin,
Heiko Carstens, Rusty Russell
On Tue, 2008-06-24 at 16:06 +0800, Zhang, Yanmin wrote:
> On Tue, 2008-06-24 at 11:36 +1000, Rusty Russell wrote:
> > On Tuesday 24 June 2008 02:58:44 Mike Travis wrote:
> > > Rusty Russell wrote:
> > > > On Monday 23 June 2008 02:29:07 Vegard Nossum wrote:
> > > >> And the (cpu < nr_cpu_ids) fails because the CPU has just been
> > > >> offlined (or failed to initialize, but it's the same thing), while
> > > >> NR_CPUS is the value that was compiled in as CONFIG_NR_CPUS (so the
> > > >> former check will always be true).
> > > >>
> > > >> I don't think it is valid to ask for a per_cpu() variable on a CPU
> > > >> which does not exist, though
> > > >
> > > > Yes it is. As long as cpu_possible(cpu), per_cpu(cpu) is valid.
> > > >
> > > > The number check should be removed: checking cpu_possible() is
> > > > sufficient.
> > > >
> > > > Hope that helps,
> > > > Rusty.
> > >
> > > I don't see a check for index being out of range in cpu_possible().
> >
> > You're right. It assumes cpu is < NR_CPUS. Hmm, I have no idea what's going
> > on. nr_cpu_ids (ignore that it's a horrible name for a bad idea) should be
> > fine to test against.
> >
> > Vegard's analysis is flawed: just because cpu is offline, it still must be <
> > nr_cpu_ids, which is based on possible cpus. Unless something crazy is
> > happening, but a quick grep doesn't reveal anyone manipulating nr_cpu_ids.
> >
> > If changing this fixes the bug, something else is badly wrong...
> > Rusty.
>
> In function _cpu_up, the panic happens when calling __raw_notifier_call_chain
> at the second time. Kernel doesn't panic when calling it at the first time. If
> just say because of nr_cpu_ids, that's not right.
>
> By checking source codes, I find function do_boot_cpu is the culprit.
> Consider below call chain:
> _cpu_up=>__cpu_up=>smp_ops.cpu_up=>native_cpu_up=>do_boot_cpu.
>
> So do_boot_cpu is called in the end. In do_boot_cpu, if boot_error==true,
> cpu_clear(cpu, cpu_possible_map) is executed. So later on, when _cpu_up
> calls __raw_notifier_call_chain at the second time to report CPU_UP_CANCELED,
> because this cpu is already cleared from cpu_possible_map, get_cpu_sysdev returns
> NULL.
>
> Many resources are related to cpu_possible_map, so it's better not to change it.
>
> Below patch against 2.6.26-rc7 fixes it by removing the bit clearing in cpu_possible_map.
>
> Vegard, would you like to help test it?
>
> Signed-off-by: Zhang Yanmin <yanmin_zhang@linux.intel.com>
>
> ---
>
> diff -Nraup linux-2.6.26-rc7/arch/x86/kernel/smpboot.c linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c
> --- linux-2.6.26-rc7/arch/x86/kernel/smpboot.c 2008-06-24 09:03:54.000000000 +0800
> +++ linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c 2008-06-24 09:04:45.000000000 +0800
> @@ -996,7 +996,6 @@ do_rest:
> #endif
> cpu_clear(cpu, cpu_callout_map); /* was set by do_boot_cpu() */
> cpu_clear(cpu, cpu_initialized); /* was set by cpu_init() */
> - cpu_clear(cpu, cpu_possible_map);
> cpu_clear(cpu, cpu_present_map);
> per_cpu(x86_cpu_to_apicid, cpu) = BAD_APICID;
> }
>
Andrew,
Would you like to pick up this patch? Rusty Russell <rusty@rustcorp.com.au> acked it.
Thanks,
yanmin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-26 0:59 ` Zhang, Yanmin
@ 2008-06-26 2:15 ` Andrew Morton
2008-06-26 9:00 ` Vegard Nossum
0 siblings, 1 reply; 24+ messages in thread
From: Andrew Morton @ 2008-06-26 2:15 UTC (permalink / raw)
To: Zhang, Yanmin
Cc: Ingo Molnar, Mike Travis, Vegard Nossum, Adrian Bunk,
Srivatsa Vaddagiri, linux-kernel, Gautham R Shenoy,
Rafael J. Wysocki, Zhang, Yanmin, Heiko Carstens, Rusty Russell
On Thu, 26 Jun 2008 08:59:39 +0800 "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:
>
> On Tue, 2008-06-24 at 16:06 +0800, Zhang, Yanmin wrote:
> > On Tue, 2008-06-24 at 11:36 +1000, Rusty Russell wrote:
> > > On Tuesday 24 June 2008 02:58:44 Mike Travis wrote:
> > > > Rusty Russell wrote:
> > > > > On Monday 23 June 2008 02:29:07 Vegard Nossum wrote:
> > > > >> And the (cpu < nr_cpu_ids) fails because the CPU has just been
> > > > >> offlined (or failed to initialize, but it's the same thing), while
> > > > >> NR_CPUS is the value that was compiled in as CONFIG_NR_CPUS (so the
> > > > >> former check will always be true).
> > > > >>
> > > > >> I don't think it is valid to ask for a per_cpu() variable on a CPU
> > > > >> which does not exist, though
> > > > >
> > > > > Yes it is. As long as cpu_possible(cpu), per_cpu(cpu) is valid.
> > > > >
> > > > > The number check should be removed: checking cpu_possible() is
> > > > > sufficient.
> > > > >
> > > > > Hope that helps,
> > > > > Rusty.
> > > >
> > > > I don't see a check for index being out of range in cpu_possible().
> > >
> > > You're right. It assumes cpu is < NR_CPUS. Hmm, I have no idea what's going
> > > on. nr_cpu_ids (ignore that it's a horrible name for a bad idea) should be
> > > fine to test against.
> > >
> > > Vegard's analysis is flawed: just because cpu is offline, it still must be <
> > > nr_cpu_ids, which is based on possible cpus. Unless something crazy is
> > > happening, but a quick grep doesn't reveal anyone manipulating nr_cpu_ids.
> > >
> > > If changing this fixes the bug, something else is badly wrong...
> > > Rusty.
> >
> > In function _cpu_up, the panic happens when calling __raw_notifier_call_chain
> > at the second time. Kernel doesn't panic when calling it at the first time. If
> > just say because ___of nr_cpu_ids, that's not right.
> >
> > By checking source codes, I find function do_boot_cpu is the culprit.
> > Consider below call chain:
> > _cpu_up=>__cpu_up=>smp_ops.cpu_up=>native_cpu_up=>do_boot_cpu.
> >
> > So ___do_boot_cpu is called in the end. In ___do_boot_cpu, if boot_error==true,
> > cpu_clear(cpu, cpu_possible_map) is executed. So later on, when ____cpu_up
> > calls _____raw_notifier_call_chain at the second time to report CPU_UP_CANCELED,
> > because this cpu is already cleared from ___cpu_possible_map, get_cpu_sysdev returns
> > NULL.
> >
> > Many resources are related to ___cpu_possible_map, so it's better not to change it.
> >
> > Below patch against 2.6.26-rc7 fixes it by removing the bit clearing in ___cpu_possible_map.
> >
> > Vegard, would you like to help test it?
> >
> > _________Signed-off-by: Zhang Yanmin ___<yanmin_zhang@linux.intel.com>
> >
> > ---
> >
> > diff -Nraup linux-2.6.26-rc7/arch/x86/kernel/smpboot.c linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c
> > --- linux-2.6.26-rc7/arch/x86/kernel/smpboot.c 2008-06-24 09:03:54.000000000 +0800
> > +++ linux-2.6.26-rc7_cpuhotplug/arch/x86/kernel/smpboot.c 2008-06-24 09:04:45.000000000 +0800
> > @@ -996,7 +996,6 @@ do_rest:
> > #endif
> > cpu_clear(cpu, cpu_callout_map); /* was set by do_boot_cpu() */
> > cpu_clear(cpu, cpu_initialized); /* was set by cpu_init() */
> > - cpu_clear(cpu, cpu_possible_map);
> > cpu_clear(cpu, cpu_present_map);
> > per_cpu(x86_cpu_to_apicid, cpu) = BAD_APICID;
> > }
> >
> Andrew,
>
> Would you like to pick up this patch? ___Rusty Russell <rusty@rustcorp.com.au> acked it.
>
Could. But arch/x86/kernel/smpboot.c is an x86-tree file. I'd expect
the x86 maintainers would like a usable changelog and a Tested-by: (if
indeed Vegard tested it).
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-26 2:15 ` Andrew Morton
@ 2008-06-26 9:00 ` Vegard Nossum
2008-06-26 12:40 ` Jason Wessel
0 siblings, 1 reply; 24+ messages in thread
From: Vegard Nossum @ 2008-06-26 9:00 UTC (permalink / raw)
To: Andrew Morton
Cc: Zhang, Yanmin, Ingo Molnar, Mike Travis, Adrian Bunk,
Srivatsa Vaddagiri, linux-kernel, Gautham R Shenoy,
Rafael J. Wysocki, Zhang, Yanmin, Heiko Carstens, Rusty Russell
On Thu, Jun 26, 2008 at 4:15 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> Could. But arch/x86/kernel/smpboot.c is an x86-tree file. I'd expect
> the x86 maintainers would like a usable changelog and a Tested-by: (if
> indeed Vegard tested it).
Hi,
I had the patch (on top of v2.6.26-rc8) in testing for a couple of
hours now, but like I expected, the APIC error is really hard to
reproduce. So I fear that we never hit the failure case in the first
place. On the other hand... I hit a number of (other?) problems:
1. Spontaneous reboot. This could have been the APIC error thing, I
have no way to know. I've never seen this before.
2. NULL pointer dereference in pick_next_task_fair(). Not new.
3. NULL pointer dereference in page_cgroup_zoneinfo. Never seen this
before (see screenshot #1).
4. Page fault in I'm guessing configure_kgdbts() during boot. Never
seen this before either (screenshot #2).
So even though I'm inclined to believe the patch is correct, I will
not ack it or claim successful test coverage.
The screenshots:
#1: http://www.kernel.org/pub/linux/kernel/people/vegard/kernels/20080626/DSCF3017.JPG
#2: http://www.kernel.org/pub/linux/kernel/people/vegard/kernels/20080626/DSCF3018.JPG
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-26 9:00 ` Vegard Nossum
@ 2008-06-26 12:40 ` Jason Wessel
2008-06-26 13:59 ` Vegard Nossum
0 siblings, 1 reply; 24+ messages in thread
From: Jason Wessel @ 2008-06-26 12:40 UTC (permalink / raw)
To: Vegard Nossum
Cc: Andrew Morton, Zhang, Yanmin, Ingo Molnar, Mike Travis,
Adrian Bunk, Srivatsa Vaddagiri, linux-kernel, Gautham R Shenoy,
Rafael J. Wysocki, Zhang, Yanmin, Heiko Carstens, Rusty Russell
Vegard Nossum wrote:
> On Thu, Jun 26, 2008 at 4:15 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>
>> Could. But arch/x86/kernel/smpboot.c is an x86-tree file. I'd expect
>> the x86 maintainers would like a usable changelog and a Tested-by: (if
>> indeed Vegard tested it).
>>
>
> Hi,
>
> I had the patch (on top of v2.6.26-rc8) in testing for a couple of
> hours now, but like I expected, the APIC error is really hard to
> reproduce. So I fear that we never hit the failure case in the first
> place. On the other hand... I hit a number of (other?) problems:
>
> 1. Spontaneous reboot. This could have been the APIC error thing, I
> have no way to know. I've never seen this before.
> 2. NULL pointer dereference in pick_next_task_fair(). Not new.
> 3. NULL pointer dereference in page_cgroup_zoneinfo. Never seen this
> before (see screenshot #1).
> 4. Page fault in I'm guessing configure_kgdbts() during boot. Never
> seen this before either (screenshot #2).
>
How often does #4 occur?
Perhaps you can email me directly the .config and kernel boot arguments
you used? It would be good to determine if the #4 is a victim or
culprit of the crash. If it is a culprit I would like to try and fix it.
Thanks,
Jason.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-26 12:40 ` Jason Wessel
@ 2008-06-26 13:59 ` Vegard Nossum
0 siblings, 0 replies; 24+ messages in thread
From: Vegard Nossum @ 2008-06-26 13:59 UTC (permalink / raw)
To: Jason Wessel
Cc: Andrew Morton, Zhang, Yanmin, Ingo Molnar, Mike Travis,
Adrian Bunk, Srivatsa Vaddagiri, linux-kernel, Gautham R Shenoy,
Rafael J. Wysocki, Zhang, Yanmin, Heiko Carstens, Rusty Russell
On Thu, Jun 26, 2008 at 2:40 PM, Jason Wessel
<jason.wessel@windriver.com> wrote:
>> 4. Page fault in I'm guessing configure_kgdbts() during boot. Never
>> seen this before either (screenshot #2).
>>
>
> How often does #4 occur?
Only this once yet.
>
> Perhaps you can email me directly the .config and kernel boot arguments
> you used? It would be good to determine if the #4 is a victim or
> culprit of the crash. If it is a culprit I would like to try and fix it.
I don't think this really has anything to do with the CPU hotplug
crashing. It seems to be completely spurious, as this was during boot
before userspace had even started. It's not even certain that it has
anything to do with kgdb at all. I mean, that stack trace is not very
reliable...
Command line was: kernel /vmlinuz-2.6.26-rc8-dirty ro
root=/dev/VolGroup00/LogVol00 rhgb debug 3
You can find vmlinux and config here:
http://www.kernel.org/pub/linux/kernel/people/vegard/kernels/20080626/
..but it probably takes a bit for the mirrors to pick up. In the
meantime, I can provide you with the relevant line numbers:
# configure_kgdbts
$ addr2line -e vmlinux.20080626 -i c030ab18 | sed 's/.*linux-2.6\///'
drivers/misc/kgdbts.c:911
drivers/misc/kgdbts.c:1015
# mtrr_del
$ addr2line -e vmlinux.20080626 -i c011007b | sed 's/.*linux-2.6\///'
arch/x86/kernel/cpu/mtrr/main.c:548
# loop_init
$ addr2line -e vmlinux.20080626 -i c078111a | sed 's/.*linux-2.6\///'
drivers/block/loop.c:1550
# init_kgdbts
$ addr2line -e vmlinux.20080626 -i c0781163 | sed 's/.*linux-2.6\///'
drivers/misc/kgdbts.c:1033
Thanks.
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference
2008-06-24 8:06 ` Zhang, Yanmin
` (2 preceding siblings ...)
2008-06-26 0:59 ` Zhang, Yanmin
@ 2008-07-10 19:10 ` Vegard Nossum
3 siblings, 0 replies; 24+ messages in thread
From: Vegard Nossum @ 2008-07-10 19:10 UTC (permalink / raw)
To: Zhang, Yanmin
Cc: Rusty Russell, Mike Travis, Adrian Bunk, Srivatsa Vaddagiri,
linux-kernel, Gautham R Shenoy, Rafael J. Wysocki, Zhang, Yanmin,
Heiko Carstens
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1802 bytes --]
On Tue, Jun 24, 2008 at 10:06 AM, Zhang, Yanmin<yanmin_zhang@linux.intel.com> wrote:> In function _cpu_up, the panic happens when calling __raw_notifier_call_chain> at the second time. Kernel doesn't panic when calling it at the first time. If> just say because of nr_cpu_ids, that's not right.>> By checking source codes, I find function do_boot_cpu is the culprit.> Consider below call chain:> _cpu_up=>__cpu_up=>smp_ops.cpu_up=>native_cpu_up=>do_boot_cpu.>> So do_boot_cpu is called in the end. In do_boot_cpu, if boot_error==true,> cpu_clear(cpu, cpu_possible_map) is executed. So later on, when _cpu_up> calls __raw_notifier_call_chain at the second time to report CPU_UP_CANCELED,> because this cpu is already cleared from cpu_possible_map, get_cpu_sysdev returns> NULL.>> Many resources are related to cpu_possible_map, so it's better not to change it.>> Below patch against 2.6.26-rc7 fixes it by removing the bit clearing in cpu_possible_map.>> Vegard, would you like to help test it?
Yay! I just hit this again with your patch applied
Inquiring remote APIC #1...... APIC #1 ID: failed... APIC #1 VERSION: failed... APIC #1 SPIV: failed
and it works correctly, no NULL pointer error this time :-)
I know it's applied to mainline already, actually with this tag too,even though I wasn't able to confirm it before:
Tested-by: Vegard Nossum <vegard.nossum@gmail.com>
Thanks :-)
Vegard
-- "The animistic metaphor of the bug that maliciously sneaked in whilethe programmer was not looking is intellectually dishonest as itdisguises that the error is the programmer's own creation." -- E. W. Dijkstra, EWD1036ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 24+ messages in thread
