lkml.org 
[lkml]   [2018]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] clk: boston: fix memory leak of 'onecell' on error return paths
From
Date
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@canonical.com>
>>>
>>> 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@canonical.com>
>>> ---
>>> 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

\
 
 \ /
  Last update: 2018-05-09 18:45    [W:0.078 / U:0.580 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site