Re: linux-next: Tree for June 5

Vegard Nossum
2008-06-06T14:36:20Z

[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]

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.

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

Vegard

--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-sched-don-t-call-node_to_cpumask-on-nodes-nr_no.patch --]
[-- Type: text/x-patch; name=0001-sched-don-t-call-node_to_cpumask-on-nodes-nr_no.patch, Size: 1830 bytes --]

From 216dcbdec79d76c4d738f2c0aad41061f80564e4 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegardno@ben.ifi.uio.no>
Date: Fri, 6 Jun 2008 16:31:19 +0200
Subject: [PATCH] sched: don't call node_to_cpumask() on nodes > nr_node_ids

Signed-off-by: Vegard Nossum <vegardno@ben.ifi.uio.no>
---
kernel/sched.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index fc9ba90..8ab9cd6 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -6770,7 +6770,7 @@ static void free_sched_groups(const cpumask_t *cpu_map, cpumask_t *nodemask)
if (!sched_group_nodes)
continue;

- for (i = 0; i < MAX_NUMNODES; i++) {
+ for (i = 0; i < nr_node_ids; i++) {
struct sched_group *oldsg, *sg = sched_group_nodes[i];

*nodemask = node_to_cpumask(i);
@@ -7097,7 +7097,7 @@ static int __build_sched_domains(const cpumask_t *cpu_map,
#endif

/* Set up physical groups */
- for (i = 0; i < MAX_NUMNODES; i++) {
+ for (i = 0; i < nr_node_ids; i++) {
SCHED_CPUMASK_VAR(nodemask, allmasks);
SCHED_CPUMASK_VAR(send_covered, allmasks);

@@ -7121,7 +7121,7 @@ static int __build_sched_domains(const cpumask_t *cpu_map,
send_covered, tmpmask);
}

- for (i = 0; i < MAX_NUMNODES; i++) {
+ for (i = 0; i < nr_node_ids; i++) {
/* Set up node groups */
struct sched_group *sg, *prev;
SCHED_CPUMASK_VAR(nodemask, allmasks);
@@ -7160,9 +7160,9 @@ static int __build_sched_domains(const cpumask_t *cpu_map,
cpus_or(*covered, *covered, *nodemask);
prev = sg;

- for (j = 0; j < MAX_NUMNODES; j++) {
+ for (j = 0; j < nr_node_ids; j++) {
SCHED_CPUMASK_VAR(notcovered, allmasks);
- int n = (i + j) % MAX_NUMNODES;
+ int n = (i + j) % nr_node_ids;
node_to_cpumask_ptr(pnodemask, n);

cpus_complement(*notcovered, *covered);
--
1.5.3.1

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-x86-don-t-return-invalid-pointers-from-node_to_cpum.patch --]
[-- Type: text/x-patch; name=0001-x86-don-t-return-invalid-pointers-from-node_to_cpum.patch, Size: 966 bytes --]

From b993e7349b954555715c2adad690711465bbd60c Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Fri, 6 Jun 2008 16:33:25 +0200
Subject: [PATCH] x86: don't return invalid pointers from node_to_cpumask()

Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
arch/x86/kernel/setup.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 8ecf7b4..8411c55 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -385,6 +385,7 @@ cpumask_t *_node_to_cpumask_ptr(int node)
dump_stack();
return &cpu_online_map;
}
+ BUG_ON(node >= nr_node_ids);
return &node_to_cpumask_map[node];
}
EXPORT_SYMBOL(_node_to_cpumask_ptr);
@@ -400,6 +401,7 @@ cpumask_t node_to_cpumask(int node)
dump_stack();
return cpu_online_map;
}
+ BUG_ON(node >= nr_node_ids);
return node_to_cpumask_map[node];
}
EXPORT_SYMBOL(node_to_cpumask);
--
1.5.4.1