Re: [PATCH -next] scsi: ufs: don't kfree() devm_kzalloc()'ed memory

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Thu, 2014-11-27 at 01:11 +0900, Akinobu Mita wrote:
> 2014-11-27 0:51 GMT+09:00 James Bottomley
> <James.Bottomley@hansenpartnership.com>:
> > On Thu, 2014-11-27 at 00:37 +0900, Akinobu Mita wrote:
> >> In ufshcd_parse_clock_info(), temporary memory allocated by
> >> devm_kzalloc() is released by kfree().  The devres mechanism cannot
> >> track the release and it causes trouble when removing the driver.
> >>
> >> Fix it by allocating it with plain kcalloc() instead of devm_kzalloc()
> >> as the temporary memory is always freed in that function.
> >>
> >> Signed-off-by: Akinobu Mita <mita@fixstars.com>
> >> Cc: Vinayak Holikatti <vinholikatti@gmail.com>
> >> Cc: Dolev Raviv <draviv@codeaurora.org>
> >> Cc: Subhash Jadavani <subhashj@codeaurora.org>
> >> Cc: Yaniv Gardi <ygardi@codeaurora.org>
> >> Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
> >> Cc: Maya Erez <merez@codeaurora.org>
> >> Cc: Sahitya Tummala <stummala@codeaurora.org>
> >> Cc: Christoph Hellwig <hch@lst.de>
> >> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> >> Cc: linux-scsi@vger.kernel.org
> >> ---
> >>  drivers/scsi/ufs/ufshcd-pltfrm.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> >> index 0c030ad..67b1f6c 100644
> >> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> >> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> >> @@ -99,8 +99,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
> >>               goto out;
> >>       }
> >>
> >> -     clkfreq = devm_kzalloc(dev, sz * sizeof(*clkfreq),
> >> -                     GFP_KERNEL);
> >> +     clkfreq = kcalloc(sz, sizeof(*clkfreq), GFP_KERNEL);
> >
> > I agree this doesn't need to be devm memory since it's copied into clki,
> > but where is it freed?  I think you need a kfree(clkfreq) at the bottom
> > of the routine.
> 
> Oops.  I was working on the older code, and blindly applied the -next just
> before sending this patch.  The problem I said in this commit log has been
> fixed by the commit e8cb64db81e8c88c5c824ca74c2e57b4c6919ca6 ("scsi: ufs:
> fix static checker warning in ufshcd_parse_clock_info").
> 
> So I withdraw this patch for now.  I will try with another title next time.

I don't necessarily disagree with the patch if you correct it.  In the
current state, clkfreq is only used as a temporary in the routine as you
say, but it persists the entire lifetime of the driver ... it's a tiny
amount of memory and the devm_ allocation ensures it eventually gets
cleaned up, so it's probably not that important in the grand scheme of
things.

James