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.]
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 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 -- work in progress]
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 907b73e..5984d5f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10145,20 +10145,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); }
@@ -10198,6 +10195,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 };
@@ -10212,7 +10210,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 * @@ -14468,6 +14470,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, @@ -14475,7 +14478,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; }
static void intel_output_poll_changed(struct drm_device *dev)