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-and-tested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 781c26c37b3880309734f4066dd05062e1628698..17b1cba3b5f11c281eedc13c5adaa5a98ac4eef8 100644 (file)
--- 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.