Re: linux-next: Tree for June 5

Vegard Nossum wrote:

On Fri, Jun 6, 2008 at 4:20 PM, Mike Travis [off-list ref] wrote:

quoted

Vegard Nossum wrote:

quoted

On Fri, Jun 6, 2008 at 3:50 PM, Vegard Nossum [off-list ref] wrote:

quoted

On Fri, Jun 6, 2008 at 3:33 PM, Mike Travis [off-list ref] wrote:

quoted

Vegard Nossum wrote:

quoted

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).

...

quoted

quoted

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.

...

quoted

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