Re: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

[Gregory CLEMENT <gregory.clement@free-electrons.com>]

On 01/15/2013 04:37 PM, Jason Cooper wrote:
> Mike,
> 
> On Tue, Jan 15, 2013 at 03:23:08PM +0000, Cong Ding wrote:
>> From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
>> From: Cong Ding <dinggnu@gmail.com>
>> 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 <dinggnu@gmail.com>
>> ---
>>  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 <jason@lakedaemon.net>
> 

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:

> Otherwise, just let me know.
> 
> thx,
> 
> Jason.
> 
>> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
>> index ff004578..1066a43 100644
>> --- a/drivers/clk/mvebu/clk-cpu.c
>> +++ b/drivers/clk/mvebu/clk-cpu.c
>> @@ -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);
we can free it later

>> +			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);
we can free it later

>>  			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:

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

>>  	kfree(cpuclk);
>>  }
>>  
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


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