Re: [PATCH v3 01/20] usb: ehci-orion: Fix clock reference leaking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dear Alan Stern,

On Tue, 6 May 2014 10:30:57 -0400 (EDT), Alan Stern wrote:

> >  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.

Right.

> > +
> > +	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.

Correct.

> 
> >  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.

Indeed. Will fix in v4.

> 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.

Sure. I've added an additional commit prior to this patch that renames
the goto labels to something more meaningful. This will be part of v4.

Thanks for the review!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux