drm/i915/userptr: Disallow wrapping GTT into a userptr
authorChris Wilson <chris@chris-wilson.co.uk>
Wed, 8 Mar 2017 21:59:03 +0000 (21:59 +0000)
committerChris Wilson <chris@chris-wilson.co.uk>
Thu, 9 Mar 2017 07:31:14 +0000 (07:31 +0000)
If we allow the user to convert a GTT mmap address into a userptr, we
may end up in recursion hell, where currently we hit a mutex deadlock
but other possibilities include use-after-free during the
unbind/cancel_userptr.

[  143.203989] gem_userptr_bli D    0   902    898 0x00000000
[  143.204054] Call Trace:
[  143.204137]  __schedule+0x511/0x1180
[  143.204195]  ? pci_mmcfg_check_reserved+0xc0/0xc0
[  143.204274]  schedule+0x57/0xe0
[  143.204327]  schedule_timeout+0x383/0x670
[  143.204374]  ? trace_hardirqs_on_caller+0x187/0x280
[  143.204457]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[  143.204507]  ? usleep_range+0x110/0x110
[  143.204657]  ? irq_exit+0x89/0x100
[  143.204710]  ? retint_kernel+0x2d/0x2d
[  143.204794]  ? trace_hardirqs_on_caller+0x187/0x280
[  143.204857]  ? _raw_spin_unlock_irq+0x33/0x60
[  143.204944]  wait_for_common+0x1f0/0x2f0
[  143.205006]  ? out_of_line_wait_on_atomic_t+0x170/0x170
[  143.205103]  ? wake_up_q+0xa0/0xa0
[  143.205159]  ? flush_workqueue_prep_pwqs+0x15a/0x2c0
[  143.205237]  wait_for_completion+0x1d/0x20
[  143.205292]  flush_workqueue+0x2e9/0xbb0
[  143.205339]  ? flush_workqueue+0x163/0xbb0
[  143.205418]  ? __schedule+0x533/0x1180
[  143.205498]  ? check_flush_dependency+0x1a0/0x1a0
[  143.205681]  i915_gem_userptr_mn_invalidate_range_start+0x1c7/0x270 [i915]
[  143.205865]  ? i915_gem_userptr_dmabuf_export+0x40/0x40 [i915]
[  143.205955]  __mmu_notifier_invalidate_range_start+0xc6/0x120
[  143.206044]  ? __mmu_notifier_invalidate_range_start+0x51/0x120
[  143.206123]  zap_page_range_single+0x1c7/0x1f0
[  143.206171]  ? unmap_single_vma+0x160/0x160
[  143.206260]  ? unmap_mapping_range+0xa9/0x1b0
[  143.206308]  ? vma_interval_tree_subtree_search+0x75/0xd0
[  143.206397]  unmap_mapping_range+0x18f/0x1b0
[  143.206444]  ? zap_vma_ptes+0x70/0x70
[  143.206524]  ? __pm_runtime_resume+0x67/0xa0
[  143.206723]  i915_gem_release_mmap+0x1ba/0x1c0 [i915]
[  143.206846]  i915_vma_unbind+0x5c2/0x690 [i915]
[  143.206925]  ? __lock_is_held+0x52/0x100
[  143.207076]  i915_gem_object_set_tiling+0x1db/0x650 [i915]
[  143.207236]  i915_gem_set_tiling_ioctl+0x1d3/0x3b0 [i915]
[  143.207377]  ? i915_gem_set_tiling_ioctl+0x5/0x3b0 [i915]
[  143.207457]  drm_ioctl+0x36c/0x670
[  143.207535]  ? debug_lockdep_rcu_enabled.part.0+0x1a/0x30
[  143.207730]  ? i915_gem_object_set_tiling+0x650/0x650 [i915]
[  143.207793]  ? drm_getunique+0x120/0x120
[  143.207875]  ? __handle_mm_fault+0x996/0x14a0
[  143.207939]  ? vm_insert_page+0x340/0x340
[  143.208028]  ? up_write+0x28/0x50
[  143.208086]  ? vm_mmap_pgoff+0x160/0x190
[  143.208163]  do_vfs_ioctl+0x12c/0xa60
[  143.208218]  ? debug_lockdep_rcu_enabled+0x35/0x40
[  143.208267]  ? ioctl_preallocate+0x150/0x150
[  143.208353]  ? __do_page_fault+0x36a/0x6e0
[  143.208400]  ? mark_held_locks+0x23/0xc0
[  143.208479]  ? up_read+0x1f/0x40
[  143.208526]  ? entry_SYSCALL_64_fastpath+0x5/0xc6
[  143.208669]  ? __fget_light+0xa7/0xc0
[  143.208747]  SyS_ioctl+0x41/0x70

To prevent the possibility of a deadlock, we defer scheduling the worker
until after we have proven that given the current mm, the userptr range
does not overlap a GGTT mmaping. If another thread tries to remap the
GGTT over the userptr before the worker is scheduled, it will be stopped
by its invalidate-range flushing the current work, before the deadlock
can occur.

