| Message ID | fb5b2128c791be6fbf294ffe182cfcc99cb9dd83.1436007416.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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 9079fcd..9499003 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8876,20 +8876,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); > } > > @@ -8929,6 +8926,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 }; > > @@ -8943,7 +8941,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(&obj->base); This needs to be drm_gem_object_unreference_unlocked(). It's much simpler if you just document this as consuming the obj reference. If you want to fix it, you have to move the struct_mutex into the caller i.e. eliminate intel_framebuffer_create() and call __intel_framebuffer_create(). -Chris
Hi Chris, thank you for the quick response (on a weekend no less). On Sat, Jul 04, 2015 at 01:31:48PM +0100, Chris Wilson wrote: > > - return intel_framebuffer_create(dev, &mode_cmd, obj); > > + fb = intel_framebuffer_create(dev, &mode_cmd, obj); > > + if (IS_ERR(fb)) > > + drm_gem_object_unreference(&obj->base); > > This needs to be drm_gem_object_unreference_unlocked(). You're absolutely right, thanks for pointing this out. I'm posting a rectified v4 right now. > It's much simpler if you just document this as consuming the > obj reference. Yes but I believe that is what Ville took exception to. If you guys all agree that documenting this is sufficient then you can just merge Tvrtko's v2. The rationale of the v3 + v4 I've submitted is to offer an alternative in the hope of pushing this forward. > If you want to fix it, you have to move the struct_mutex into > the caller i.e. eliminate intel_framebuffer_create() and call > __intel_framebuffer_create(). Hm, I don't understand. The (locking) intel_framebuffer_create is used by intel_framebuffer_create_for_mode as well as intel_user_framebuffer_create. The (non-locking) __intel_framebuffer_create is used by intelfb_alloc. So it seems both are needed. Daniel added __intel_framebuffer_create with a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb allocation"). Incidentally this is also the commit that introduced the double unref. :-) We could eliminate the (non-locking) __intel_framebuffer_create however by briefly unlocking struct_mutex in intelfb_alloc after i915_gem_alloc_object and then relocking before intel_pin_and_fence_fb_obj (this is on top of Tvrtko's patch which moves the locking from intelfb_create to intelfb_alloc). Best regards, Lukas
On Sun, Jul 05, 2015 at 06:49:02PM +0200, Lukas Wunner wrote: > Hi Chris, > > thank you for the quick response (on a weekend no less). > > > On Sat, Jul 04, 2015 at 01:31:48PM +0100, Chris Wilson wrote: > > > - return intel_framebuffer_create(dev, &mode_cmd, obj); > > > + fb = intel_framebuffer_create(dev, &mode_cmd, obj); > > > + if (IS_ERR(fb)) > > > + drm_gem_object_unreference(&obj->base); > > > > This needs to be drm_gem_object_unreference_unlocked(). > > You're absolutely right, thanks for pointing this out. > I'm posting a rectified v4 right now. > > > > It's much simpler if you just document this as consuming the > > obj reference. > > Yes but I believe that is what Ville took exception to. > > If you guys all agree that documenting this is sufficient then > you can just merge Tvrtko's v2. The rationale of the v3 + v4 > I've submitted is to offer an alternative in the hope of pushing > this forward. > > > > If you want to fix it, you have to move the struct_mutex into > > the caller i.e. eliminate intel_framebuffer_create() and call > > __intel_framebuffer_create(). > > Hm, I don't understand. The (locking) intel_framebuffer_create > is used by intel_framebuffer_create_for_mode as well as > intel_user_framebuffer_create. > > The (non-locking) __intel_framebuffer_create is used by > intelfb_alloc. So it seems both are needed. Daniel added > __intel_framebuffer_create with a8bb6818270c ("drm/i915: > Fix error path leak in fbdev fb allocation"). Incidentally > this is also the commit that introduced the double unref. :-) > > We could eliminate the (non-locking) __intel_framebuffer_create > however by briefly unlocking struct_mutex in intelfb_alloc after > i915_gem_alloc_object and then relocking before > intel_pin_and_fence_fb_obj (this is on top of Tvrtko's patch > which moves the locking from intelfb_create to intelfb_alloc). I mean, there will only be the non-locking variant, the mutex is moved to the caller. -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9079fcd..9499003 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8876,20 +8876,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); } @@ -8929,6 +8926,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 }; @@ -8943,7 +8941,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(&obj->base); + + return fb; } static struct drm_framebuffer * @@ -13379,6 +13381,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, @@ -13386,7 +13389,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(&obj->base); + + return fb; } static void intel_output_poll_changed(struct drm_device *dev)
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. 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(-)