Techwizz/kernel-fxtec-pro1x
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

drm/i915: Do not hold mutex when faulting in user addresses

Browse source 
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>
This commit is contained in:
Chris Wilson 2010-11-17 09:10:42 +00:00
parent 1bb95834bb
commit 51311d0a5c
1 changed files with 27 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

63
drivers/gpu/drm/i915/i915_gem.c
View file

@ -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.