On Tue, Jun 30, 2015 at 10:06:27AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> We had three failure modes here:
>
> 1.
> Deadlock in intelfb_alloc failure path where it calls
> drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> (caller of intelfb_alloc) was already holding it.
>
> 2.
> Double unreference on the object if __intel_framebuffer_create fails
> since both it and the caller (intelfb_alloc) do the unreference.
>
> 3.
> Deadlock in intelfb_create failure path where it calls
> drm_framebuffer_unreference, which grabs the struct mutex and
> intelfb_create was already holding it.
>
> v2:
> * Reformat commit msg to 72 chars. (Lukas Wunner)
> * Added third failure mode. (Lukas Wunner)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
> Reported-By: Lukas Wunner <lukas@xxxxxxxxx>
> Tested-By: Lukas Wunner <lukas@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Lukas Wunner <lukas@xxxxxxxxx>
> ---
> Ville, you suggested some changes earlier;
>
> """
> I suggest removing the unref from __intel_framebuffer_create().
> """
>
> Do you view that as an improvement in code clarity/organisation,
> or you think my version is actually wrong for some reason?
I find it rather unexpected that the function drops the passed
reference on error. My usual rule is: do nothing on error, if possible.
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 2a1724e..118420b 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -141,7 +141,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> {
> struct intel_fbdev *ifbdev =
> container_of(helper, struct intel_fbdev, helper);
> - struct drm_framebuffer *fb;
> + struct drm_framebuffer *fb = NULL;
> struct drm_device *dev = helper->dev;
> struct drm_mode_fb_cmd2 mode_cmd = {};
> struct drm_i915_gem_object *obj;
> @@ -159,6 +159,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> sizes->surface_depth);
>
> + mutex_lock(&dev->struct_mutex);
> +
> size = mode_cmd.pitches[0] * mode_cmd.height;
> size = PAGE_ALIGN(size);
> obj = i915_gem_object_create_stolen(dev, size);
> @@ -172,8 +174,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>
> fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
> if (IS_ERR(fb)) {
> + /* Drops object reference on failure. */
> ret = PTR_ERR(fb);
> - goto out_unref;
> + goto out;
> }
>
> /* Flush everything out, we'll be doing GTT only from now on */
> @@ -183,15 +186,18 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> goto out_fb;
> }
>
> + mutex_unlock(&dev->struct_mutex);
> +
> ifbdev->fb = to_intel_framebuffer(fb);
>
> return 0;
>
> out_fb:
> - drm_framebuffer_remove(fb);
> -out_unref:
> drm_gem_object_unreference(&obj->base);
> out:
> + mutex_unlock(&dev->struct_mutex);
> + if (fb)
> + drm_framebuffer_remove(fb);
> return ret;
> }
>
> @@ -209,8 +215,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> int size, ret;
> bool prealloc = false;
>
> - mutex_lock(&dev->struct_mutex);
> -
> if (intel_fb &&
> (sizes->fb_width > intel_fb->base.width ||
> sizes->fb_height > intel_fb->base.height)) {
> @@ -225,7 +229,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
> ret = intelfb_alloc(helper, sizes);
> if (ret)
> - goto out_unlock;
> + return ret;
> intel_fb = ifbdev->fb;
> } else {
> DRM_DEBUG_KMS("re-using BIOS fb\n");
> @@ -237,6 +241,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
> obj = intel_fb->obj;
> size = obj->base.size;
>
> + mutex_lock(&dev->struct_mutex);
> +
> info = framebuffer_alloc(0, &dev->pdev->dev);
> if (!info) {
> ret = -ENOMEM;
> @@ -307,7 +313,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> out_unpin:
> i915_gem_object_ggtt_unpin(obj);
> drm_gem_object_unreference(&obj->base);
> -out_unlock:
> mutex_unlock(&dev->struct_mutex);
> return ret;
> }
> --
> 2.4.2
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
