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@xxxxxxx>
> [MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]
> Tested-by: Paul Hordiienko <pvt.gord@xxxxxxxxx>
> [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
> Tested-by: William Brown <william@xxxxxxxxxxxxxxxx>
> [MBP 8,2 2011 intel SNB + amd turks pre-retina]
> Tested-by: Lukas Wunner <lukas@xxxxxxxxx>
> [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
> Tested-by: Bruno Bierbaumer <bruno@xxxxxxxxxxxxxx>
> [MBP 11,3 2013 intel HSW + nvidia GK107 retina]
>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
> allocation")
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> }
>
> #ifndef CONFIG_DRM_FBDEV_EMULATION
> --
> 2.1.0
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
