| Message ID | 20180509134031.11611-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } /*