Messages in this thread | | | Date | Fri, 06 Jun 2008 07:41:38 -0700 | From | Mike Travis <> | Subject | Re: linux-next: Tree for June 5 |
| |
Vegard Nossum wrote: > On Fri, Jun 6, 2008 at 4:20 PM, Mike Travis <travis@sgi.com> wrote: >> Vegard Nossum wrote: >>> On Fri, Jun 6, 2008 at 3:50 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote: >>>> On Fri, Jun 6, 2008 at 3:33 PM, Mike Travis <travis@sgi.com> wrote: >>>>> Vegard Nossum wrote: >>>>>> I reproced it with gc 4.1.2. I think the error is somewhere in kernel/sched.c. >>>>>> >>>>>> static int __build_sched_domains(const cpumask_t *cpu_map, >>>>>> struct sched_domain_attr *attr) >>>>>> { >>>>>> ... >>>>>> for (i = 0; i < MAX_NUMNODES; i++) { >>>>>> ... >>>>>> sg = kmalloc_node(sizeof(struct sched_group), GFP_KERNEL, i); >>>>>> ... >>>>>> >>>>>> This code is calling into the allocator with a spurious value of i, >>>>>> which causes SLAB to use an index (of 4 in my case) that is out of >>>>>> bounds for its nodelist array (at least it hasn't been initialized). >>>>>> > > ... > >>> The error is of course that the node masks for nodes > nr_node_ids are >>> not valid. While this function ignores that: >>> >>> cpumask_t *_node_to_cpumask_ptr(int node) >>> { >>> if (node_to_cpumask_map == NULL) { >>> printk(KERN_WARNING >>> "_node_to_cpumask_ptr(%d): no node_to_cpumask_map!\n", >>> node); >>> dump_stack(); >>> return &cpu_online_map; >>> } >>> return &node_to_cpumask_map[node]; >>> } >>> EXPORT_SYMBOL(_node_to_cpumask_ptr); >>> >>> Notice the return statement. It needs to check if node < nr_node_ids. >>> > > ... > >> Thanks, yes I had that some after thought. It should check the node >> index if CONFIG_DEBUG_PER_CPU_MAPS is enabled. One gotcha is that >> nr_node_ids is intialized to MAX_NUMNODES until setup_node_to_cpumask_map() >> sets it to the correct value. So uses before that should be caught by >> the earlier check. > > I think it should always check the node index. The code in > kernel/sched.c (see above) calls node_to_cpumask(i) on nodes 0 < i < > MAX_NUMNODES and it WILL use invalid pointers. Or should > kernel/sched.c be changed to use nr_node_ids instead of MAX_NUMNODES? > I believe there are more places that do this than just sched.c.
Yes, using MAX_NUMNODES is usually incorrect (the same for NR_CPUS). When I originally submitted the patch I searched for all usages to make sure they were correct. Unfortunately, later changes might not have been validated. (Hmm, maybe adding to checkpatch.pl a similar warning as it now does for NR_CPUS...?)
> > I have attached two patches. The sched one fixes Andrew's boot > problem. The x86 one is untested, but I believe it is better to BUG > than silently corrupt some arbitrary memory. (Then the callers can be > found easily and fixed at least.)
Andrew (or maybe it was Ingo) had suggested that instead of BUG use dump_stack() and continue whenever possible. In this case returning an empty cpumask would be correct.
Thanks, Mike
| |