Commit ad46cb53 authored by Chris Wilson's avatar Chris Wilson Committed by Jani Nikula

drm/i915: Prevent recursive deadlock on releasing a busy userptr

During release of the GEM object we hold the struct_mutex. As the
object may be holding onto the last reference for the task->mm,
calling mmput() may trigger exit_mmap() which close the vma
which will call drm_gem_vm_close() and attempt to reacquire
the struct_mutex. In order to avoid that recursion, we have
to defer the mmput() until after we drop the struct_mutex,
i.e. we need to schedule a worker to do the clean up. A further issue
spotted by Tvrtko was caused when we took a GTT mmapping of a userptr
buffer object. In that case, we would never call mmput as the object
would be cyclically referenced by the GTT mmapping and not freed upon
process exit - keeping the entire process mm alive after the process
task was reaped. The fix employed is to replace the mm_users/mmput()
reference handling to mm_count/mmdrop() for the shared i915_mm_struct.

   INFO: task test_surfaces:1632 blocked for more than 120 seconds.
         Tainted: GF          O 3.14.5+ #1
   "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
   test_surfaces   D 0000000000000000     0  1632   1590 0x00000082
    ffff88014914baa8 0000000000000046 0000000000000000 ffff88014914a010
    0000000000012c40 0000000000012c40 ffff8800a0058210 ffff88014784b010
    ffff88014914a010 ffff880037b1c820 ffff8800a0058210 ffff880037b1c824
   Call Trace:
    [<ffffffff81582499>] schedule+0x29/0x70
    [<ffffffff815825fe>] schedule_preempt_disabled+0xe/0x10
    [<ffffffff81583b93>] __mutex_lock_slowpath+0x183/0x220
    [<ffffffff81583c53>] mutex_lock+0x23/0x40
    [<ffffffffa005c2a3>] drm_gem_vm_close+0x33/0x70 [drm]
    [<ffffffff8115a483>] remove_vma+0x33/0x70
    [<ffffffff8115a5dc>] exit_mmap+0x11c/0x170
    [<ffffffff8104d6eb>] mmput+0x6b/0x100
    [<ffffffffa00f44b9>] i915_gem_userptr_release+0x89/0xc0 [i915]
    [<ffffffffa00e6706>] i915_gem_free_object+0x126/0x250 [i915]
    [<ffffffffa005c06a>] drm_gem_object_free+0x2a/0x40 [drm]
    [<ffffffffa005cc32>] drm_gem_object_handle_unreference_unlocked+0xe2/0x120 [drm]
    [<ffffffffa005ccd4>] drm_gem_object_release_handle+0x64/0x90 [drm]
    [<ffffffff8127ffeb>] idr_for_each+0xab/0x100
    [<ffffffffa005cc70>] ?  drm_gem_object_handle_unreference_unlocked+0x120/0x120 [drm]
    [<ffffffff81583c46>] ? mutex_lock+0x16/0x40
    [<ffffffffa005c354>] drm_gem_release+0x24/0x40 [drm]
    [<ffffffffa005b82b>] drm_release+0x3fb/0x480 [drm]
    [<ffffffff8118d482>] __fput+0xb2/0x260
    [<ffffffff8118d6de>] ____fput+0xe/0x10
    [<ffffffff8106f27f>] task_work_run+0x8f/0xf0
    [<ffffffff81052228>] do_exit+0x1a8/0x480
    [<ffffffff81052551>] do_group_exit+0x51/0xc0
    [<ffffffff810525d7>] SyS_exit_group+0x17/0x20
    [<ffffffff8158e092>] system_call_fastpath+0x16/0x1b

v2: Incorporate feedback from Tvrtko and remove the unnessary mm
referencing when creating the i915_mm_struct and improve some of the
function names and comments.
Reported-by: default avatarJacek Danecki <jacek.danecki@intel.com>
Test-case: igt/gem_userptr_blits/process-exit*
Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Tested-by: default avatar"Gong, Zhipeng" <zhipeng.gong@intel.com>
Cc: Jacek Danecki <jacek.danecki@intel.com>
Cc: "Ursulin, Tvrtko" <tvrtko.ursulin@intel.com>
Reviewed-by: default avatar"Ursulin, Tvrtko" <tvrtko.ursulin@intel.com>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org # hold off until 3.17 ships for additional testing
Signed-off-by: default avatarJani Nikula <jani.nikula@intel.com>
parent 2ce7598c
...@@ -184,6 +184,7 @@ enum hpd_pin { ...@@ -184,6 +184,7 @@ enum hpd_pin {
if ((1 << (domain)) & (mask)) if ((1 << (domain)) & (mask))
struct drm_i915_private; struct drm_i915_private;
struct i915_mm_struct;
struct i915_mmu_object; struct i915_mmu_object;
enum intel_dpll_id { enum intel_dpll_id {
...@@ -1506,9 +1507,8 @@ struct drm_i915_private { ...@@ -1506,9 +1507,8 @@ struct drm_i915_private {
struct i915_gtt gtt; /* VM representing the global address space */ struct i915_gtt gtt; /* VM representing the global address space */
struct i915_gem_mm mm; struct i915_gem_mm mm;
#if defined(CONFIG_MMU_NOTIFIER) DECLARE_HASHTABLE(mm_structs, 7);
DECLARE_HASHTABLE(mmu_notifiers, 7); struct mutex mm_lock;
#endif
/* Kernel Modesetting */ /* Kernel Modesetting */
...@@ -1814,8 +1814,8 @@ struct drm_i915_gem_object { ...@@ -1814,8 +1814,8 @@ struct drm_i915_gem_object {
unsigned workers :4; unsigned workers :4;
#define I915_GEM_USERPTR_MAX_WORKERS 15 #define I915_GEM_USERPTR_MAX_WORKERS 15
struct mm_struct *mm; struct i915_mm_struct *mm;
struct i915_mmu_object *mn; struct i915_mmu_object *mmu_object;
struct work_struct *work; struct work_struct *work;
} userptr; } userptr;
}; };
......
This diff is collapsed.
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment