Re: [bug report] ARM: Orion: fix driver probe error handling with respect to clk

[Simon Baatz <gmbnomis@gmail.com>]

Hi Dan

On Mon, Feb 06, 2017 at 06:29:47PM +0300, Dan Carpenter wrote:
> The patch baffab28b131: "ARM: Orion: fix driver probe error handling
> with respect to clk" from Jul 19, 2012, leads to the following static
> checker warning:
> 
> 	drivers/mtd/nand/orion_nand.c:172 orion_nand_probe()
> 	warn: 'clk' was already freed.
> 
> drivers/mtd/nand/orion_nand.c
>    150          /* Not all platforms can gate the clock, so it is not
>    151             an error if the clock does not exists. */
>    152          clk = clk_get(&pdev->dev, NULL);
>    153          if (!IS_ERR(clk)) {
>    154                  clk_prepare_enable(clk);
>    155                  clk_put(clk);
> 
> Huh?  Apparently clk_get() and clk_put() are not ref counted
> opperations?
> 
> You would think they would be from the name.  What it looks like to me
> is that clk_put() should be renamed clk_free().  The comments on
> clk_put() are not totally clear on this.  I'm just joking.  :P  There
> aren't any comments...

This looks fishy indeed (btw.  that's not my code).  Instead of
holding a clock pointer, the driver seems to use clk_get()/clk_put()
every time it "needs" a clock (see also orion_nand_remove()).  This
results in calling clk_put() on an enabled clock here and not holding
a reference to the clock after the probe.

Looks somewhat similar to the situation fixed in ac0696629d73 ('usb:
ehci-orion: fix clock reference leaking') to me.
 
>    156          }
>    157  
>    158          ret = nand_scan(mtd, 1);
>    159          if (ret)
>    160                  goto no_dev;
>    161  
>    162          mtd->name = "orion_nand";
>    163          ret = mtd_device_register(mtd, board->parts, board->nr_parts);
>    164          if (ret) {
>    165                  nand_release(mtd);
>    166                  goto no_dev;
>    167          }
>    168  
>    169          return 0;
>    170  
>    171  no_dev:
>    172          if (!IS_ERR(clk)) {
>    173                  clk_disable_unprepare(clk);
> 
> Any later reference to "clk" after clk_put() is a use after free.


Yes, sure. But the clk_put() above should not be there in the first
place. When I added this code, I probably should have had a closer
look at the clock handling above. (Or, more realistically, I just did
not understand enough at that time...)

> 
>    174                  clk_put(clk);
>    175          }
>    176  
>    177          return ret;
>    178  }


- Simon