| Message ID | 20191220000923.9924-1-navid.emamdoost@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clocksource/drivers: Fix memory leaks in ttc_setup_clockevent and ttc_setup_clocksource | expand |
On Thu, Dec 19, 2019 at 06:09:21PM -0600, Navid Emamdoost wrote: > In the implementation of ttc_setup_clockevent() and > ttc_setup_clocksource(), the allocated memory for ttccs is leaked when > clk_notifier_register() fails. Use goto to direct the execution into error > handling path. No, that memory was never leaked since that function did not return on registration errors before your patch. > Fixes: 70504f311d4b ("clocksource/drivers/cadence_ttc: Convert init function to return error") Perhaps you meant to fix the actual leak that was added by this commit in a different function, ttc_setup_clockevent(), when returning on notifier-registration errors? Also should the clock be left enabled on errors? > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > --- > drivers/clocksource/timer-cadence-ttc.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c > index c6d11a1cb238..46d69982ad33 100644 > --- a/drivers/clocksource/timer-cadence-ttc.c > +++ b/drivers/clocksource/timer-cadence-ttc.c > @@ -328,10 +328,8 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base, > ttccs->ttc.clk = clk; > > err = clk_prepare_enable(ttccs->ttc.clk); > - if (err) { > - kfree(ttccs); > - return err; > - } > + if (err) > + goto release_ttcce; > > ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk); > > @@ -341,8 +339,10 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base, > > err = clk_notifier_register(ttccs->ttc.clk, > &ttccs->ttc.clk_rate_change_nb); > - if (err) > + if (err) { > pr_warn("Unable to register clock notifier.\n"); > + goto release_ttcce; > + } Johan
On 20. 12. 19 6:10, Johan Hovold wrote: > On Thu, Dec 19, 2019 at 06:09:21PM -0600, Navid Emamdoost wrote: >> In the implementation of ttc_setup_clockevent() and >> ttc_setup_clocksource(), the allocated memory for ttccs is leaked when >> clk_notifier_register() fails. Use goto to direct the execution into error >> handling path. > > No, that memory was never leaked since that function did not return on > registration errors before your patch. > >> Fixes: 70504f311d4b ("clocksource/drivers/cadence_ttc: Convert init function to return error") > > Perhaps you meant to fix the actual leak that was added by this commit > in a different function, ttc_setup_clockevent(), when returning on > notifier-registration errors? > > Also should the clock be left enabled on errors? > >> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> >> --- >> drivers/clocksource/timer-cadence-ttc.c | 22 +++++++++++++--------- >> 1 file changed, 13 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c >> index c6d11a1cb238..46d69982ad33 100644 >> --- a/drivers/clocksource/timer-cadence-ttc.c >> +++ b/drivers/clocksource/timer-cadence-ttc.c >> @@ -328,10 +328,8 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base, >> ttccs->ttc.clk = clk; >> >> err = clk_prepare_enable(ttccs->ttc.clk); >> - if (err) { >> - kfree(ttccs); >> - return err; >> - } >> + if (err) >> + goto release_ttcce; >> >> ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk); >> >> @@ -341,8 +339,10 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base, >> >> err = clk_notifier_register(ttccs->ttc.clk, >> &ttccs->ttc.clk_rate_change_nb); >> - if (err) >> + if (err) { >> pr_warn("Unable to register clock notifier.\n"); >> + goto release_ttcce; >> + } > + this is IMHO v3 version. It means just label it properly and also keep changes log. Thanks, Michal
> In the implementation of ttc_setup_clockevent() and > ttc_setup_clocksource(), the allocated memory for ttccs is leaked when > clk_notifier_register() fails. I suggest to correct your commit message. > Use goto to direct the execution into error handling path. * Should the desired completion of the exception handling be described by an other wording? * Will it be better to improve the affected functions by separate update steps? * How do you think about to add the tag “Reported-by” for Michal Simek? https://lore.kernel.org/linux-arm-kernel/2a6cdb63-397b-280a-7379-740e8f43ddf6@xilinx.com/ … > +++ b/drivers/clocksource/timer-cadence-ttc.c … > @@ -363,16 +363,20 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base, … > return 0; > + > +release_ttcce: > + > + kfree(ttcce); > + return err; … * Please omit a blank line after the label. * Is there a need to call the function “clk_disable_unprepare” by another jump target? Regards, Markus
diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c index c6d11a1cb238..46d69982ad33 100644 --- a/drivers/clocksource/timer-cadence-ttc.c +++ b/drivers/clocksource/timer-cadence-ttc.c @@ -328,10 +328,8 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base, ttccs->ttc.clk = clk; err = clk_prepare_enable(ttccs->ttc.clk); - if (err) { - kfree(ttccs); - return err; - } + if (err) + goto release_ttcce; ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk); @@ -341,8 +339,10 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base, err = clk_notifier_register(ttccs->ttc.clk, &ttccs->ttc.clk_rate_change_nb); - if (err) + if (err) { pr_warn("Unable to register clock notifier.\n"); + goto release_ttcce; + } ttccs->ttc.base_addr = base; ttccs->cs.name = "ttc_clocksource"; @@ -363,16 +363,20 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base, ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET); err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE); - if (err) { - kfree(ttccs); - return err; - } + if (err) + goto release_ttcce; ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET; sched_clock_register(ttc_sched_clock_read, timer_width, ttccs->ttc.freq / PRESCALE); return 0; + +release_ttcce: + + kfree(ttcce); + return err; + } static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
In the implementation of ttc_setup_clockevent() and ttc_setup_clocksource(), the allocated memory for ttccs is leaked when clk_notifier_register() fails. Use goto to direct the execution into error handling path. Fixes: 70504f311d4b ("clocksource/drivers/cadence_ttc: Convert init function to return error") Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- drivers/clocksource/timer-cadence-ttc.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)