Re: [patch] drm/tegra: small leak on error in tegra_fb_alloc()

[Thierry Reding <thierry.reding@xxxxxxxxx>]

On Sat, Nov 09, 2013 at 09:34:06PM -0800, Dan Carpenter wrote:
> If we don't have enough memory for ->planes then we leak "fb".
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>

Hi Dan,

Thanks for catching this. Perhaps tweak the commit subject to:

	drm/tegra: fix small leak on error in tegra_fb_alloc()

?

One additional comment below.

> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
> index 490f771..1362d78 100644
> --- a/drivers/gpu/drm/tegra/fb.c
> +++ b/drivers/gpu/drm/tegra/fb.c
> @@ -98,8 +98,10 @@ static struct tegra_fb *tegra_fb_alloc(struct drm_device *drm,
>  		return ERR_PTR(-ENOMEM);
>  
>  	fb->planes = kzalloc(num_planes * sizeof(*planes), GFP_KERNEL);
> -	if (!fb->planes)
> -		return ERR_PTR(-ENOMEM);
> +	if (!fb->planes) {
> +		err = -ENOMEM;
> +		goto free_fb;
> +	}
>  
>  	fb->num_planes = num_planes;
>  
> @@ -112,12 +114,17 @@ static struct tegra_fb *tegra_fb_alloc(struct drm_device *drm,
>  	if (err < 0) {
>  		dev_err(drm->dev, "failed to initialize framebuffer: %d\n",
>  			err);
> -		kfree(fb->planes);
> -		kfree(fb);
> -		return ERR_PTR(err);
> +		goto free_planes;
>  	}
>  
>  	return fb;
> +
> +free_planes:
> +	kfree(fb->planes);
> +free_fb:
> +	kfree(fb);
> +
> +	return ERR_PTR(err);
>  }

I think in this case I'd actually prefer for the kfree(fb) to be
duplicated and not have this error unwind. It's actually shorter
to do it that way in this case.

What I mean is:

	if (!fb->planes) {
		kfree(fb);
		return ERR_PTR(-ENOMEM);
	}

Thierry

Attachment: pgpCWWhARtlwr.pgp
Description: PGP signature