Re: linux-next: Tree for June 5

[Mike Travis <travis@sgi.com>]

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