On Tue, 6 May 2014, Gregory CLEMENT wrote:
> In order to disable the clock while the module was removing, a call to
> devm_clk_get() was made. This was wrong and lead to a leak module
> ref-counts.
>
> In order to have a reference of the clock get, a private structure was
> added using the override mechanism provided by the ehci framework.
>
> The bug was introduced in v3.6, however the ehci framework allowing to
> use the override mechanism have only been introduced in v3.8, so this
> patch won't apply before it.
>
> Fixes: 8c869edaee07c623066266827371235fb9c12e01 ('ARM: Orion: EHCI: Add
> support for enabling clocks')
> Cc: <sta...@vger.kernel.org> # v3.8+
> Signed-off-by: Gregory CLEMENT <gregory.clem...@free-electrons.com>
> hcd = usb_create_hcd(&ehci_orion_hc_driver,
> &pdev->dev, dev_name(&pdev->dev));
> if (!hcd) {
> err = -ENOMEM;
> - goto err2;
> + goto err1;
> }
>
> hcd->rsrc_start = res->start;
> @@ -211,6 +215,15 @@ static int ehci_orion_drv_probe(struct platform_device
> *pdev)
> ehci->caps = hcd->regs + 0x100;
> hcd->has_tt = 1;
>
> + priv = hcd_to_orion_priv(hcd);
> + /*
> + * Not all platforms can gate the clock, so it is not an error if
> + * the clock does not exists.
> + */
> + priv->clk = devm_clk_get(&pdev->dev, NULL);
> + if (!IS_ERR(priv->clk))
> + clk_prepare_enable(priv->clk);
> +
> /*
> * (Re-)program MBUS remapping windows if we are asked to.
> */
> @@ -240,16 +253,16 @@ static int ehci_orion_drv_probe(struct platform_device
> *pdev)
>
> err = usb_add_hcd(hcd, irq, IRQF_SHARED);
> if (err)
> - goto err3;
> + goto err2;
>
> device_wakeup_enable(hcd->self.controller);
> return 0;
>
> -err3:
> - usb_put_hcd(hcd);
> err2:
> - if (!IS_ERR(clk))
> - clk_disable_unprepare(clk);
> + usb_put_hcd(hcd);
At this point, priv has just become a dangling pointer, because it
points to something that was allocated along with hcd.
> +
> + if (!IS_ERR(priv->clk))
> + clk_disable_unprepare(priv->clk);
And now you are dereferencing memory that has been deallocated. The
real problem is that you get and enable the clock _after_ creating hcd,
but you don't disable it _before_ deallocating hcd.
> err1:
> dev_err(&pdev->dev, "init %s fail, %d\n",
> dev_name(&pdev->dev), err);
> @@ -260,14 +273,14 @@ err1:
> static int ehci_orion_drv_remove(struct platform_device *pdev)
> {
> struct usb_hcd *hcd = platform_get_drvdata(pdev);
> - struct clk *clk;
> + struct orion_ehci_hcd *priv = hcd_to_orion_priv(hcd);
>
> usb_remove_hcd(hcd);
> usb_put_hcd(hcd);
>
> - clk = devm_clk_get(&pdev->dev, NULL);
> - if (!IS_ERR(clk))
> - clk_disable_unprepare(clk);
> + if (!IS_ERR(priv->clk))
> + clk_disable_unprepare(priv->clk);
This has the same problem as above.
Also, for both this patch and 02/20, it would be better to replace the
"errN" labels with something more descriptive, so that it's not
necessary to renumber them every time something changes.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
