If the user replaces their vma as we are looking up their pages in the pin_user_pages fast path, we miss the revocation as the mmu-notifier is not yet installed. We end up with a bunch of stale pages mixed in with the fresh. However, the slow path is always run in the worker after first attaching to the notifier, and so will not miss a revocation as the pages are being looked up. Since this is safer, despite it hitting range-invalidate caused by the pin_user_pages themselves, always take the slow path.
Signed-off-by: Chris Wilson <[email protected]> --- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 114 ++++---------------- 1 file changed, 21 insertions(+), 93 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index e946032b13e4..9d4e69825987 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -512,58 +512,13 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) kfree(work); } -static struct sg_table * -__i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj) -{ - struct get_pages_work *work; - - /* Spawn a worker so that we can acquire the - * user pages without holding our mutex. Access - * to the user pages requires mmap_lock, and we have - * a strict lock ordering of mmap_lock, struct_mutex - - * we already hold struct_mutex here and so cannot - * call gup without encountering a lock inversion. - * - * Userspace will keep on repeating the operation - * (thanks to EAGAIN) until either we hit the fast - * path or the worker completes. If the worker is - * cancelled or superseded, the task is still run - * but the results ignored. (This leads to - * complications that we may have a stray object - * refcount that we need to be wary of when - * checking for existing objects during creation.) - * If the worker encounters an error, it reports - * that error back to this function through - * obj->userptr.work = ERR_PTR. - */ - work = kmalloc(sizeof(*work), GFP_KERNEL); - if (work == NULL) - return ERR_PTR(-ENOMEM); - - obj->userptr.work = &work->work; - - work->obj = i915_gem_object_get(obj); - - work->task = current; - get_task_struct(work->task); - - INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker); - queue_work(to_i915(obj->base.dev)->mm.userptr_wq, &work->work); - - return ERR_PTR(-EAGAIN); -} - static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) { - const unsigned long num_pages = obj->base.size >> PAGE_SHIFT; - struct mm_struct *mm = obj->userptr.mm->mm; - struct page **pvec; - struct sg_table *pages; - bool active; - int pinned; - unsigned int gup_flags = 0; + struct get_pages_work *work; + struct work_struct *prev; - /* If userspace should engineer that these pages are replaced in + /* + * If userspace should engineer that these pages are replaced in * the vma between us binding this page into the GTT and completion * of rendering... Their loss. If they change the mapping of their * pages they need to create a new bo to point to the new vma. @@ -580,59 +535,32 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) * egregious cases from causing harm. */ - if (obj->userptr.work) { + prev = READ_ONCE(obj->userptr.work); + if (prev) { /* active flag should still be held for the pending work */ - if (IS_ERR(obj->userptr.work)) - return PTR_ERR(obj->userptr.work); + if (IS_ERR(prev)) + return PTR_ERR(prev); else return -EAGAIN; } - pvec = NULL; - pinned = 0; + work = kmalloc(sizeof(*work), GFP_KERNEL); + if (work == NULL) + return -ENOMEM; - if (mm == current->mm) { - pvec = kvmalloc_array(num_pages, sizeof(struct page *), - GFP_KERNEL | - __GFP_NORETRY | - __GFP_NOWARN); - /* - * Using __get_user_pages_fast() with a read-only - * access is questionable. A read-only page may be - * COW-broken, and then this might end up giving - * the wrong side of the COW.. - * - * We may or may not care. - */ - if (pvec) { - /* defer to worker if malloc fails */ - if (!i915_gem_object_is_readonly(obj)) - gup_flags |= FOLL_WRITE; - pinned = pin_user_pages_fast_only(obj->userptr.ptr, - num_pages, gup_flags, - pvec); - } - } + /* Attach our mmu-notifier first to catch revocations as we work */ + __i915_gem_userptr_set_active(obj, true); - 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 == ERR_PTR(-EAGAIN); - } else { - pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages); - active = !IS_ERR(pages); - } - if (active) - __i915_gem_userptr_set_active(obj, true); + obj->userptr.work = &work->work; + work->obj = i915_gem_object_get(obj); - if (IS_ERR(pages)) - unpin_user_pages(pvec, pinned); - kvfree(pvec); + work->task = current; + get_task_struct(work->task); - return PTR_ERR_OR_ZERO(pages); + INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker); + queue_work(to_i915(obj->base.dev)->mm.userptr_wq, &work->work); + + return 0; } static void -- 2.20.1 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
