Re: linux-next: Tree for June 5

[Mike Travis <travis@sgi.com>]

Mike Travis wrote:
> Mike Travis wrote:
>> 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
> 
> Aha, here's the missing patch:
> 
> a953e4597abd51b74c99e0e3b7074532a60fd031
> 

Oops, message got away from me prematurely... ;-)

Ingo - can we push this from tip to linux-next?

Thanks,
Mike