LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: wang.yi59@zte.com.cn
Cc: paul.burton@mips.com, mturquette@baylibre.com,
	linux-mips@linux-mips.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, zhong.weidong@zte.com.cn,
	up2wing@163.com
Subject: Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
Date: Tue, 30 Oct 2018 09:25:38 -0700	[thread overview]
Message-ID: <154091673873.98144.2128870769302542417@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <201810301413240217200@zte.com.cn>

Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > Quoting Yi Wang (2018-10-29 01:31:47)
> > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > before leaving from the error handling cases.
> >
> > How did you find this? Visual inspection? Some coccinelle script?
> 
> Smatch report this:
> drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'

Ok. Please include those details in the commit text.

> 
> >
> > >
> > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > ---
> > > v2: fix syntax issue in comment, thanks to  Sergei.
> > >
> > >  drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > index 15af423..f5d54a6 100644
> > > --- a/drivers/clk/imgtec/clk-boston.c
> > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > @@ -73,27 +73,32 @@ 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)
> > >                 pr_err("failed to add DT provider: %d\n", err);
> > > +
> > > +       return;
> > > +
> > > +error:
> > > +       kfree(onecell);
> >
> > Ok, sure. But then clks are still left registered on failure?
> 
> Yeah, but this patch does not change the original flow of the function, so I suppose
> if you deem this is not proper, it's better to improve that in another patch, what do
> you think?
> 

I think we should attempt to fix all the theoretical problems with the
driver instead of just fixing some things to make static checkers happy.
It looks like this driver was written with the assumption that if things
go bad we give up all hope. It would be better to clean everything up
properly if things go bad and have better code.


           reply	other threads:[~2018-10-30 16:25 UTC|newest]

Thread overview: expand[flat|nested]  mbox.gz  Atom feed
 [parent not found: <201810301413240217200@zte.com.cn>]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=154091673873.98144.2128870769302542417@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@linux-mips.org \
    --cc=mturquette@baylibre.com \
    --cc=paul.burton@mips.com \
    --cc=up2wing@163.com \
    --cc=wang.yi59@zte.com.cn \
    --cc=zhong.weidong@zte.com.cn \
    --subject='Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).