* [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT
@ 2017-03-07 20:58 Chris Wilson
2017-03-07 20:58 ` [PATCH v2 2/3] drm/i915/userptr: Only flush the workqueue if required Chris Wilson
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Chris Wilson @ 2017-03-07 20:58 UTC (permalink / raw)
To: intel-gfx
If the worker fails, it no longer has pages to release and can be
immediately removed from the invalidate-tree.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 22b46398831e..6ef05d5b884d 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -541,6 +541,8 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
}
obj->userptr.work = ERR_CAST(pages);
+ if (IS_ERR(pages))
+ __i915_gem_userptr_set_active(obj, false);
}
mutex_unlock(&obj->mm.lock);
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 2/3] drm/i915/userptr: Only flush the workqueue if required
2017-03-07 20:58 [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Chris Wilson
@ 2017-03-07 20:58 ` Chris Wilson
2017-03-08 8:06 ` Tvrtko Ursulin
2017-03-08 9:35 ` Michał Winiarski
2017-03-07 20:58 ` [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr Chris Wilson
` (5 subsequent siblings)
6 siblings, 2 replies; 23+ messages in thread
From: Chris Wilson @ 2017-03-07 20:58 UTC (permalink / raw)
To: intel-gfx
To avoid waiting for work from other invalidate-range threads where
not required, only wait on the userptr cancel workqueue if we have added
some work to it.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 6ef05d5b884d..dc9bf5282071 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -145,7 +145,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
del_object(mo);
spin_unlock(&mn->lock);
- flush_workqueue(mn->wq);
+ if (!list_empty(&cancelled))
+ flush_workqueue(mn->wq);
}
static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-07 20:58 [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Chris Wilson
2017-03-07 20:58 ` [PATCH v2 2/3] drm/i915/userptr: Only flush the workqueue if required Chris Wilson
@ 2017-03-07 20:58 ` Chris Wilson
2017-03-08 8:25 ` Tvrtko Ursulin
` (2 more replies)
2017-03-07 21:48 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Patchwork
` (4 subsequent siblings)
6 siblings, 3 replies; 23+ messages in thread
From: Chris Wilson @ 2017-03-07 20:58 UTC (permalink / raw)
To: intel-gfx
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.
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>
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 76 ++++++++++++++++++++++-----------
1 file changed, 52 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index dc9bf5282071..7addbf08bcb9 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -488,6 +488,36 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
return ret;
}
+static bool 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_end >= end)
+ return false;
+
+ if (vma->vm_ops == gem_vm_ops) /* GTT mmapping */
+ break;
+
+ addr = vma->vm_end;
+ }
+
+ return true;
+}
+
static void
__i915_gem_userptr_get_pages_worker(struct work_struct *_work)
{
@@ -556,8 +586,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 +623,7 @@ __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;
+ __i915_gem_userptr_set_active(obj, true);
return ERR_PTR(-EAGAIN);
}
@@ -602,10 +631,10 @@ 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 +661,36 @@ 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(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);
+ pages = __i915_gem_userptr_get_pages_schedule(obj);
else
pages = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
- if (IS_ERR(pages)) {
- __i915_gem_userptr_set_active(obj, active);
+ up_read(&mm->mmap_sem);
+
+ if (IS_ERR(pages))
release_pages(pvec, pinned, 0);
- }
drm_free_large(pvec);
+
return pages;
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT
2017-03-07 20:58 [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Chris Wilson
2017-03-07 20:58 ` [PATCH v2 2/3] drm/i915/userptr: Only flush the workqueue if required Chris Wilson
2017-03-07 20:58 ` [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr Chris Wilson
@ 2017-03-07 21:48 ` Patchwork
2017-03-08 8:02 ` [PATCH v2 1/3] " Tvrtko Ursulin
` (3 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-03-07 21:48 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT
URL : https://patchwork.freedesktop.org/series/20855/
State : success
== Summary ==
Series 20855v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20855/revisions/1/mbox/
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 468s
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 615s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 539s
fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 600s
fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 509s
fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 496s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 438s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 433s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 437s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 510s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 491s
fi-kbl-7500u total:278 pass:259 dwarn:1 dfail:0 fail:0 skip:18 time: 477s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 506s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 599s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 504s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 548s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 551s
fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time: 422s
dcca4ca8923adc61254f23eb66ca18a4c9e1ffd3 drm-tip: 2017y-03m-07d-18h-17m-03s UTC integration manifest
16415ac drm/i915/userptr: Disallow wrapping GTT into a userptr
6ae9f57 drm/i915/userptr: Only flush the workqueue if required
c6bcac8 drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4090/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT
2017-03-07 20:58 [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Chris Wilson
` (2 preceding siblings ...)
2017-03-07 21:48 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Patchwork
@ 2017-03-08 8:02 ` Tvrtko Ursulin
2017-03-08 9:34 ` Michał Winiarski
` (2 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Tvrtko Ursulin @ 2017-03-08 8:02 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 07/03/2017 20:58, Chris Wilson wrote:
> If the worker fails, it no longer has pages to release and can be
> immediately removed from the invalidate-tree.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_userptr.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 22b46398831e..6ef05d5b884d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -541,6 +541,8 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
> }
>
> obj->userptr.work = ERR_CAST(pages);
> + if (IS_ERR(pages))
> + __i915_gem_userptr_set_active(obj, false);
> }
> mutex_unlock(&obj->mm.lock);
>
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/3] drm/i915/userptr: Only flush the workqueue if required
2017-03-07 20:58 ` [PATCH v2 2/3] drm/i915/userptr: Only flush the workqueue if required Chris Wilson
@ 2017-03-08 8:06 ` Tvrtko Ursulin
2017-03-08 9:35 ` Michał Winiarski
1 sibling, 0 replies; 23+ messages in thread
From: Tvrtko Ursulin @ 2017-03-08 8:06 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 07/03/2017 20:58, Chris Wilson wrote:
> To avoid waiting for work from other invalidate-range threads where
> not required, only wait on the userptr cancel workqueue if we have added
> some work to it.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_userptr.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 6ef05d5b884d..dc9bf5282071 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -145,7 +145,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> del_object(mo);
> spin_unlock(&mn->lock);
>
> - flush_workqueue(mn->wq);
> + if (!list_empty(&cancelled))
> + flush_workqueue(mn->wq);
> }
>
> static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-07 20:58 ` [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr Chris Wilson
@ 2017-03-08 8:25 ` Tvrtko Ursulin
2017-03-08 10:01 ` Chris Wilson
2017-03-08 10:33 ` [PATCH v3] " Chris Wilson
2017-03-08 21:59 ` [PATCH v4] " Chris Wilson
2 siblings, 1 reply; 23+ messages in thread
From: Tvrtko Ursulin @ 2017-03-08 8:25 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 07/03/2017 20:58, Chris Wilson wrote:
> 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.
>
> 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>
> ---
> drivers/gpu/drm/i915/i915_gem_userptr.c | 76 ++++++++++++++++++++++-----------
> 1 file changed, 52 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index dc9bf5282071..7addbf08bcb9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -488,6 +488,36 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> return ret;
> }
>
> +static bool 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 I understand this correctly it is checking for more than ggtt, so
would be tempted to either remove those parts of the checks or remove
the helper to something like invalid_backing_store.
Also, again if I understand it correctly, if the backing store was made
out of two VMAs then wouldn't this return a false positive?
In which case the first option from above sounds better to me. Just
checking if there is any overlap with the GTT mapped areas in this
function and leave the holes etc to get_user_pages.
> +
> + if (vma->vm_end >= end)
> + return false;
> +
> + if (vma->vm_ops == gem_vm_ops) /* GTT mmapping */
> + break;
> +
> + addr = vma->vm_end;
> + }
> +
> + return true;
> +}
> +
> static void
> __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
> {
> @@ -556,8 +586,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 +623,7 @@ __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;
> + __i915_gem_userptr_set_active(obj, true);
> return ERR_PTR(-EAGAIN);
> }
>
> @@ -602,10 +631,10 @@ 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 +661,36 @@ 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(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);
> + pages = __i915_gem_userptr_get_pages_schedule(obj);
> else
> pages = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
I don't see _set_active on the fast gup path, maybe it is just too early? :)
> - if (IS_ERR(pages)) {
> - __i915_gem_userptr_set_active(obj, active);
> + up_read(&mm->mmap_sem);
> +
> + if (IS_ERR(pages))
> release_pages(pvec, pinned, 0);
> - }
> drm_free_large(pvec);
> +
> return pages;
> }
>
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT
2017-03-07 20:58 [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Chris Wilson
` (3 preceding siblings ...)
2017-03-08 8:02 ` [PATCH v2 1/3] " Tvrtko Ursulin
@ 2017-03-08 9:34 ` Michał Winiarski
2017-03-08 13:17 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev2) Patchwork
2017-03-08 22:48 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev3) Patchwork
6 siblings, 0 replies; 23+ messages in thread
From: Michał Winiarski @ 2017-03-08 9:34 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Tue, Mar 07, 2017 at 08:58:49PM +0000, Chris Wilson wrote:
> If the worker fails, it no longer has pages to release and can be
> immediately removed from the invalidate-tree.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
-Michał
> ---
> drivers/gpu/drm/i915/i915_gem_userptr.c | 2 ++
> 1 file changed, 2 insertions(+)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/3] drm/i915/userptr: Only flush the workqueue if required
2017-03-07 20:58 ` [PATCH v2 2/3] drm/i915/userptr: Only flush the workqueue if required Chris Wilson
2017-03-08 8:06 ` Tvrtko Ursulin
@ 2017-03-08 9:35 ` Michał Winiarski
1 sibling, 0 replies; 23+ messages in thread
From: Michał Winiarski @ 2017-03-08 9:35 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Tue, Mar 07, 2017 at 08:58:50PM +0000, Chris Wilson wrote:
> To avoid waiting for work from other invalidate-range threads where
> not required, only wait on the userptr cancel workqueue if we have added
> some work to it.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
-Michał
> ---
> drivers/gpu/drm/i915/i915_gem_userptr.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-08 8:25 ` Tvrtko Ursulin
@ 2017-03-08 10:01 ` Chris Wilson
2017-03-08 13:04 ` Tvrtko Ursulin
0 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2017-03-08 10:01 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Wed, Mar 08, 2017 at 08:25:12AM +0000, Tvrtko Ursulin wrote:
>
> On 07/03/2017 20:58, Chris Wilson wrote:
> >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.
> >
> >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>
> >---
> > drivers/gpu/drm/i915/i915_gem_userptr.c | 76 ++++++++++++++++++++++-----------
> > 1 file changed, 52 insertions(+), 24 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >index dc9bf5282071..7addbf08bcb9 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >@@ -488,6 +488,36 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> > return ret;
> > }
> >
> >+static bool 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 I understand this correctly it is checking for more than ggtt, so
> would be tempted to either remove those parts of the checks or
> remove the helper to something like invalid_backing_store.
>
> Also, again if I understand it correctly, if the backing store was
> made out of two VMAs then wouldn't this return a false positive?
No. The vmas have to be contiguous or else the range covers an absent
page (which will EFAULT).
> In which case the first option from above sounds better to me. Just
> checking if there is any overlap with the GTT mapped areas in this
> function and leave the holes etc to get_user_pages.
It's just whilst I was here the contiguity check falls out :) Since we
also have to have the !vma check.
> >- 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);
> >+ pages = __i915_gem_userptr_get_pages_schedule(obj);
> > else
> > pages = __i915_gem_userptr_set_pages(obj, pvec, num_pages);
>
> I don't see _set_active on the fast gup path, maybe it is just too early? :)
Drat. No, I was just thinking about the worker.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-07 20:58 ` [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr Chris Wilson
2017-03-08 8:25 ` Tvrtko Ursulin
@ 2017-03-08 10:33 ` Chris Wilson
2017-03-08 13:28 ` Tvrtko Ursulin
2017-03-08 21:59 ` [PATCH v4] " Chris Wilson
2 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2017-03-08 10:33 UTC (permalink / raw)
To: intel-gfx
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.
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>
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 88 +++++++++++++++++++++++----------
1 file changed, 62 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index dc9bf5282071..2a98deed622d 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -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_end >= end)
+ return false;
+
+ if (vma->vm_ops == gem_vm_ops) /* GTT mmapping */
+ break;
+
+ 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;
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-08 10:01 ` Chris Wilson
@ 2017-03-08 13:04 ` Tvrtko Ursulin
2017-03-08 13:10 ` Chris Wilson
0 siblings, 1 reply; 23+ messages in thread
From: Tvrtko Ursulin @ 2017-03-08 13:04 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 08/03/2017 10:01, Chris Wilson wrote:
> On Wed, Mar 08, 2017 at 08:25:12AM +0000, Tvrtko Ursulin wrote:
>>
>> On 07/03/2017 20:58, Chris Wilson wrote:
>>> 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.
>>>
>>> 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>
>>> ---
>>> drivers/gpu/drm/i915/i915_gem_userptr.c | 76 ++++++++++++++++++++++-----------
>>> 1 file changed, 52 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
>>> index dc9bf5282071..7addbf08bcb9 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
>>> @@ -488,6 +488,36 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
>>> return ret;
>>> }
>>>
>>> +static bool 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 I understand this correctly it is checking for more than ggtt, so
>> would be tempted to either remove those parts of the checks or
>> remove the helper to something like invalid_backing_store.
>>
>> Also, again if I understand it correctly, if the backing store was
>> made out of two VMAs then wouldn't this return a false positive?
>
> No. The vmas have to be contiguous or else the range covers an absent
> page (which will EFAULT).
I assume you imply there cannot be two adjacent VMAs, that the core
would merge them? But I am thinking there could be two non-mergeable
ones if the backing store was different. Not 100% sure, this is just my
intuition talking. But it would still be a valid userptr object, one
half backed with anon pages, another say file backed.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-08 13:04 ` Tvrtko Ursulin
@ 2017-03-08 13:10 ` Chris Wilson
2017-03-08 13:12 ` Tvrtko Ursulin
0 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2017-03-08 13:10 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Wed, Mar 08, 2017 at 01:04:29PM +0000, Tvrtko Ursulin wrote:
>
> On 08/03/2017 10:01, Chris Wilson wrote:
> >On Wed, Mar 08, 2017 at 08:25:12AM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 07/03/2017 20:58, Chris Wilson wrote:
> >>>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.
> >>>
> >>>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>
> >>>---
> >>>drivers/gpu/drm/i915/i915_gem_userptr.c | 76 ++++++++++++++++++++++-----------
> >>>1 file changed, 52 insertions(+), 24 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >>>index dc9bf5282071..7addbf08bcb9 100644
> >>>--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> >>>+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >>>@@ -488,6 +488,36 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> >>> return ret;
> >>>}
> >>>
> >>>+static bool 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 I understand this correctly it is checking for more than ggtt, so
> >>would be tempted to either remove those parts of the checks or
> >>remove the helper to something like invalid_backing_store.
> >>
> >>Also, again if I understand it correctly, if the backing store was
> >>made out of two VMAs then wouldn't this return a false positive?
> >
> >No. The vmas have to be contiguous or else the range covers an absent
> >page (which will EFAULT).
>
> I assume you imply there cannot be two adjacent VMAs, that the core
> would merge them? But I am thinking there could be two non-mergeable
> ones if the backing store was different. Not 100% sure, this is just
> my intuition talking. But it would still be a valid userptr object,
> one half backed with anon pages, another say file backed.
There can be two adjacent vmas: vma->vm_next->vm_start == vma->vm_end.
There can't be overlapping ones.
We are testing whether vma->vm_next->vm_start > vma->vm_end which
implies a hole, for which there can be no struct pages.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-08 13:10 ` Chris Wilson
@ 2017-03-08 13:12 ` Tvrtko Ursulin
0 siblings, 0 replies; 23+ messages in thread
From: Tvrtko Ursulin @ 2017-03-08 13:12 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 08/03/2017 13:10, Chris Wilson wrote:
> On Wed, Mar 08, 2017 at 01:04:29PM +0000, Tvrtko Ursulin wrote:
>>
>> On 08/03/2017 10:01, Chris Wilson wrote:
>>> On Wed, Mar 08, 2017 at 08:25:12AM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 07/03/2017 20:58, Chris Wilson wrote:
>>>>> 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.
>>>>>
>>>>> 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>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_gem_userptr.c | 76 ++++++++++++++++++++++-----------
>>>>> 1 file changed, 52 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
>>>>> index dc9bf5282071..7addbf08bcb9 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
>>>>> @@ -488,6 +488,36 @@ __i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
>>>>> return ret;
>>>>> }
>>>>>
>>>>> +static bool 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 I understand this correctly it is checking for more than ggtt, so
>>>> would be tempted to either remove those parts of the checks or
>>>> remove the helper to something like invalid_backing_store.
>>>>
>>>> Also, again if I understand it correctly, if the backing store was
>>>> made out of two VMAs then wouldn't this return a false positive?
>>>
>>> No. The vmas have to be contiguous or else the range covers an absent
>>> page (which will EFAULT).
>>
>> I assume you imply there cannot be two adjacent VMAs, that the core
>> would merge them? But I am thinking there could be two non-mergeable
>> ones if the backing store was different. Not 100% sure, this is just
>> my intuition talking. But it would still be a valid userptr object,
>> one half backed with anon pages, another say file backed.
>
> There can be two adjacent vmas: vma->vm_next->vm_start == vma->vm_end.
> There can't be overlapping ones.
>
> We are testing whether vma->vm_next->vm_start > vma->vm_end which
> implies a hole, for which there can be no struct pages.
Bah sorry, I'm jumping between things and did not register the addr
assignment at the end of the loop.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev2)
2017-03-07 20:58 [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Chris Wilson
` (4 preceding siblings ...)
2017-03-08 9:34 ` Michał Winiarski
@ 2017-03-08 13:17 ` Patchwork
2017-03-08 22:48 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev3) Patchwork
6 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-03-08 13:17 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev2)
URL : https://patchwork.freedesktop.org/series/20855/
State : success
== Summary ==
Series 20855v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20855/revisions/2/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
fail -> PASS (fi-snb-2600) fdo#100007
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 458s
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 614s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 532s
fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 600s
fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 506s
fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 509s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 444s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 433s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 437s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 521s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 492s
fi-kbl-7500u total:278 pass:259 dwarn:1 dfail:0 fail:0 skip:18 time: 484s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 499s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 600s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 495s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 552s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 551s
fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 421s
035f22af3e974f803031dc9f20e0969d5ae0a433 drm-tip: 2017y-03m-08d-11h-24m-39s UTC integration manifest
6d02a25 drm/i915/userptr: Disallow wrapping GTT into a userptr
f11e429 drm/i915/userptr: Only flush the workqueue if required
280ebd0 drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4093/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-08 10:33 ` [PATCH v3] " Chris Wilson
@ 2017-03-08 13:28 ` Tvrtko Ursulin
2017-03-08 13:46 ` Chris Wilson
0 siblings, 1 reply; 23+ messages in thread
From: Tvrtko Ursulin @ 2017-03-08 13:28 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 08/03/2017 10:33, Chris Wilson wrote:
> 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.
>
> 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>
> ---
> drivers/gpu/drm/i915/i915_gem_userptr.c | 88 +++++++++++++++++++++++----------
> 1 file changed, 62 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index dc9bf5282071..2a98deed622d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -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_end >= end)
> + return false;
Okay risking more embarrassing misses, but is this not a false negative?
Userptr created from an equal or smaller than the GTT mapping would not
trigger this return false for some reason?
> +
> + if (vma->vm_ops == gem_vm_ops) /* GTT mmapping */
> + break;
> +
> + 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);
Hm, now that this is under the mmap_sem could we afford a more thorough
flavour of gup and lessen the likelihood of needing a worker?
> }
>
> 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;
> }
>
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-08 13:28 ` Tvrtko Ursulin
@ 2017-03-08 13:46 ` Chris Wilson
2017-03-08 17:25 ` Chris Wilson
0 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2017-03-08 13:46 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Wed, Mar 08, 2017 at 01:28:31PM +0000, Tvrtko Ursulin wrote:
>
> On 08/03/2017 10:33, Chris Wilson wrote:
> >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.
> >
> >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>
> >---
> > drivers/gpu/drm/i915/i915_gem_userptr.c | 88 +++++++++++++++++++++++----------
> > 1 file changed, 62 insertions(+), 26 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >index dc9bf5282071..2a98deed622d 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> >@@ -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_end >= end)
> >+ return false;
>
> Okay risking more embarrassing misses, but is this not a false
> negative? Userptr created from an equal or smaller than the GTT
> mapping would not trigger this return false for some reason?
I was thinking about the case where the vma is far to the right of addr,
where it started past end. However, that is now protected by checking
vm_start > addr for the first vma. I didn't want a false positive, but
yeah this should now be a false negative for a single GTT vma spanning
the userptr. Weird that gem_userptr_blits didn't catch that case :|
> >+ 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);
>
> Hm, now that this is under the mmap_sem could we afford a more
> thorough flavour of gup and lessen the likelihood of needing a
> worker?
I'm planning to go the other way, and force anything that isn't uberfast
to use a worker and signal a completion (then use that to queue the
request submission -- well following a few more tricks to do async GTT
binding etc). The goal being to reduce lock hold times.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-08 13:46 ` Chris Wilson
@ 2017-03-08 17:25 ` Chris Wilson
2017-03-08 21:58 ` Chris Wilson
0 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2017-03-08 17:25 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
On Wed, Mar 08, 2017 at 01:46:35PM +0000, Chris Wilson wrote:
> On Wed, Mar 08, 2017 at 01:28:31PM +0000, Tvrtko Ursulin wrote:
> > Okay risking more embarrassing misses, but is this not a false
> > negative? Userptr created from an equal or smaller than the GTT
> > mapping would not trigger this return false for some reason?
>
> I was thinking about the case where the vma is far to the right of addr,
> where it started past end. However, that is now protected by checking
> vm_start > addr for the first vma. I didn't want a false positive, but
> yeah this should now be a false negative for a single GTT vma spanning
> the userptr. Weird that gem_userptr_blits didn't catch that case :|
Ah, added the boundary tests to gem_userptr_blits/invalid-gtt-mapping
and by printk confirmed hitting the bug you pointed out. However, the
igt still passes because we defer to the worker and that generates the
EFAULT. Catching it as a real error is proving difficult, needs Michal's
machinery on top of the overlap of neighbouring vma.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-08 17:25 ` Chris Wilson
@ 2017-03-08 21:58 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2017-03-08 21:58 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
On Wed, Mar 08, 2017 at 05:25:18PM +0000, Chris Wilson wrote:
> On Wed, Mar 08, 2017 at 01:46:35PM +0000, Chris Wilson wrote:
> > On Wed, Mar 08, 2017 at 01:28:31PM +0000, Tvrtko Ursulin wrote:
> > > Okay risking more embarrassing misses, but is this not a false
> > > negative? Userptr created from an equal or smaller than the GTT
> > > mapping would not trigger this return false for some reason?
> >
> > I was thinking about the case where the vma is far to the right of addr,
> > where it started past end. However, that is now protected by checking
> > vm_start > addr for the first vma. I didn't want a false positive, but
> > yeah this should now be a false negative for a single GTT vma spanning
> > the userptr. Weird that gem_userptr_blits didn't catch that case :|
>
> Ah, added the boundary tests to gem_userptr_blits/invalid-gtt-mapping
> and by printk confirmed hitting the bug you pointed out. However, the
> igt still passes because we defer to the worker and that generates the
> EFAULT. Catching it as a real error is proving difficult, needs Michal's
> machinery on top of the overlap of neighbouring vma.
Done. I can provoke the deadlock by adding a few more boundary cases to
Michal's tests.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-07 20:58 ` [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr Chris Wilson
2017-03-08 8:25 ` Tvrtko Ursulin
2017-03-08 10:33 ` [PATCH v3] " Chris Wilson
@ 2017-03-08 21:59 ` Chris Wilson
2017-03-09 6:55 ` Tvrtko Ursulin
2 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2017-03-08 21:59 UTC (permalink / raw)
To: intel-gfx
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>
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 88 +++++++++++++++++++++++----------
1 file changed, 62 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index dc9bf5282071..07046fa4c6a9 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -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;
}
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev3)
2017-03-07 20:58 [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Chris Wilson
` (5 preceding siblings ...)
2017-03-08 13:17 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev2) Patchwork
@ 2017-03-08 22:48 ` Patchwork
6 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2017-03-08 22:48 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev3)
URL : https://patchwork.freedesktop.org/series/20855/
State : success
== Summary ==
Series 20855v3 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20855/revisions/3/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
incomplete -> PASS (fi-skl-6770hq)
Test gem_exec_suspend:
Subgroup basic-s4-devices:
pass -> DMESG-WARN (fi-bxt-t5700) fdo#100125
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 466s
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 621s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 541s
fi-bxt-t5700 total:278 pass:257 dwarn:1 dfail:0 fail:0 skip:20 time: 610s
fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 506s
fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 506s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 435s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 439s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 441s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 503s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 488s
fi-kbl-7500u total:278 pass:259 dwarn:1 dfail:0 fail:0 skip:18 time: 485s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 509s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 595s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 502s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 551s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 550s
fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time: 418s
79e440ca583b2678e540d616d87cc6f250368be9 drm-tip: 2017y-03m-08d-20h-49m-20s UTC integration manifest
937ef6c drm/i915/userptr: Disallow wrapping GTT into a userptr
5df3704 drm/i915/userptr: Only flush the workqueue if required
0ac0652 drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4106/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-08 21:59 ` [PATCH v4] " Chris Wilson
@ 2017-03-09 6:55 ` Tvrtko Ursulin
2017-03-09 7:39 ` Chris Wilson
0 siblings, 1 reply; 23+ messages in thread
From: Tvrtko Ursulin @ 2017-03-09 6:55 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 08/03/2017 21:59, Chris Wilson wrote:
> 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>
> ---
> drivers/gpu/drm/i915/i915_gem_userptr.c | 88 +++++++++++++++++++++++----------
> 1 file changed, 62 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index dc9bf5282071..07046fa4c6a9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -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;
> }
>
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4] drm/i915/userptr: Disallow wrapping GTT into a userptr
2017-03-09 6:55 ` Tvrtko Ursulin
@ 2017-03-09 7:39 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2017-03-09 7:39 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Thu, Mar 09, 2017 at 06:55:02AM +0000, Tvrtko Ursulin wrote:
>
> On 08/03/2017 21:59, Chris Wilson wrote:
> >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>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Thanks for catchin this and providing a testcase that proved Tvrtko was
right. Pushed,
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2017-03-09 7:39 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-07 20:58 [PATCH v2 1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Chris Wilson
2017-03-07 20:58 ` [PATCH v2 2/3] drm/i915/userptr: Only flush the workqueue if required Chris Wilson
2017-03-08 8:06 ` Tvrtko Ursulin
2017-03-08 9:35 ` Michał Winiarski
2017-03-07 20:58 ` [PATCH v2 3/3] drm/i915/userptr: Disallow wrapping GTT into a userptr Chris Wilson
2017-03-08 8:25 ` Tvrtko Ursulin
2017-03-08 10:01 ` Chris Wilson
2017-03-08 13:04 ` Tvrtko Ursulin
2017-03-08 13:10 ` Chris Wilson
2017-03-08 13:12 ` Tvrtko Ursulin
2017-03-08 10:33 ` [PATCH v3] " Chris Wilson
2017-03-08 13:28 ` Tvrtko Ursulin
2017-03-08 13:46 ` Chris Wilson
2017-03-08 17:25 ` Chris Wilson
2017-03-08 21:58 ` Chris Wilson
2017-03-08 21:59 ` [PATCH v4] " Chris Wilson
2017-03-09 6:55 ` Tvrtko Ursulin
2017-03-09 7:39 ` Chris Wilson
2017-03-07 21:48 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT Patchwork
2017-03-08 8:02 ` [PATCH v2 1/3] " Tvrtko Ursulin
2017-03-08 9:34 ` Michał Winiarski
2017-03-08 13:17 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev2) Patchwork
2017-03-08 22:48 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/userptr: Deactivate a failed userptr if the worker reports an EFAULT (rev3) Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.