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

[Felipe Balbi]

Hi,

On Fri, May 03, 2013 at 12:13:53AM +0100, Russell King - ARM Linux 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.

read the patch again, there's no kfree() on any device. There is a kfree
of supplies, reg_data and config.

On top of that, the code being changed here doesn't even exist, so I
wonder which tree is this code based off. usb-host.c has always being
using omap_device_build() which internally calls
platform_device_alloc().

> 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? :)

How about we pass yours for not reading the patch before flaming ? Note
that $SUBJECT is *not* touching at all that line which kzallocs a
platform_device. Wrong as it is, it's not part of $SUBJECT.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130503/b244bb23/attachment.sig>