Date: Thu, 21 Sep 2017 14:54:42 +0100
From: Rui Miguel Silva <rmfrfs@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Arvind Yadav <arvind.yadav.cs@gmail.com>, johan@kernel.org,
        elder@kernel.org, gregkh@linuxfoundation.org,
        devel@driverdev.osuosl.org, greybus-dev@lists.linaro.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: staging: greybus: Release memory obtained by
 kasprintf
Message-ID: <20170921135442.GB21164@arch-late.localdomain>
References: <21ea1d2e0ef0c3d4d59b2e5d2e189533c8b1668a.1505991013.git.arvind.yadav.cs@gmail.com>
 <20170921125918.keiog5yd2jw342zc@mwanda>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170921125918.keiog5yd2jw342zc@mwanda>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2515
Lines: 75

Hi,
On Thu, Sep 21, 2017 at 03:59:18PM +0300, Dan Carpenter wrote:
> On Thu, Sep 21, 2017 at 05:05:27PM +0530, Arvind Yadav wrote:
> > Free memory region, if gb_lights_channel_config is not successful.

Arvind, thanks for patch and good catch.
But please look at the subject of other patches applied to this
file and try to stick with the labels, staging: greybus: light:

> > 
> 
> The question I have is do we free this on module unload?  I don't see
> that we do.  I feel like we should do a free after calling
> __gb_lights_led_unregister().  But that's awkward because we call
> __gb_lights_led_unregister() when this function fails so it would end
> up being a double free.

Yes Dan, You are correct, this should be free in
__gb_lights_led_unregister(), and not here.

> 
> > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> > ---
> >  drivers/staging/greybus/light.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> > index 3f4148c..b00d47c 100644
> > --- a/drivers/staging/greybus/light.c
> > +++ b/drivers/staging/greybus/light.c
> > @@ -984,7 +984,7 @@ static int gb_lights_channel_config(struct gb_light *light,
> >  
> >  	ret = channel_attr_groups_set(channel, cdev);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto err;
> >  
> >  	gb_lights_led_operations_set(channel, cdev);
> >  
> > @@ -994,15 +994,18 @@ static int gb_lights_channel_config(struct gb_light *light,
> >  	 * configurations.
> >  	 */
> >  	if (!is_channel_flash(channel))
> > -		return ret;
> > +		goto err;
> 
> "ret" is zero here.  This is actually a success return.  It would be
> cleaner to just write "return 0;".  Anyway, this patch introduces a use
> after free so that doesn't work.
> 
> Also it's better to choose a label name which says what the label does
> so in this case it would be "goto err_free_name" or "goto err_cdev_name"
> or whatever, but something to indicate that it's to do with freeing
> the cdev->name.  Just "err" is too ambiguous.
> 
> >  
> >  	light->has_flash = true;
> >  
> >  	ret = gb_lights_channel_flash_config(channel);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto err;
> >  
> >  	return ret;
>         ^^^^^^^^^^
> Here as well, change this from "return ret;" to "return 0;".

It should be return 0; from the start, you are right, but that
would be a complete different change than the actual fix that now
goes far away from this code.

Thank both,
---
Cheers,
	Rui