[v6,3/4] drm/i915: Fix failure paths around initial fbdev allocation

Commit Message

Lukas Wunner June 30, 2015, 9:06 a.m. UTC

From: Tvrtko Ursulin <[e-mail]>

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.

2.
Deadlock in intelfb_create failure path where it calls
drm_framebuffer_unreference, which grabs the struct mutex and
intelfb_create was already holding it.

[Chris Wilson on why struct_mutex needs to be locked in the second half
of intelfb_create: "there is a bug here where we don't take an explicit
pin on the VMA we setup for the fbdev which requires the lock."]

v2:
   * Reformat commit msg to 72 chars. (Lukas Wunner)
   * Add third failure mode. (Lukas Wunner)

v5:
   * Rebase on drm-intel-nightly 2015y-09m-01d-09h-06m-08s UTC,
     rephrase commit message. (Jani Nicula)

v6:
   * In intelfb_alloc, if __intel_framebuffer_create failed,
     fb will be an ERR_PTR, thus not null. So in the failure
     path we need to check for IS_ERR_OR_NULL to avoid calling
     drm_framebuffer_remove on the ERR_PTR. (Lukas Wunner)
   * Since this is init code a drm_framebuffer_unreference should
     be all we need. drm_framebuffer_remove is for framebuffers
     that userspace has created - and is getting somewhat
     defeatured. (Daniel Vetter)

Signed-off-by: Tvrtko Ursulin <[e-mail]>
Fixes: 60a5ca015ffd ("drm/i915: Add locking around
    framebuffer_references--")
Reported-by: Lukas Wunner <[e-mail]>
[Lukas: Create v3 + v4 + v5 + v6 based on Tvrtko's v2]
Signed-off-by: Lukas Wunner <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Daniel Vetter Oct. 30, 2015, 6:28 p.m. UTC | #1

On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> From: Tvrtko Ursulin <[e-mail]>
> 
> 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.
> 
> 2.
> Deadlock in intelfb_create failure path where it calls
> drm_framebuffer_unreference, which grabs the struct mutex and
> intelfb_create was already holding it.
> 
> [Chris Wilson on why struct_mutex needs to be locked in the second half
> of intelfb_create: "there is a bug here where we don't take an explicit
> pin on the VMA we setup for the fbdev which requires the lock."]

The vma is pinned, the problem is that we re-lookup it a few times, which
is racy. We should instead track the vma directly, but oh well we don't.

With that clarified in the commit message this is

Reviewed-by: Daniel Vetter <[e-mail]>
> 
> v2:
>    * Reformat commit msg to 72 chars. (Lukas Wunner)
>    * Add third failure mode. (Lukas Wunner)
> 
> v5:
>    * Rebase on drm-intel-nightly 2015y-09m-01d-09h-06m-08s UTC,
>      rephrase commit message. (Jani Nicula)
> 
> v6:
>    * In intelfb_alloc, if __intel_framebuffer_create failed,
>      fb will be an ERR_PTR, thus not null. So in the failure
>      path we need to check for IS_ERR_OR_NULL to avoid calling
>      drm_framebuffer_remove on the ERR_PTR. (Lukas Wunner)
>    * Since this is init code a drm_framebuffer_unreference should
>      be all we need. drm_framebuffer_remove is for framebuffers
>      that userspace has created - and is getting somewhat
>      defeatured. (Daniel Vetter)
> 
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> Fixes: 60a5ca015ffd ("drm/i915: Add locking around
>     framebuffer_references--")
> Reported-by: Lukas Wunner <[e-mail]>
> [Lukas: Create v3 + v4 + v5 + v6 based on Tvrtko's v2]
> Signed-off-by: Lukas Wunner <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index ec82b51..12597b5 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -119,7 +119,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_i915_private *dev_priv = to_i915(dev);
>  	struct drm_mode_fb_cmd2 mode_cmd = {};
> @@ -138,6 +138,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);
>  
> @@ -165,16 +167,19 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
>  	if (ret) {
>  		DRM_ERROR("failed to pin obj: %d\n", ret);
> -		goto out_fb;
> +		goto out;
>  	}
>  
> +	mutex_unlock(&dev->struct_mutex);
> +
>  	ifbdev->fb = to_intel_framebuffer(fb);
>  
>  	return 0;
>  
> -out_fb:
> -	drm_framebuffer_remove(fb);
>  out:
> +	mutex_unlock(&dev->struct_mutex);
> +	if (!IS_ERR_OR_NULL(fb))
> +		drm_framebuffer_unreference(fb);
>  	return ret;
>  }
>  
> @@ -192,8 +197,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)) {
> @@ -208,7 +211,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");
> @@ -220,6 +223,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	obj = intel_fb->obj;
>  	size = obj->base.size;
>  
> +	mutex_lock(&dev->struct_mutex);
> +
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		ret = PTR_ERR(info);
> @@ -281,7 +286,6 @@ out_destroy_fbi:
>  out_unpin:
>  	i915_gem_object_ggtt_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);
> -out_unlock:
>  	mutex_unlock(&dev->struct_mutex);
>  	return ret;
>  }
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lukas Wunner Nov. 7, 2015, 10:41 a.m. UTC | #2

Hi Jani,

three patches with fbdev deadlock & failure path fixes,
each with Reviewed-by tag by Ville or Daniel, the third one
with amended commit message as requested by Daniel in
<[e-mail]>.

Patch 1 fixes double unref of bo in failure path of intelfb_alloc().
Patch 2 fixes another double unref of bo in failure path of intelfb_alloc().
Patch 3 fixes deadlocks in intelfb_create(), one during driver
initialization and one in failure path.

Browsable on GitHub:
https://github.com/l1k/linux/commits/intel_fbdev_fixes

The previous iteration (v6, <[e-mail]>)
contained a 4th patch, a new version of which I'll submit separately.

Thanks,

Lukas


Lukas Wunner (2):
  drm/i915: On fb alloc failure, unref gem object where it gets refed
  drm/i915: Fix double unref in intelfb_alloc failure path

Tvrtko Ursulin (1):
  drm/i915: Fix failure paths around initial fbdev allocation

 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
 drivers/gpu/drm/i915/intel_fbdev.c   | 25 ++++++++++++++-----------
 2 files changed, 28 insertions(+), 18 deletions(-)

Jani Nikula Nov. 9, 2015, 2:23 p.m. UTC | #3

On Sat, 07 Nov 2015, Lukas Wunner <[e-mail]> wrote:
> Hi Jani,
>
> three patches with fbdev deadlock & failure path fixes,
> each with Reviewed-by tag by Ville or Daniel, the third one
> with amended commit message as requested by Daniel in
> <[e-mail]>.
>
> Patch 1 fixes double unref of bo in failure path of intelfb_alloc().
> Patch 2 fixes another double unref of bo in failure path of intelfb_alloc().
> Patch 3 fixes deadlocks in intelfb_create(), one during driver
> initialization and one in failure path.
>
> Browsable on GitHub:
> https://github.com/l1k/linux/commits/intel_fbdev_fixes
>
> The previous iteration (v6, <[e-mail]>)
> contained a 4th patch, a new version of which I'll submit separately.

Pushed all three to drm-intel-next-queued. Thanks for the patches and
review.

For future reference, please consider posting new versions of series as
new threads. This one got pretty messy in the end, with so many
different versions.

Also, I don't know how you generate and send your patches, but even the
updated patches had dates of the original or very old versions of
them. Like [1] is June 30 although you sent it just a couple of days
ago. Please look into that.

BR,
Jani.


[1] http://mid.gmane.org/[e-mail]

Lukas Wunner Nov. 12, 2015, 7:20 p.m. UTC | #4

Hi Jani,

On Mon, Nov 09, 2015 at 04:23:34PM +0200, Jani Nikula wrote:
> On Sat, 07 Nov 2015, Lukas Wunner <[e-mail]> wrote:
> > three patches with fbdev deadlock & failure path fixes,
> > each with Reviewed-by tag by Ville or Daniel, the third one
> > with amended commit message as requested by Daniel in
> > <[e-mail]>.
> Pushed all three to drm-intel-next-queued. Thanks for the patches and
> review.

Thank you!


> Also, I don't know how you generate and send your patches, but even the
> updated patches had dates of the original or very old versions of
> them. Like [1] is June 30 although you sent it just a couple of days
> ago. Please look into that.

git format-patch tries to tunnel the author date through RFC 5322
by setting the Date-header to it.

git send-email, which most people use, strips that and replaces it
with the date of its invocation.

I use msmtp instead of git send-email, which preserves the Date-header.

When I edit commits, git commit --amend updates the commit date but not
the author date.

That's why you see these ancient timestamps.

If you find that annoying I'll see to it that I modify the author date
manually before sending out a new series. (At least in commits of my own.
I try to avoid tampering with other people's commits.)


> For future reference, please consider posting new versions of series as
> new threads. This one got pretty messy in the end, with so many
> different versions.

Daniel asked me to submit a patch "in-reply the previous version" in
<[e-mail]> and I adhered to that request
also when sending a new version of an entire series. In that case I'll
*not* submit in-reply-to in the future, got that.

Best regards,

Lukas

Jani Nikula Nov. 13, 2015, 7:06 a.m. UTC | #5

On Thu, 12 Nov 2015, Lukas Wunner <[e-mail]> wrote:
> I use msmtp instead of git send-email, which preserves the Date-header.

Any particular reason not to use git send-email?

You can configure it to use a sendmail-like program instead of a server,
and actually that's the default. If you have msmtp installed as sendmail
(for example msmtp-mta package does this on Debian) git send-email will
use that out of the box. FWIW this is exactly what I do.

> When I edit commits, git commit --amend updates the commit date but not
> the author date.
>
> That's why you see these ancient timestamps.
>
> If you find that annoying I'll see to it that I modify the author date
> manually before sending out a new series. (At least in commits of my own.
> I try to avoid tampering with other people's commits.)

It's just that my mail client uses the Date: header for sorting, and the
old ones tend to get buried. It's disadvantageous to you to use old
dates. ;)

>> For future reference, please consider posting new versions of series as
>> new threads. This one got pretty messy in the end, with so many
>> different versions.
>
> Daniel asked me to submit a patch "in-reply the previous version" in
> <[e-mail]> and I adhered to that request
> also when sending a new version of an entire series. In that case I'll
> *not* submit in-reply-to in the future, got that.

You'll probably get a different reply from everyone you ask. ;)

My rule of thumb is that if you update individual patches (whether in a
series or not), send them in-reply to the previous version. Each patch
in-reply to its preceding version in the series. If you update more than
about half the patches in a series, it's perhaps less confusing to send
a new series with no in-reply-to. (Oh, and this contradicts with the
example in git send-email man page.)

BR,
Jani.

Daniel Vetter Nov. 17, 2015, 1:44 p.m. UTC | #6

On Fri, Nov 13, 2015 at 09:06:29AM +0200, Jani Nikula wrote:
> On Thu, 12 Nov 2015, Lukas Wunner <[e-mail]> wrote:
> >> For future reference, please consider posting new versions of series as
> >> new threads. This one got pretty messy in the end, with so many
> >> different versions.
> >
> > Daniel asked me to submit a patch "in-reply the previous version" in
> > <[e-mail]> and I adhered to that request
> > also when sending a new version of an entire series. In that case I'll
> > *not* submit in-reply-to in the future, got that.
> 
> You'll probably get a different reply from everyone you ask. ;)
> 
> My rule of thumb is that if you update individual patches (whether in a
> series or not), send them in-reply to the previous version. Each patch
> in-reply to its preceding version in the series. If you update more than
> about half the patches in a series, it's perhaps less confusing to send
> a new series with no in-reply-to. (Oh, and this contradicts with the
> example in git send-email man page.)

I have the same rule-of-thumb, but probably failed to convey it clearly.
Sorry for the confusion.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index ec82b51..12597b5 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -119,7 +119,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_i915_private *dev_priv = to_i915(dev);
 	struct drm_mode_fb_cmd2 mode_cmd = {};
@@ -138,6 +138,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);
 
@@ -165,16 +167,19 @@  static int intelfb_alloc(struct drm_fb_helper *helper,
 	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
 	if (ret) {
 		DRM_ERROR("failed to pin obj: %d\n", ret);
-		goto out_fb;
+		goto out;
 	}
 
+	mutex_unlock(&dev->struct_mutex);
+
 	ifbdev->fb = to_intel_framebuffer(fb);
 
 	return 0;
 
-out_fb:
-	drm_framebuffer_remove(fb);
 out:
+	mutex_unlock(&dev->struct_mutex);
+	if (!IS_ERR_OR_NULL(fb))
+		drm_framebuffer_unreference(fb);
 	return ret;
 }
 
@@ -192,8 +197,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)) {
@@ -208,7 +211,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");
@@ -220,6 +223,8 @@  static int intelfb_create(struct drm_fb_helper *helper,
 	obj = intel_fb->obj;
 	size = obj->base.size;
 
+	mutex_lock(&dev->struct_mutex);
+
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -281,7 +286,6 @@  out_destroy_fbi:
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
 	drm_gem_object_unreference(&obj->base);
-out_unlock:
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }