[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Thu, May 02, 2013 at 07:40:58PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Thu, May 02, 2013 at 08:28:44PM +0400, Alexander Shiyan wrote:
> > > On 20:03-20130502, Alexander Shiyan wrote:
> > > > 
> > > > Signed-off-by: Alexander Shiyan <shc_work at mail.ru>
> > > > ---
> > > >  arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++----
> > > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
> > > > index aa27d7f..8d17a0d 100644
> > > > --- a/arch/arm/mach-omap2/usb-host.c
> > > > +++ b/arch/arm/mach-omap2/usb-host.c
> > > > @@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > >  	supplies->dev_name = dev_id;
> > > >  
> > > >  	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
> > > > -	if (!reg_data)
> > > > +	if (!reg_data) {
> > > > +		kfree(supplies);
> > > >  		return -ENOMEM;
> > > > +	}
> > > >  
> > > >  	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> > > >  	reg_data->consumer_supplies = supplies;
> > > > @@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > >  
> > > >  	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
> > > >  			GFP_KERNEL);
> > > > -	if (!config)
> > > > +	if (!config) {
> > > > +		kfree(supplies);
> > > > +		kfree(reg_data);
> > > >  		return -ENOMEM;
> > > > +	}
> > > >  
> > > >  	config->supply_name = name;
> > > >  	config->gpio = gpio;
> > > > @@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > >  
> > > >  	/* create a regulator device */
> > > >  	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> > > > -	if (!pdev)
> > > > +	if (!pdev) {
> > > > +		kfree(supplies);
> > > > +		kfree(reg_data);
> > > > +		kfree(config);
> > > >  		return -ENOMEM;
> > > > +	}
> > > >  
> > > >  	pdev->id = PLATFORM_DEVID_AUTO;
> > > >  	pdev->name = reg_name;
> > > >  	pdev->dev.platform_data = config;
> > > >  
> > > >  	ret = platform_device_register(pdev);
> > > > -	if (ret)
> > > > +	if (ret) {
> > > >  		pr_err("%s: Failed registering regulator %s for %s\n",
> > > >  				__func__, name, dev_id);
> > > > +		kfree(supplies);
> > > > +		kfree(reg_data);
> > > > +		kfree(config);
> > > > +	}
> > > 
> > > Might be better to switch to devm_XXX managed functions?
> 
> I don't think it makes sense since the platform_device hasn't been
> registered yet.
> 
> Still, patch can be improved with proper goto labels instead of
> sprinkling different kfree() calls in every single error branch.
> 
> > If anyone can rewrite driver to use devm_xx, it would have been better.
> > I'm not going to redo the patch yet, let it be so, I just showed a point
> > for OMAP-developers.
> 
> fair enough.

Well, as long as this crap violates the driver model by using kfree() on
a device...  Devices are refcounted and must only be freed when the
refcount drops to zero.

This is exactly why we have platform_device_alloc(),
platform_device_register_full() and friends - so that people don't have to
fsck around with kzalloc themselves and get it wrong like the above does.

Would you like me to pass your details to gregkh for another one of his
public humilation exercises over basic kernel programming stuff? :)