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

Simon Baatz
2017-02-06T23:40:06Z

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