Re: [PATCH] clk: boston: fix memory leak of 'onecell' on error return paths

[Colin Ian King]

On 09/05/18 17:33, Paul Burton wrote:
> Hi Colin & Dan,
> 
> On Wed, May 09, 2018 at 05:01:35PM +0300, Dan Carpenter wrote:
>> On Wed, May 09, 2018 at 02:40:31PM +0100, Colin King wrote:
>>> From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
>>>
>>> There are several error return paths that don't free up onecell
>>> and hence we have some memory leaks. Add an error exit path that
>>> kfree's onecell to fix the leaks.
>>>
>>> Signed-off-by: Colin Ian King <colin.king@xxxxxxxxxxxxx>
>>> ---
>>>  drivers/clk/imgtec/clk-boston.c | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
>>> index 15af423cc0c9..d6bc468ff551 100644
>>> --- a/drivers/clk/imgtec/clk-boston.c
>>> +++ b/drivers/clk/imgtec/clk-boston.c
>>> @@ -73,27 +73,34 @@ static void __init clk_boston_setup(struct device_node *np)
>>>  	hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
>>>  	if (IS_ERR(hw)) {
>>>  		pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
>>> -		return;
>>> +		goto error;
>>
>> I hate vague label names like "error" and "out"...
>>
>> There are a bunch of other resources that we should free if we decide
>> it's worth freeing things.
> 
> Agreed - for example unregistering the clocks that we'd be discarding
> references to by freeing onecell.
> 
>> Can this even boot without the clk?
> 
> Nope. If this clock setup fails then whether you free this memory or not
> you're going to be unable to do anything useful with it.
> 
> I imagine this patch is the result of some static analysis rather than a
> problem being observed at runtime?

Indeed. I propose ignoring this then as it's fairly terminal if one
can't get memory anyhow in the early boot.

Colin
> 
> Thanks,
>     Paul
> 
>> When
>> the label names says what is freed, then you mentally only have to keep
>> track of the most recently allocated resource.  So if
>>
>> 	hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
>>
>> succeeds then the next goto is going to "goto free_clk_input;".
>>
>> regards,
>> dan carpenter