[Intel-gfx] [PATCH] drm/i915: Fix failure paths around initial fbdev allocation

[Ville Syrjälä]

On Tue, Jun 30, 2015 at 10:06:27AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> 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 at intel.com>
> Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
> Reported-By: Lukas Wunner <lukas at wunner.de>
> Tested-By: Lukas Wunner <lukas at wunner.de>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Lukas Wunner <lukas at wunner.de>
> ---
> 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