v2: Improve discussion of how we end up in the deadlock.
v3: Don't forget to mark the userptr as active after a successful
gup_fast. Rename overlaps_ggtt to noncontiguous_or_overlaps_ggtt.
v4: Fix test ordering between invalid GTT mmaping and range completion
(Tvrtko)

Reported-by: Michał Winiarski <michal.winiarski@intel.com>
Testcase: igt/gem_userptr_blits/map-fixed-invalidate-gup
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20170308215903.24171-1-chris@chris-wilson.co.uk
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
drivers/gpu/drm/i915/i915_gem_userptr.c

index dc9bf52..07046fa 100644 (file)
@@ -488,6 +488,37 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
        return ret;
 }
 
+static bool noncontiguous_or_overlaps_ggtt(struct drm_i915_gem_object *obj,
+                                          struct mm_struct *mm)
+{
+       const struct vm_operations_struct *gem_vm_ops =
+               obj->base.dev->driver->gem_vm_ops;
+       unsigned long addr = obj->userptr.ptr;
+       const unsigned long end = addr + obj->base.size;
+       struct vm_area_struct *vma;
+
+       /* Check for a contiguous set of vma covering the userptr, if any
+        * are absent, they will EFAULT. More importantly if any point back
+        * to a drm_i915_gem_object GTT mmaping, we may trigger a deadlock
+        * between the deferred gup of this userptr and the object being
+        * unbound calling invalidate_range -> cancel_userptr.
+        */
+       for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
+               if (vma->vm_start > addr) /* gap */
+                       break;
+
+               if (vma->vm_ops == gem_vm_ops) /* GTT mmapping */
+                       break;
+
+               if (vma->vm_end >= end)
+                       return false;
+
+               addr = vma->vm_end;
+       }
+
+       return true;
+}
+
 static void
 __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 {
@@ -556,8 +587,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 }
 
 static struct sg_table *
-__i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj,
-                                     bool *active)
+__i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
 {
        struct get_pages_work *work;
 
@@ -594,7 +624,6 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj,
        INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker);
        schedule_work(&work->work);
 
-       *active = true;
        return ERR_PTR(-EAGAIN);
 }
 
@@ -602,10 +631,11 @@ static struct sg_table *
 i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 {
        const int num_pages = obj->base.size >> PAGE_SHIFT;
+       struct mm_struct *mm = obj->userptr.mm->mm;
        struct page **pvec;
        struct sg_table *pages;
-       int pinned, ret;
        bool active;
+       int pinned;
 
        /* If userspace should engineer that these pages are replaced in
         * the vma between us binding this page into the GTT and completion
@@ -632,37 +662,43 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
                        return ERR_PTR(-EAGAIN);
        }
 
-       /* Let the mmu-notifier know that we have begun and need cancellation */
-       ret = __i915_gem_userptr_set_active(obj, true);
-       if (ret)
-               return ERR_PTR(ret);
-
        pvec = NULL;
        pinned = 0;
-       if (obj->userptr.mm->mm == current->mm) {
-               pvec = drm_malloc_gfp(num_pages, sizeof(struct page *),
-                                     GFP_TEMPORARY);
-               if (pvec == NULL) {
-                       __i915_gem_userptr_set_active(obj, false);
-                       return ERR_PTR(-ENOMEM);
-               }
 
-               pinned = __get_user_pages_fast(obj->userptr.ptr, num_pages,
-                                              !obj->userptr.read_only, pvec);
+       down_read(&mm->mmap_sem);
+       if (unlikely(noncontiguous_or_overlaps_ggtt(obj, mm))) {
+               pinned = -EFAULT;
+       } else if (mm == current->mm) {
+               pvec = drm_malloc_gfp(num_pages, sizeof(struct page *),
+                                     GFP_TEMPORARY |
+                                     __GFP_NORETRY |
+                                     __GFP_NOWARN);
+               if (pvec) /* defer to worker if malloc fails */
+                       pinned = __get_user_pages_fast(obj->userptr.ptr,
+                                                      num_pages,
+                                                      !obj->userptr.read_only,
+                                                      pvec);
        }
 
        active = false;
-       if (pinned < 0)
-               pages = ERR_PTR(pinned), pinned = 0;
-       else if (pinned < num_pages)
-               pages = __i915_gem_userptr_get_pages_schedule(obj, &active);
-       else
+       if (pinned < 0) {
+               pages = ERR_PTR(pinned);
+               pinned = 0;
+       } else if (pinned < num_pages) {
+               pages = __i915_gem_userptr_get_pages_schedule(obj);
+               active = pages == ERR_PTR(-EAGAIN);
+       } else {
                pages = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
-       if (IS_ERR(pages)) {
-               __i915_gem_userptr_set_active(obj, active);
-               release_pages(pvec, pinned, 0);
+               active = !IS_ERR(pages);
        }
+       if (active)
+               __i915_gem_userptr_set_active(obj, true);
+       up_read(&mm->mmap_sem);
+
+       if (IS_ERR(pages))
+               release_pages(pvec, pinned, 0);
        drm_free_large(pvec);
+
        return pages;
 }