Re: linux-next: Tree for June 5

[Mike Travis <travis@sgi.com>]

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