cpumask: fix cpumask leak in partition_sched_domains

PATCH In review Xiaotian Feng 4 participants 8 messages Jul 27, 2013 – Aug 16, 2013 Scheduler

Share & tools

Xiaotian Feng

author

2013-07-27 07:27 UTC #0 lore raw

PATCH 1/1 cpumask: fix cpumask leak in partition_sched_domains

If doms_new is NULL, partition_sched_domains() will reset ndoms_cur to 0, and free old sched domains with free_sched_domains(doms_cur, ndoms_cur). As ndoms_cur is 0, the cpumask will not be freed.

Signed-off-by Xiaotian Feng <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc [email protected]

---
 kernel/sched/core.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/sched/core.c +3 -2

index b7c32cb..3d6c57b 100644
@@ -6184,8 +6184,9 @@ match1:
 		;
 	}
 
+	n= ndoms_cur;
 	if (doms_new == NULL) {
-		ndoms_cur = 0;
+		n = 0;
 		doms_new = &fallback_doms;
 		cpumask_andnot(doms_new[0], cpu_active_mask, cpu_isolated_map);
 		WARN_ON_ONCE(dattr_new);

@@ -6193,7 +6194,7 @@ match1:
 
 	/* Build new domains */
 	for (i = 0; i < ndoms_new; i++) {
-		for (j = 0; j < ndoms_cur && !new_topology; j++) {
+		for (j = 0; j < n && !new_topology; j++) {
 			if (cpumask_equal(doms_new[i], doms_cur[j])
 			    && dattrs_equal(dattr_new, i, dattr_cur, j))
 				goto match2;

Signature · 1 line

1.7.9.6 (Apple Git-31.1)

Xiaotian Feng

author

2013-08-06 02:31 UTC #1 lore raw

Quoted 8 lines · Xiaotian Feng

If doms_new is NULL, partition_sched_domains() will reset ndoms_cur to 0, and free old sched domains with free_sched_domains(doms_cur, ndoms_cur). As ndoms_cur is 0, the cpumask will not be freed.
Signed-off-by Xiaotian Feng <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc [email protected]

Any comments? Cc'ed Rusty.

Quoted 31 lines

---
 kernel/sched/core.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/sched/core.c +3 -3

index b7c32cb..3d6c57b 100644
@@ -6184,8 +6184,9 @@ match1:
                ;
        }
 
+       n= ndoms_cur;
        if (doms_new == NULL) {
-               ndoms_cur = 0;
+               n = 0;
                doms_new = &fallback_doms;
                cpumask_andnot(doms_new[0], cpu_active_mask, cpu_isolated_map);
                WARN_ON_ONCE(dattr_new);

@@ -6193,7 +6194,7 @@ match1:
 
        /* Build new domains */
        for (i = 0; i < ndoms_new; i++) {
-               for (j = 0; j < ndoms_cur && !new_topology; j++) {
+               for (j = 0; j < n && !new_topology; j++) {
                        if (cpumask_equal(doms_new[i], doms_cur[j])
                            && dattrs_equal(dattr_new, i, dattr_cur, j))
                                goto match2;
--

1.7.9.6 (Apple Git-31.1)

Rusty Russell

2013-08-06 04:40 UTC #2 lore raw

Quoted 11 lines · Xiaotian Feng

Quoted 8 lines · Xiaotian Feng
If doms_new is NULL, partition_sched_domains() will reset ndoms_cur to 0, and free old sched domains with free_sched_domains(doms_cur, ndoms_cur). As ndoms_cur is 0, the cpumask will not be freed.
Signed-off-by Xiaotian Feng <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc [email protected]
Any comments? Cc'ed Rusty.

The code is a little convoluted, but your fix is logical.

Quoted 12 lines

---
 kernel/sched/core.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/sched/core.c +0 -0

index b7c32cb..3d6c57b 100644
@@ -6184,8 +6184,9 @@ match1:
                ;
        }

Quoted 1 line

kernel/sched/core.c match1: · line 6187
Quoted 1 line
kernel/sched/core.c +1 -0
+       n= ndoms_cur;

You're missing a ' ' here:

        n = ndoms_cur;

Quoted 8 lines

kernel/sched/core.c +1 -1

        if (doms_new == NULL) {
-               ndoms_cur = 0;
+               n = 0;
                doms_new = &fallback_doms;
                cpumask_andnot(doms_new[0], cpu_active_mask, cpu_isolated_map);
                WARN_ON_ONCE(dattr_new);

@@ -6193,7 +6194,7 @@ match1:

Quoted 10 lines

kernel/sched/core.c +1 -2

        /* Build new domains */
        for (i = 0; i < ndoms_new; i++) {
-               for (j = 0; j < ndoms_cur && !new_topology; j++) {
+               for (j = 0; j < n && !new_topology; j++) {
                        if (cpumask_equal(doms_new[i], doms_cur[j])
                            && dattrs_equal(dattr_new, i, dattr_cur, j))
                                goto match2;
--

1.7.9.6 (Apple Git-31.1)

Cheers,
Rusty.

Xiaotian Feng

author

2013-08-06 05:10 UTC #3 lore raw

Quoted 6 lines · Rusty Russell

Quoted 5 lines · Xiaotian Feng
Quoted 4 lines · Xiaotian Feng
If doms_new is NULL, partition_sched_domains() will reset ndoms_cur to 0, and free old sched domains with free_sched_domains(doms_cur, ndoms_cur). As ndoms_cur is 0, the cpumask will not be freed.

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
Signed-off-by Xiaotian Feng <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc [email protected]
Any comments? Cc'ed Rusty.
The code is a little convoluted, but your fix is logical.

Yes, it's quite convoluted :(

Quoted 12 lines

---
 kernel/sched/core.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/sched/core.c +0 -0

index b7c32cb..3d6c57b 100644
@@ -6184,8 +6184,9 @@ match1:
                ;
        }

Quoted 5 lines

Quoted 1 line
kernel/sched/core.c match1: · line 6187
Quoted 1 line
kernel/sched/core.c +1 -0
+       n= ndoms_cur;
You're missing a ' ' here:
        n = ndoms_cur;

I'll update this, thanks :)

Quoted 21 lines

Quoted 8 lines

kernel/sched/core.c +1 -1

        if (doms_new == NULL) {
-               ndoms_cur = 0;
+               n = 0;
                doms_new = &fallback_doms;
                cpumask_andnot(doms_new[0], cpu_active_mask, cpu_isolated_map);
                WARN_ON_ONCE(dattr_new);

@@ -6193,7 +6194,7 @@ match1:

Quoted 10 lines

kernel/sched/core.c +1 -2

        /* Build new domains */
        for (i = 0; i < ndoms_new; i++) {
-               for (j = 0; j < ndoms_cur && !new_topology; j++) {
+               for (j = 0; j < n && !new_topology; j++) {
                        if (cpumask_equal(doms_new[i], doms_cur[j])
                            && dattrs_equal(dattr_new, i, dattr_cur, j))
                                goto match2;
--

1.7.9.6 (Apple Git-31.1)

Cheers,
Rusty.

Xiaotian Feng

author

2013-08-06 12:08 UTC #4 lore raw

PATCH 1/1 cpumask: fix cpumask leak in partition_sched_domains

In reply to Rusty Russell

Signed-off-by Xiaotian Feng <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Rusty Russell <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc [email protected]

---
 kernel/sched/core.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/sched/core.c +3 -2

index b7c32cb..3d6c57b 100644
@@ -6184,8 +6184,9 @@ match1:
 		;
 	}
 
+	n = ndoms_cur;
 	if (doms_new == NULL) {
-		ndoms_cur = 0;
+		n = 0;
 		doms_new = &fallback_doms;
 		cpumask_andnot(doms_new[0], cpu_active_mask, cpu_isolated_map);
 		WARN_ON_ONCE(dattr_new);

@@ -6193,7 +6194,7 @@ match1:
 
 	/* Build new domains */
 	for (i = 0; i < ndoms_new; i++) {
-		for (j = 0; j < ndoms_cur && !new_topology; j++) {
+		for (j = 0; j < n && !new_topology; j++) {
 			if (cpumask_equal(doms_new[i], doms_cur[j])
 			    && dattrs_equal(dattr_new, i, dattr_cur, j))
 				goto match2;

Signature · 1 line

1.7.9.6 (Apple Git-31.1)

Xiaotian Feng

author

2013-08-15 01:55 UTC #5 lore raw

Quoted 10 lines · Xiaotian Feng

If doms_new is NULL, partition_sched_domains() will reset ndoms_cur to 0, and free old sched domains with free_sched_domains(doms_cur, ndoms_cur). As ndoms_cur is 0, the cpumask will not be freed.
Signed-off-by Xiaotian Feng <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Rusty Russell <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc [email protected]

Any comments on this patch? Without this patch, I can see following with kmemleak.

unreferenced object 0xffff880118d26aa8 (size 512):
comm "swapper/0", pid 1, jiffies 4294892366 (age 287.736s) hex dump (first 32 bytes):

    0f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff817350e6>] kmemleak_alloc+0x26/0x50
    [<ffffffff811b92c6>] kmem_cache_alloc_node_trace+0x116/0x2d0
    [<ffffffff8139e66f>] alloc_cpumask_var_node+0x1f/0x90
    [<ffffffff8139e6ee>] alloc_cpumask_var+0xe/0x10
    [<ffffffff810a328c>] alloc_sched_domains+0x5c/0x80
    [<ffffffff81daf8c6>] sched_init_smp+0x365/0x47d
    [<ffffffff81d8f01e>] kernel_init_freeable+0xe3/0x1ef
    [<ffffffff81731b1e>] kernel_init+0xe/0xf0
    [<ffffffff817543ac>] ret_from_fork+0x7c/0xb0
    [<ffffffffffffffff>] 0xffffffffffffffff

Quoted 31 lines

---
 kernel/sched/core.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/sched/core.c +3 -3

index b7c32cb..3d6c57b 100644
@@ -6184,8 +6184,9 @@ match1:
                ;
        }
 
+       n = ndoms_cur;
        if (doms_new == NULL) {
-               ndoms_cur = 0;
+               n = 0;
                doms_new = &fallback_doms;
                cpumask_andnot(doms_new[0], cpu_active_mask, cpu_isolated_map);
                WARN_ON_ONCE(dattr_new);

@@ -6193,7 +6194,7 @@ match1:
 
        /* Build new domains */
        for (i = 0; i < ndoms_new; i++) {
-               for (j = 0; j < ndoms_cur && !new_topology; j++) {
+               for (j = 0; j < n && !new_topology; j++) {
                        if (cpumask_equal(doms_new[i], doms_cur[j])
                            && dattrs_equal(dattr_new, i, dattr_cur, j))
                                goto match2;
--

1.7.9.6 (Apple Git-31.1)

Peter Zijlstra

maintainer

2013-08-15 09:09 UTC #6 lore raw

In reply to Xiaotian Feng

Quoted 10 lines · Xiaotian Feng

If doms_new is NULL, partition_sched_domains() will reset ndoms_cur to 0, and free old sched domains with free_sched_domains(doms_cur, ndoms_cur). As ndoms_cur is 0, the cpumask will not be freed.
Signed-off-by Xiaotian Feng <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Rusty Russell <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc [email protected]

Thanks!

tip-bot for Xiaotian Feng

merge bot

2013-08-16 18:47 UTC #7 lore raw

[tip:sched/core] cpumask: Fix cpumask leak in partition_sched_domains()

In reply to Xiaotian Feng

cpumask: Fix cpumask leak in partition_sched_domains()

Commit-ID:  c8d2d47a9cbb4222ae4e993aa0e3703430c8193c
Gitweb:     http://git.kernel.org/tip/c8d2d47a9cbb4222ae4e993aa0e3703430c8193c
Author:     Xiaotian Feng <[email protected]>
AuthorDate: Tue, 6 Aug 2013 20:06:42 +0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 16 Aug 2013 17:44:27 +0200

cpumask: Fix cpumask leak in partition_sched_domains()

Signed-off-by Xiaotian Feng <[email protected]>

Cc Rusty Russell <[email protected]>

Cc [email protected]

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/sched/core.c +3 -2

index b7415cf..cf8f100 100644
@@ -6184,8 +6184,9 @@ match1:
 		;
 	}
 
+	n = ndoms_cur;
 	if (doms_new == NULL) {
-		ndoms_cur = 0;
+		n = 0;
 		doms_new = &fallback_doms;
 		cpumask_andnot(doms_new[0], cpu_active_mask, cpu_isolated_map);
 		WARN_ON_ONCE(dattr_new);

@@ -6193,7 +6194,7 @@ match1:
 
 	/* Build new domains */
 	for (i = 0; i < ndoms_new; i++) {
-		for (j = 0; j < ndoms_cur && !new_topology; j++) {
+		for (j = 0; j < n && !new_topology; j++) {
 			if (cpumask_equal(doms_new[i], doms_cur[j])
 			    && dattrs_equal(dattr_new, i, dattr_cur, j))
 				goto match2;