clk: mvebu/clk-cpu.c: fix memory leakage

the variable cpuclk and clk_name should be properly freed.

Signed-off-by Cong Ding <[email protected]>

---
 drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/clk/mvebu/clk-cpu.c +1 -1

index ff004578..1a0d84f 100644
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;

Dear Cong Ding,

Quoted 2 lines · Cong Ding

the variable cpuclk and clk_name should be properly freed.

Thanks for reporting this memory leak and for your patch but I think we could do even better, see below:

Quoted 19 lines

Signed-off-by Cong Ding <[email protected]>

---
 drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/clk/mvebu/clk-cpu.c +1 -1

index ff004578..1a0d84f 100644
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 137

Quoted 5 lines

drivers/clk/mvebu/clk-cpu.c +1 -1

@@ -134,11 +134,11 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		if (WARN_ON(!clk_name))
-			return;
+			goto clk_name_out;

I agree

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 141

Quoted 5 lines

drivers/clk/mvebu/clk-cpu.c +1 -1

 		err = of_property_read_u32(dn, "reg", &cpu);
 		if (WARN_ON(err))
-			return;
+			goto bail_out;

I agree

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 169

Quoted 8 lines

drivers/clk/mvebu/clk-cpu.c +1 -0

 		sprintf(clk_name, "cpu%d", cpu);
 		parent_clk = of_clk_get(node, 0);

@@ -166,7 +166,10 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	return;
 bail_out:
+	kfree(clk_name);

Here you free only one clk_name whereas we could have previous clk_name which have been already allocated during the for_each_node_by_type loop.

A more annoying thing is that you use clk_name whereas it is only valid in the for_each_node_by_type statement and then this patch breaks the compilation:

drivers/clk/mvebu/clk-cpu.c: In function ?of_cpu_clk_setup?:
drivers/clk/mvebu/clk-cpu.c:169:8: error: ?clk_name? undeclared (first use in this function) drivers/clk/mvebu/clk-cpu.c:169:8: note: each undeclared identifier is reported only once for each function it appears in

From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001

From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
From: Cong Ding <[email protected]>
Date: Mon, 14 Jan 2013 18:06:26 +0100
Subject: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

the variable cpuclk and clk_name should be properly freed when error happens.

Signed-off-by Cong Ding <[email protected]>

---
 drivers/clk/mvebu/clk-cpu.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

drivers/clk/mvebu/clk-cpu.c +10 -5

index ff004578..1066a43 100644
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;

@@ -134,11 +134,13 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		if (WARN_ON(!clk_name))
-			return;
+			goto bail_out;
 
 		err = of_property_read_u32(dn, "reg", &cpu);
-		if (WARN_ON(err))
-			return;
+		if (WARN_ON(err)) {
+			kfree(clk_name);
+			goto bail_out;
+		}
 
 		sprintf(clk_name, "cpu%d", cpu);
 		parent_clk = of_clk_get(node, 0);

@@ -156,8 +158,10 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		init.num_parents = 1;
 
 		clk = clk_register(NULL, &cpuclk[cpu].hw);
-		if (WARN_ON(IS_ERR(clk)))
+		if (WARN_ON(IS_ERR(clk))) {
+			kfree(clk_name);
 			goto bail_out;
+		}
 		clks[cpu] = clk;
 	}
 	clk_data.clk_num = MAX_CPU;

@@ -167,6 +171,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 	return;
 bail_out:
 	kfree(clks);
+clks_out:
 	kfree(cpuclk);
 }
 

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Mike,

Quoted 11 lines · Cong Ding

From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
From: Cong Ding <[email protected]>
Date: Mon, 14 Jan 2013 18:06:26 +0100
Subject: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

the variable cpuclk and clk_name should be properly freed when error happens.

Signed-off-by Cong Ding <[email protected]>

---
 drivers/clk/mvebu/clk-cpu.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Do you want to take this fix through the clock tree? If so,

Acked-by Jason Cooper <[email protected]>

Otherwise, just let me know.

thx,

Jason.

Quoted 20 lines

