From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com ([192.55.52.115]:35027 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750923AbbIJKZ3 (ORCPT ); Thu, 10 Sep 2015 06:25:29 -0400 Subject: Re: [Intel-gfx] [PATCH v4 2/3] drm/i915: Fix userptr deadlock with aliased GTT mmappings To: Chris Wilson , intel-gfx@lists.freedesktop.org References: <1441878715-24948-1-git-send-email-chris@chris-wilson.co.uk> <1441878715-24948-2-git-send-email-chris@chris-wilson.co.uk> Cc: stable@vger.kernel.org From: Tvrtko Ursulin Message-ID: <55F15A96.1080008@linux.intel.com> Date: Thu, 10 Sep 2015 11:25:26 +0100 MIME-Version: 1.0 In-Reply-To: <1441878715-24948-2-git-send-email-chris@chris-wilson.co.uk> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: On 09/10/2015 10:51 AM, Chris Wilson wrote: > Michał Winiarski found a really evil way to trigger a struct_mutex > deadlock with userptr. He found that if he allocated a userptr bo and > then GTT mmaped another bo, or even itself, at the same address as the > userptr using MAP_FIXED, he could then cause a deadlock any time we then > had to invalidate the GTT mmappings (so at will). Tvrtko then found by > repeatedly allocating GTT mmappings he could alias with an old userptr > mmap and also trigger the deadlock. > > To counter act the deadlock, we make the observation that we only need > to take the struct_mutex if the object has any pages to revoke, and that > before userspace can alias with the userptr address space, it must have > invalidated the userptr->pages. Thus if we can check for those pages > outside of the struct_mutex, we can avoid the deadlock. To do so we > introduce a separate flag for userptr objects that we can inspect from > the mmu-notifier underneath its spinlock. > > The patch makes one eye-catching change. That is the removal serial=0 > after detecting a to-be-freed object inside the invalidate walker. I > felt setting serial=0 was a questionable pessimisation: it denies us the > chance to reuse the current iterator for the next loop (before it is > freed) and being explicit makes the reader question the validity of the > locking (since the object-free race could occur elsewhere). The > serialisation of the iterator is through the spinlock, if the object is > freed before the next loop then the notifier.serial will be incremented > and we start the walk from the beginning as we detect the invalid cache. > > To try and tame the error paths and interactions with the userptr->active > flag, we have to do a fair amount of rearranging of get_pages_userptr(). > > v2: Grammar fixes > v3: Reorder set-active so that it is only set when obj->pages is set > (and so needs cancellation). Only the order of setting obj->pages and > the active-flag is crucial. Calling gup after invalidate-range begin > means the userptr sees the new set of backing storage (and so will not > need to invalidate its new pages), but we have to be careful not to set > the active-flag prior to successfully establishing obj->pages. > v4: Take the active->flag early so we know in the mmu-notifier when we > have to cancel a pending gup-worker. > v5: Rearrange the error path so that is not so convoluted > > Reported-by: Michał Winiarski > Testcase: igt/gem_userptr_blits/map-fixed* > Signed-off-by: Chris Wilson > Cc: Michał Winiarski > Cc: Tvrtko Ursulin > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 175 ++++++++++++++++++++------------ > 1 file changed, 109 insertions(+), 66 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c > index 800a5394aa1e..505c608ade7e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c > @@ -59,6 +59,7 @@ struct i915_mmu_object { > struct interval_tree_node it; > struct list_head link; > struct drm_i915_gem_object *obj; > + bool active; > bool is_linear; > }; > > @@ -114,7 +115,8 @@ restart: > > obj = mo->obj; > > - if (!kref_get_unless_zero(&obj->base.refcount)) > + if (!mo->active || > + !kref_get_unless_zero(&obj->base.refcount)) > continue; > > spin_unlock(&mn->lock); > @@ -151,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, > else > it = interval_tree_iter_first(&mn->objects, start, end); > if (it != NULL) { > - obj = container_of(it, struct i915_mmu_object, it)->obj; > + struct i915_mmu_object *mo = > + container_of(it, struct i915_mmu_object, it); > > /* The mmu_object is released late when destroying the > * GEM object so it is entirely possible to gain a > @@ -160,11 +163,9 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, > * the struct_mutex - and consequently use it after it > * is freed and then double free it. > */ > - if (!kref_get_unless_zero(&obj->base.refcount)) { > - spin_unlock(&mn->lock); > - serial = 0; > - continue; > - } > + if (mo->active && > + kref_get_unless_zero(&mo->obj->base.refcount)) > + obj = mo->obj; > > serial = mn->serial; > } > @@ -566,6 +567,30 @@ __i915_gem_userptr_set_pages(struct drm_i915_gem_object *obj, > } > > static void > +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, > + bool value) > +{ > + /* During mm_invalidate_range we need to cancel any userptr that > + * overlaps the range being invalidated. Doing so requires the > + * struct_mutex, and that risks recursion. In order to cause > + * recursion, the user must alias the userptr address space with > + * a GTT mmapping (possible with a MAP_FIXED) - then when we have > + * to invalidate that mmaping, mm_invalidate_range is called with > + * the userptr address *and* the struct_mutex held. To prevent that > + * we set a flag under the i915_mmu_notifier spinlock to indicate > + * whether this object is valid. > + */ > +#if defined(CONFIG_MMU_NOTIFIER) > + if (obj->userptr.mmu_object == NULL) > + return; > + > + spin_lock(&obj->userptr.mmu_object->mn->lock); > + obj->userptr.mmu_object->active = value; > + spin_unlock(&obj->userptr.mmu_object->mn->lock); > +#endif > +} > + > +static void > __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > { > struct get_pages_work *work = container_of(_work, typeof(*work), work); > @@ -613,6 +638,8 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > } > } > obj->userptr.work = ERR_PTR(ret); > + if (ret) > + __i915_gem_userptr_set_active(obj, false); > } > > obj->userptr.workers--; > @@ -627,11 +654,60 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > } > > static int > +__i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj, > + bool *active) > +{ > + 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_sem, and we have > + * a strict lock ordering of mmap_sem, 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. > + */ > + if (obj->userptr.workers >= I915_GEM_USERPTR_MAX_WORKERS) > + return -EAGAIN; > + > + work = kmalloc(sizeof(*work), GFP_KERNEL); > + if (work == NULL) > + return -ENOMEM; > + > + obj->userptr.work = &work->work; > + obj->userptr.workers++; > + > + work->obj = obj; > + drm_gem_object_reference(&obj->base); > + > + work->task = current; > + get_task_struct(work->task); > + > + INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker); > + schedule_work(&work->work); > + > + *active = true; > + return -EAGAIN; > +} > + > +static int > i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) > { > const int num_pages = obj->base.size >> PAGE_SHIFT; > struct page **pvec; > int pinned, ret; > + bool active; > > /* If userspace should engineer that these pages are replaced in > * the vma between us binding this page into the GTT and completion > @@ -649,6 +725,18 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) > * to the vma (discard or cloning) which should prevent the more > * egregious cases from causing harm. > */ > + if (IS_ERR(obj->userptr.work)) { > + /* active flag will have been dropped already by the worker */ > + ret = PTR_ERR(obj->userptr.work); > + obj->userptr.work = NULL; > + return ret; > + } > + if (obj->userptr.work) > + /* active flag should still be held for the pending work */ > + return -EAGAIN; > + > + /* Let the mmu-notifier know that we have begun and need cancellation */ > + __i915_gem_userptr_set_active(obj, true); > > pvec = NULL; > pinned = 0; > @@ -657,73 +745,27 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) > GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY); > if (pvec == NULL) { > pvec = drm_malloc_ab(num_pages, sizeof(struct page *)); > - if (pvec == NULL) > + if (pvec == NULL) { > + __i915_gem_userptr_set_active(obj, false); > return -ENOMEM; > + } > } > > pinned = __get_user_pages_fast(obj->userptr.ptr, num_pages, > !obj->userptr.read_only, pvec); > } > - if (pinned < num_pages) { > - if (pinned < 0) { > - ret = pinned; > - pinned = 0; > - } else { > - /* Spawn a worker so that we can acquire the > - * user pages without holding our mutex. Access > - * to the user pages requires mmap_sem, and we have > - * a strict lock ordering of mmap_sem, 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. > - */ > - ret = -EAGAIN; > - if (obj->userptr.work == NULL && > - obj->userptr.workers < I915_GEM_USERPTR_MAX_WORKERS) { > - struct get_pages_work *work; > - > - work = kmalloc(sizeof(*work), GFP_KERNEL); > - if (work != NULL) { > - obj->userptr.work = &work->work; > - obj->userptr.workers++; > - > - work->obj = obj; > - drm_gem_object_reference(&obj->base); > - > - work->task = current; > - get_task_struct(work->task); > - > - INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker); > - schedule_work(&work->work); > - } else > - ret = -ENOMEM; > - } else { > - if (IS_ERR(obj->userptr.work)) { > - ret = PTR_ERR(obj->userptr.work); > - obj->userptr.work = NULL; > - } > - } > - } > - } else { > + > + active = false; > + if (pinned < 0) > + ret = pinned; > + else if (pinned < num_pages) > + ret = __i915_gem_userptr_get_pages_schedule(obj, &active); > + else > ret = __i915_gem_userptr_set_pages(obj, pvec, num_pages); > - if (ret == 0) { > - obj->userptr.work = NULL; > - pinned = 0; > - } > + if (ret) { > + __i915_gem_userptr_set_active(obj, active); > + release_pages(pvec, pinned, 0); I think it would be safer to set pinned to zero in the pinned < 0 case, rather than rely on release_pages implementation knowledge. Otherwise it is too fragile. If you can put that back you can put my R-B on the patch. Regards, Tvrtko