Re: [PATCH] drm/i915: Fix failure paths around initial fbdev allocation

[Lukas Wunner <lukas@wunner.de>]

Hi,

On Mon, Jun 15, 2015 at 11:49:52AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> We had two 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.

s/intelfb_alloc/intelfb_create/
s/(caller of intelfb_alloc)//


> 
> 2.
> Double unreference on the object if __intel_framebuffer_create fails since
> both it and the caller (intelfb_alloc) do the unreference.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
Reported-By: Lukas Wunner <lukas@wunner.de>


> ---
>  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 6372cfc..b998f69 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);

Hm, the order of drm_gem_object_unreference() and drm_framebuffer_remove()
is reversed now, could this cause any issues?

Apart from that,

Reviewed-By: Lukas Wunner <lukas@wunner.de>

I will also test the patch and report back in a bit.

Thanks a lot,

Lukas


>  	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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx