Messages in this thread Patch in this message | | | Date | Sun, 22 Jun 2008 18:29:07 +0200 | From | "Vegard Nossum" <> | Subject | Re: v2.6.26-rc7: BUG: unable to handle kernel NULL pointer dereference |
| |
On Sun, Jun 22, 2008 at 5:56 PM, Adrian Bunk <bunk@kernel.org> wrote: >> commit e37d05dad7ff9744efd8ea95a70d389e9a65a6fc >> Author: Mike Travis <travis@sgi.com> >> Date: Thu May 1 04:35:16 2008 -0700 >> >> cpu: change cpu_sys_devices from array to per_cpu variable >> >> Change cpu_sys_devices from array to per_cpu variable in drivers/base/cpu.c. >>... > > Can you confirm whether this is definitely the cause or not? > > E.g. if it is and 2.6.25 works fine it might qualify as a 2.6.26-rc > regression.
Hm, no. Each time I run this test, I get a different error (has been NULL pointer, stuck CPU, circular locking dependency, ...) :-D
But if you look at the patch, I KNOW that this function is the one that returns NULL, and it does so because the check is now stricter than before. The hunk was:
- if (cpu < NR_CPUS) - return cpu_sys_devices[cpu]; + if (cpu < nr_cpu_ids && cpu_possible(cpu)) + return per_cpu(cpu_sys_devices, cpu); else return NULL;
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, so I don't know what the right fix would be. A straight revert would be possible, but probably not desirable. I'm so definitely not an expert in this area, but this "fix" _looks_ correct to me:
diff --git a/drivers/base/topology.c b/drivers/base/topology.c index fdf4044..3bd95fd 100644 --- a/drivers/base/topology.c +++ b/drivers/base/topology.c @@ -143,14 +143,10 @@ static int __cpuinit topology_cpu_callback(struct notifier int rc = 0;
switch (action) { - case CPU_UP_PREPARE: - case CPU_UP_PREPARE_FROZEN: + case CPU_ONLINE: rc = topology_add_dev(cpu); break; - case CPU_UP_CANCELED: - case CPU_UP_CANCELED_FROZEN: - case CPU_DEAD: - case CPU_DEAD_FROZEN: + case CPU_DOWN_PREPARE: topology_remove_dev(cpu); break; } I'm sorry, I can't really say whether it's a regression or not. But I'd bet it is.
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
| |