drivers/clk/mvebu/clk-cpu.c +2 -2

index ff004578..1066a43 100644
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;

@@ -134,11 +134,13 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		if (WARN_ON(!clk_name))
-			return;
+			goto bail_out;
 

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node)

Quoted 6 lines

drivers/clk/mvebu/clk-cpu.c +2 -2

 		err = of_property_read_u32(dn, "reg", &cpu);
-		if (WARN_ON(err))
-			return;
+		if (WARN_ON(err)) {
 
+			kfree(clk_name);

Quoted 8 lines

drivers/clk/mvebu/clk-cpu.c +2 -0

+			goto bail_out;
+		}
 
 		sprintf(clk_name, "cpu%d", cpu);

drivers/clk/mvebu/clk-cpu.c +0 -0

 		parent_clk = of_clk_get(node, 0);

@@ -156,8 +158,10 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		init.num_parents = 1;
 

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 162

Quoted 5 lines

drivers/clk/mvebu/clk-cpu.c +2 -1

 		clk = clk_register(NULL, &cpuclk[cpu].hw);
-		if (WARN_ON(IS_ERR(clk)))
+		if (WARN_ON(IS_ERR(clk))) {
 
+			kfree(clk_name);

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 174

Quoted 10 lines

drivers/clk/mvebu/clk-cpu.c +1 -0

 			goto bail_out;
+		}
 		clks[cpu] = clk;
 	}

drivers/clk/mvebu/clk-cpu.c +1 -0

 	clk_data.clk_num = MAX_CPU;

@@ -167,6 +171,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 	return;
 bail_out:
 	kfree(clks);
+clks_out:

Quoted 11 lines

drivers/clk/mvebu/clk-cpu.c +0 -0

 	kfree(cpuclk);
 }
 

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 16 lines · Jason Cooper

Mike,

Quoted 11 lines · Cong Ding

From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
From: Cong Ding <[email protected]>
Date: Mon, 14 Jan 2013 18:06:26 +0100
Subject: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

the variable cpuclk and clk_name should be properly freed when error happens.

Signed-off-by Cong Ding <[email protected]>

---
 drivers/clk/mvebu/clk-cpu.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Quoted 4 lines

Do you want to take this fix through the clock tree? If so,

Acked-by Jason Cooper <[email protected]>

I also think it should go through the clock tree but before this I'd like we fix the last issue.

Cong Ding,

you didn't take in account the case when the allocation of the 1st clocks when the 2nd cpu clock failed. In this case there is still a memory leak with the clock_name of the first cpu clock. See below for my proposal:

Quoted 26 lines

Otherwise, just let me know.

thx,

Jason.

Quoted 20 lines

drivers/clk/mvebu/clk-cpu.c +2 -2

index ff004578..1066a43 100644
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;

@@ -134,11 +134,13 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		if (WARN_ON(!clk_name))
-			return;
+			goto bail_out;
 

Quoted 6 lines

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node)

Quoted 6 lines

drivers/clk/mvebu/clk-cpu.c +2 -2

 		err = of_property_read_u32(dn, "reg", &cpu);
-		if (WARN_ON(err))
-			return;
+		if (WARN_ON(err)) {
 
+			kfree(clk_name);

we can free it later

Quoted 8 lines

Quoted 8 lines

drivers/clk/mvebu/clk-cpu.c +2 -0

+			goto bail_out;
+		}
 
 		sprintf(clk_name, "cpu%d", cpu);

drivers/clk/mvebu/clk-cpu.c +0 -0

 		parent_clk = of_clk_get(node, 0);

@@ -156,8 +158,10 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		init.num_parents = 1;
 

Quoted 5 lines

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 162

Quoted 5 lines

drivers/clk/mvebu/clk-cpu.c +2 -1

 		clk = clk_register(NULL, &cpuclk[cpu].hw);
-		if (WARN_ON(IS_ERR(clk)))
+		if (WARN_ON(IS_ERR(clk))) {
 
+			kfree(clk_name);

we can free it later

Quoted 10 lines

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 174

Quoted 10 lines

drivers/clk/mvebu/clk-cpu.c +1 -0

 			goto bail_out;
+		}
 		clks[cpu] = clk;
 	}

