diff mbox

[v5,2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed

Message ID 521e48fc5fc8d211ed2847070120ff4032b7a383.1441363361.git.lukas@wunner.de (mailing list archive)
State New, archived
Headers show

Commit Message

Lukas Wunner July 4, 2015, 9:50 a.m. UTC
Currently when allocating a framebuffer fails, the gem object gets
unrefed at the bottom of the call chain in __intel_framebuffer_create,
not where it gets refed, which is in intel_framebuffer_create_for_mode
(via i915_gem_alloc_object) and in intel_user_framebuffer_create
(via drm_gem_object_lookup).

This invites mistakes: As discovered by Tvrtko Ursulin, a double unref
has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).

As suggested by Ville Syrjälä, improve code clarity by moving the unref
away from __intel_framebuffer_create to where the gem object gets refed.

[Changelog is in preceding patch by Tvrtko Ursulin.]

Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
    allocation")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Ville Syrjala Oct. 13, 2015, 3:39 p.m. UTC | #1
On Sat, Jul 04, 2015 at 11:50:58AM +0200, Lukas Wunner wrote:
> Currently when allocating a framebuffer fails, the gem object gets
> unrefed at the bottom of the call chain in __intel_framebuffer_create,
> not where it gets refed, which is in intel_framebuffer_create_for_mode
> (via i915_gem_alloc_object) and in intel_user_framebuffer_create
> (via drm_gem_object_lookup).
> 
> This invites mistakes: As discovered by Tvrtko Ursulin, a double unref
> has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).
> 
> As suggested by Ville Syrjälä, improve code clarity by moving the unref
> away from __intel_framebuffer_create to where the gem object gets refed.
> 
> [Changelog is in preceding patch by Tvrtko Ursulin.]
> 
> Tested-by: Pierre Moreau <pierre.morrow@free.fr>
>     [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina]
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
>     allocation")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 421ac15..68d051d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10033,20 +10033,17 @@ __intel_framebuffer_create(struct drm_device *dev,
>  	int ret;
>  
>  	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
> -	if (!intel_fb) {
> -		drm_gem_object_unreference(&obj->base);
> +	if (!intel_fb)
>  		return ERR_PTR(-ENOMEM);
> -	}
>  
>  	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
>  	if (ret)
>  		goto err;
>  
>  	return &intel_fb->base;
> +
>  err:
> -	drm_gem_object_unreference(&obj->base);
>  	kfree(intel_fb);
> -
>  	return ERR_PTR(ret);
>  }
>  
> @@ -10086,6 +10083,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
>  				  struct drm_display_mode *mode,
>  				  int depth, int bpp)
>  {
> +	struct drm_framebuffer *fb;
>  	struct drm_i915_gem_object *obj;
>  	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
>  
> @@ -10100,7 +10098,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
>  								bpp);
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
>  
> -	return intel_framebuffer_create(dev, &mode_cmd, obj);
> +	fb = intel_framebuffer_create(dev, &mode_cmd, obj);
> +	if (IS_ERR(fb))
> +		drm_gem_object_unreference_unlocked(&obj->base);
> +
> +	return fb;
>  }
>  
>  static struct drm_framebuffer *
> @@ -14289,6 +14291,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
>  			      struct drm_file *filp,
>  			      struct drm_mode_fb_cmd2 *mode_cmd)
>  {
> +	struct drm_framebuffer *fb;
>  	struct drm_i915_gem_object *obj;
>  
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
> @@ -14296,7 +14299,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
>  	if (&obj->base == NULL)
>  		return ERR_PTR(-ENOENT);
>  
> -	return intel_framebuffer_create(dev, mode_cmd, obj);
> +	fb = intel_framebuffer_create(dev, mode_cmd, obj);
> +	if (IS_ERR(fb))
> +		drm_gem_object_unreference_unlocked(&obj->base);
> +
> +	return fb;

This one looks good to me
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  #ifndef CONFIG_DRM_FBDEV_EMULATION
> -- 
> 2.1.0
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 421ac15..68d051d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10033,20 +10033,17 @@  __intel_framebuffer_create(struct drm_device *dev,
 	int ret;
 
 	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
-	if (!intel_fb) {
-		drm_gem_object_unreference(&obj->base);
+	if (!intel_fb)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
 	if (ret)
 		goto err;
 
 	return &intel_fb->base;
+
 err:
-	drm_gem_object_unreference(&obj->base);
 	kfree(intel_fb);
-
 	return ERR_PTR(ret);
 }
 
@@ -10086,6 +10083,7 @@  intel_framebuffer_create_for_mode(struct drm_device *dev,
 				  struct drm_display_mode *mode,
 				  int depth, int bpp)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
 
@@ -10100,7 +10098,11 @@  intel_framebuffer_create_for_mode(struct drm_device *dev,
 								bpp);
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
 
-	return intel_framebuffer_create(dev, &mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, &mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 static struct drm_framebuffer *
@@ -14289,6 +14291,7 @@  intel_user_framebuffer_create(struct drm_device *dev,
 			      struct drm_file *filp,
 			      struct drm_mode_fb_cmd2 *mode_cmd)
 {
+	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
@@ -14296,7 +14299,11 @@  intel_user_framebuffer_create(struct drm_device *dev,
 	if (&obj->base == NULL)
 		return ERR_PTR(-ENOENT);
 
-	return intel_framebuffer_create(dev, mode_cmd, obj);
+	fb = intel_framebuffer_create(dev, mode_cmd, obj);
+	if (IS_ERR(fb))
+		drm_gem_object_unreference_unlocked(&obj->base);
+
+	return fb;
 }
 
 #ifndef CONFIG_DRM_FBDEV_EMULATION