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

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

[Colin King]

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;
 	}
 	onecell->hws[BOSTON_CLK_INPUT] = hw;
 
 	hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
 	if (IS_ERR(hw)) {
 		pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
-		return;
+		goto error;
 	}
 	onecell->hws[BOSTON_CLK_SYS] = hw;
 
 	hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
 	if (IS_ERR(hw)) {
 		pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
-		return;
+		goto error;
 	}
 	onecell->hws[BOSTON_CLK_CPU] = hw;
 
 	err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
-	if (err)
+	if (err) {
 		pr_err("failed to add DT provider: %d\n", err);
+		goto error;
+	}
+	return;
+
+error:
+	kfree(onecell);
+	return;
 }
 
 /*
-- 
2.17.0

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

[Colin King]

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;
 	}
 	onecell->hws[BOSTON_CLK_INPUT] = hw;
 
 	hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
 	if (IS_ERR(hw)) {
 		pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
-		return;
+		goto error;
 	}
 	onecell->hws[BOSTON_CLK_SYS] = hw;
 
 	hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
 	if (IS_ERR(hw)) {
 		pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
-		return;
+		goto error;
 	}
 	onecell->hws[BOSTON_CLK_CPU] = hw;
 
 	err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
-	if (err)
+	if (err) {
 		pr_err("failed to add DT provider: %d\n", err);
+		goto error;
+	}
+	return;
+
+error:
+	kfree(onecell);
+	return;
 }
 
 /*
-- 
2.17.0

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

[Dan Carpenter]

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.  Can this even boot without the clk?  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

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

[Dan Carpenter]

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.  Can this even boot without the clk?  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

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

[Paul Burton]

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?

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

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

[Paul Burton]

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?

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

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

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

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

[Dan Carpenter]

It would be nice to make things static check clean.  One idea would be
that the static checker could ignore resource leaks in __init functions.

regards,
dan carpenter

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

[Dan Carpenter]

It would be nice to make things static check clean.  One idea would be
that the static checker could ignore resource leaks in __init functions.

regards,
dan carpenter

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

[Stephen Boyd]

Quoting Dan Carpenter (2018-05-09 23:59:51)
> It would be nice to make things static check clean.  One idea would be
> that the static checker could ignore resource leaks in __init functions.
> 

Typically if the stuff is so important that it doesn't work without it
then we throw in a panic() or a BUG() call to indicate that all hope is
lost. Otherwise, I'm not sure what's wrong with adding in proper error
paths for clean recovery.

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

[Stephen Boyd]

Quoting Dan Carpenter (2018-05-09 23:59:51)
> It would be nice to make things static check clean.  One idea would be
> that the static checker could ignore resource leaks in __init functions.
> 

Typically if the stuff is so important that it doesn't work without it
then we throw in a panic() or a BUG() call to indicate that all hope is
lost. Otherwise, I'm not sure what's wrong with adding in proper error
paths for clean recovery.

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

[Stephen Boyd]

Quoting Dan Carpenter (2018-05-09 23:59:51)
> It would be nice to make things static check clean.  One idea would be
> that the static checker could ignore resource leaks in __init functions.
> =


Typically if the stuff is so important that it doesn't work without it
then we throw in a panic() or a BUG() call to indicate that all hope is
lost. Otherwise, I'm not sure what's wrong with adding in proper error
paths for clean recovery.

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

[Dan Carpenter]

On Tue, May 15, 2018 at 11:28:40AM -0700, Stephen Boyd wrote:
> Quoting Dan Carpenter (2018-05-09 23:59:51)
> > It would be nice to make things static check clean.  One idea would be
> > that the static checker could ignore resource leaks in __init functions.
> > 
> 
> Typically if the stuff is so important that it doesn't work without it
> then we throw in a panic() or a BUG() call to indicate that all hope is
> lost. Otherwise, I'm not sure what's wrong with adding in proper error
> paths for clean recovery.

In clk_boston_setup() then we'd have to put a ton of BUG()s in there to
silence all the warnings.  Right now the static checkers only care about
kmalloc() but in a year or two they'll be clever enough to care about
everything leaked in this function.  I don't think adding BUG() calls
is a good idea.

Plus, I have a private static checker warning for that.  When the BTRFS
filesystem was merged 10 years ago it used to call BUG() all the time if
allocations failed so I made a static checker warning to spot that
anti-pattern...

regards,
dan carpenter

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

[Dan Carpenter]

On Tue, May 15, 2018 at 11:28:40AM -0700, Stephen Boyd wrote:
> Quoting Dan Carpenter (2018-05-09 23:59:51)
> > It would be nice to make things static check clean.  One idea would be
> > that the static checker could ignore resource leaks in __init functions.
> > 
> 
> Typically if the stuff is so important that it doesn't work without it
> then we throw in a panic() or a BUG() call to indicate that all hope is
> lost. Otherwise, I'm not sure what's wrong with adding in proper error
> paths for clean recovery.

In clk_boston_setup() then we'd have to put a ton of BUG()s in there to
silence all the warnings.  Right now the static checkers only care about
kmalloc() but in a year or two they'll be clever enough to care about
everything leaked in this function.  I don't think adding BUG() calls
is a good idea.

Plus, I have a private static checker warning for that.  When the BTRFS
filesystem was merged 10 years ago it used to call BUG() all the time if
allocations failed so I made a static checker warning to spot that
anti-pattern...

regards,
dan carpenter