drivers/clk/mvebu/clk-cpu.c +1 -0

 	clk_data.clk_num = MAX_CPU;

@@ -167,6 +171,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 	return;
 bail_out:
 	kfree(clks);
+clks_out:

as cpuclk is allocated with all its member set to 0, and kfree(0) is a valid call. We can add the following lines:
while(ncpus--)

	kfree(cpuclk[ncpus].clk_name);

Quoted 2 lines

Quoted 2 lines

drivers/clk/mvebu/clk-cpu.c +0 -0

 	kfree(cpuclk);
 }

Quoted 96 lines · Gregory CLEMENT

Quoted 16 lines · Jason Cooper

Mike,

Quoted 11 lines · Cong Ding

From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
From: Cong Ding <[email protected]>
Date: Mon, 14 Jan 2013 18:06:26 +0100
Subject: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

the variable cpuclk and clk_name should be properly freed when error happens.

Signed-off-by Cong Ding <[email protected]>

---
 drivers/clk/mvebu/clk-cpu.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Quoted 4 lines

Do you want to take this fix through the clock tree? If so,

Acked-by Jason Cooper <[email protected]>

I also think it should go through the clock tree but before this I'd like we fix the last issue.

Cong Ding,

you didn't take in account the case when the allocation of the 1st clocks when the 2nd cpu clock failed. In this case there is still a memory leak with the clock_name of the first cpu clock. See below for my proposal:

Quoted 26 lines

Otherwise, just let me know.

thx,

Jason.

Quoted 20 lines

drivers/clk/mvebu/clk-cpu.c +2 -2

index ff004578..1066a43 100644
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;

@@ -134,11 +134,13 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		if (WARN_ON(!clk_name))
-			return;
+			goto bail_out;
 

Quoted 6 lines

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node)

Quoted 6 lines

drivers/clk/mvebu/clk-cpu.c +2 -2

 		err = of_property_read_u32(dn, "reg", &cpu);
-		if (WARN_ON(err))
-			return;
+		if (WARN_ON(err)) {
 
+			kfree(clk_name);

we can free it later

Quoted 8 lines

Quoted 8 lines

drivers/clk/mvebu/clk-cpu.c +2 -0

+			goto bail_out;
+		}
 
 		sprintf(clk_name, "cpu%d", cpu);

drivers/clk/mvebu/clk-cpu.c +0 -0

 		parent_clk = of_clk_get(node, 0);

@@ -156,8 +158,10 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		init.num_parents = 1;
 

Quoted 5 lines

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 162

Quoted 5 lines

drivers/clk/mvebu/clk-cpu.c +2 -1

 		clk = clk_register(NULL, &cpuclk[cpu].hw);
-		if (WARN_ON(IS_ERR(clk)))
+		if (WARN_ON(IS_ERR(clk))) {
 
+			kfree(clk_name);

we can free it later

Quoted 10 lines

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 174

Quoted 10 lines

drivers/clk/mvebu/clk-cpu.c +1 -0

 			goto bail_out;
+		}
 		clks[cpu] = clk;
 	}

drivers/clk/mvebu/clk-cpu.c +1 -0

 	clk_data.clk_num = MAX_CPU;

@@ -167,6 +171,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 	return;
 bail_out:
 	kfree(clks);
+clks_out:

as cpuclk is allocated with all its member set to 0, and kfree(0) is a valid call. We can add the following lines:
while(ncpus--)

	kfree(cpuclk[ncpus].clk_name);

Quoted 2 lines

Quoted 2 lines

drivers/clk/mvebu/clk-cpu.c void __init of_cpu_clk_setup(struct device_node *node) · line 176

Quoted 2 lines

drivers/clk/mvebu/clk-cpu.c +0 -0

 	kfree(cpuclk);
 }

I agree the version 2 patch still includes memory leakage in terms of clk_name, but I am wondering whether it is safe to call kfree(cpuclk[ncpus].clkname) directly or not. It's true that kfree(0) is valid, but cpuclk[ncpus].clkname might not be 0 when it is allocated by kzalloc. kzalloc just allocates the memory while doesn't ensure the initial value in this memory area is 0. So I

