Attachment 40332 Details for Bug 31651 – Recursive locking is bad.

[patch] Recursive locking is bad.

0001-drm-i915-Do-not-hold-mutex-when-faulting-in-user-add.patch (text/plain), 4.36 KB, created by Chris Wilson on 2010-11-17 03:08:50 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Wilson

Created: 2010-11-17 03:08:50 UTC

Size: 4.36 KB

patch

obsolete

>From 5e0e0c866ed05f90b9a98c6ac0aff66d85db89e0 Mon Sep 17 00:00:00 2001
>From: Chris Wilson <chris@chris-wilson.co.uk>
>Date: Wed, 17 Nov 2010 09:10:42 +0000
>Subject: [PATCH] drm/i915: Do not hold mutex when faulting in user addresses
>
>Linus Torvalds found that it was rather trivial to trigger a system
>freeze:
>
>  In fact, with lockdep, I don't even need to do the sysrq-d thing: it
>  shows the bug as it happens. It's the X server taking the same lock
>  recursively.
>
>  Here's the problem:
>
>    =============================================
>    [ INFO: possible recursive locking detected ]
>    2.6.37-rc2-00012-gbdbd01a #7
>    ---------------------------------------------
>    Xorg/2816 is trying to acquire lock:
>     (&dev->struct_mutex){+.+.+.}, at: [<ffffffff812c626c>] i915_gem_fault+0x50/0x17e
>
>    but task is already holding lock:
>     (&dev->struct_mutex){+.+.+.}, at: [<ffffffff812c403b>] i915_mutex_lock_interruptible+0x28/0x4a
>
>    other info that might help us debug this:
>    2 locks held by Xorg/2816:
>     #0:  (&dev->struct_mutex){+.+.+.}, at: [<ffffffff812c403b>] i915_mutex_lock_interruptible+0x28/0x4a
>     #1:  (&mm->mmap_sem){++++++}, at: [<ffffffff81022d4f>] page_fault+0x156/0x37b
>
>This recursion was introduced by rearranging the locking to avoid the
>double locking on the fast path (4f27b5d and fbd5a26d) and the
>introduction of the prefault to encourage the fast paths (b5e4f2b). In
>order to undo the problem, we rearrange the code to perform the access
>validation upfront, attempt to prefault and then fight for control of the
>mutex.  the best case scenario where the mutex is uncontended the
>prefaulting is not wasted.
>
>Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> drivers/gpu/drm/i915/i915_gem.c |   63 ++++++++++++++++----------------------
> 1 files changed, 27 insertions(+), 36 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>index eba9b16..dd7d142 100644
>--- a/drivers/gpu/drm/i915/i915_gem.c
>+++ b/drivers/gpu/drm/i915/i915_gem.c
>@@ -547,6 +547,19 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
> 	struct drm_i915_gem_object *obj_priv;
> 	int ret = 0;
> 
>+	if (args->size == 0)
>+		return 0;
>+
>+	if (!access_ok(VERIFY_WRITE,
>+		       (char __user *)(uintptr_t)args->data_ptr,
>+		       args->size))
>+		return -EFAULT;
>+
>+	ret = fault_in_pages_writeable((char __user *)(uintptr_t)args->data_ptr,
>+				       args->size);
>+	if (ret)
>+		return -EFAULT;
>+
> 	ret = i915_mutex_lock_interruptible(dev);
> 	if (ret)
> 		return ret;
>@@ -564,23 +577,6 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
> 		goto out;
> 	}
> 
>-	if (args->size == 0)
>-		goto out;
>-
>-	if (!access_ok(VERIFY_WRITE,
>-		       (char __user *)(uintptr_t)args->data_ptr,
>-		       args->size)) {
>-		ret = -EFAULT;
>-		goto out;
>-	}
>-
>-	ret = fault_in_pages_writeable((char __user *)(uintptr_t)args->data_ptr,
>-				       args->size);
>-	if (ret) {
>-		ret = -EFAULT;
>-		goto out;
>-	}
>-
> 	ret = i915_gem_object_get_pages_or_evict(obj);
> 	if (ret)
> 		goto out;
>@@ -981,7 +977,20 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> 	struct drm_i915_gem_pwrite *args = data;
> 	struct drm_gem_object *obj;
> 	struct drm_i915_gem_object *obj_priv;
>-	int ret = 0;
>+	int ret;
>+
>+	if (args->size == 0)
>+		return 0;
>+
>+	if (!access_ok(VERIFY_READ,
>+		       (char __user *)(uintptr_t)args->data_ptr,
>+		       args->size))
>+		return -EFAULT;
>+
>+	ret = fault_in_pages_readable((char __user *)(uintptr_t)args->data_ptr,
>+				      args->size);
>+	if (ret)
>+		return -EFAULT;
> 
> 	ret = i915_mutex_lock_interruptible(dev);
> 	if (ret)
>@@ -994,30 +1003,12 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> 	}
> 	obj_priv = to_intel_bo(obj);
> 
>-
> 	/* Bounds check destination. */
> 	if (args->offset > obj->size || args->size > obj->size - args->offset) {
> 		ret = -EINVAL;
> 		goto out;
> 	}
> 
>-	if (args->size == 0)
>-		goto out;
>-
>-	if (!access_ok(VERIFY_READ,
>-		       (char __user *)(uintptr_t)args->data_ptr,
>-		       args->size)) {
>-		ret = -EFAULT;
>-		goto out;
>-	}
>-
>-	ret = fault_in_pages_readable((char __user *)(uintptr_t)args->data_ptr,
>-				      args->size);
>-	if (ret) {
>-		ret = -EFAULT;
>-		goto out;
>-	}
>-
> 	/* We can only do the GTT pwrite on untiled buffers, as otherwise
> 	 * it would end up going through the fenced access, and we'll get
> 	 * different detiling behavior between reading and writing.
>-- 
>1.7.2.3
>

Actions: View | Splinter Review

Attachments on bug 31651: 40332

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.