am thinking we should call memset after the alloction or use a counter to remember the number of clk_names allocated?

- cong

the variable cpuclk and clk_name should be properly freed when error happens.

Signed-off-by Cong Ding <[email protected]>

Acked-by Jason Cooper <[email protected]>

---
 drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/clk/mvebu/clk-cpu.c +6 -3

index ff004578..9dd2551 100644
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;

@@ -134,11 +134,11 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		if (WARN_ON(!clk_name))
-			return;
+			goto bail_out;
 
 		err = of_property_read_u32(dn, "reg", &cpu);
 		if (WARN_ON(err))
-			return;
+			goto bail_out;
 
 		sprintf(clk_name, "cpu%d", cpu);
 		parent_clk = of_clk_get(node, 0);

@@ -167,6 +167,9 @@ void __init of_cpu_clk_setup(struct device_node *node)
 	return;
 bail_out:
 	kfree(clks);
+	while(ncpus--)
+		kfree(cpuclk[ncpus].clk_name);
+clks_out:
 	kfree(cpuclk);
 }
 

If you prefer, Jason agrees to take it, but you probably didn't notice it, because he uses your former(and no more valid) email when he wrote this.

Quoted 1 line

the variable cpuclk and clk_name should be properly freed when error happens.

Dear Cong Ding,

Thanks for you efforts!
I am happy with this patch and I tested it on the Armada XP DB board, so you can now add my:

Acked-by Gregory CLEMENT <[email protected]>

Mike,

could you take this patch for 3.8-rc fixes?

If you prefer, Jason agrees to take it, but you probably didn't notice it, because he uses your former(and no more valid) email when he wrote this.

Thanks,

Gregory

Quoted 45 lines

Signed-off-by Cong Ding <[email protected]>

Acked-by Jason Cooper <[email protected]>

---
 drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/clk/mvebu/clk-cpu.c +6 -3

index ff004578..9dd2551 100644
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
      clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
      if (WARN_ON(!clks))
-             return;
+             goto clks_out;
 
      for_each_node_by_type(dn, "cpu") {
              struct clk_init_data init;

@@ -134,11 +134,11 @@ void __init of_cpu_clk_setup(struct device_node *node)
              int cpu, err;
 
              if (WARN_ON(!clk_name))
-                     return;
+                     goto bail_out;
 
              err = of_property_read_u32(dn, "reg", &cpu);
              if (WARN_ON(err))
-                     return;
+                     goto bail_out;
 
              sprintf(clk_name, "cpu%d", cpu);
              parent_clk = of_clk_get(node, 0);

@@ -167,6 +167,9 @@ void __init of_cpu_clk_setup(struct device_node *node)
      return;
 bail_out:
      kfree(clks);
+     while(ncpus--)
+             kfree(cpuclk[ncpus].clk_name);
+clks_out:
      kfree(cpuclk);
 }
 

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Quoting Gregory CLEMENT (2013-01-15 12:46:03)

Quoted 8 lines · Cong Ding

Quoted 1 line

the variable cpuclk and clk_name should be properly freed when error happens.

Dear Cong Ding,

Thanks for you efforts!
I am happy with this patch and I tested it on the Armada XP DB board, so you can now add my:

Quoted 9 lines

Acked-by Gregory CLEMENT <[email protected]>

Mike,

could you take this patch for 3.8-rc fixes?

If you prefer, Jason agrees to take it, but you probably didn't notice it, because he uses your former(and no more valid) email when he wrote this.

Acked-by Mike Turquette <[email protected]>

I don't have an existing clk-fixes branch. This patch is the first fix I've reviewed for this cycle. I'm happy if you want to take it and submit as part of any other mvebu fixes you have. Otherwise I can take it.

the variable cpuclk and clk_name should be properly freed when error happens.

Signed-off-by Cong Ding <[email protected]>

Acked-by Jason Cooper <[email protected]>

---
 drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)