On Mon, Jun 29, 2015 at 12:17:33PM +0100, 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). > > To counter act the deadlock, we make the observation that when the > MAP_FIXED is made we would have an invalidate_range event for our > object. After that we should no longer alias with the rogue mmapping. If > we are then able to mark the object as no longer in use after the first > invalidate, we do not need to grab the struct_mutex for the subsequent > invalidations. > > 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. > > v2: Grammar fixes > > Reported-by: Michał Winiarski <[email protected]> > Testcase: igt/gem_userptr_blits/map-fixed* > Signed-off-by: Chris Wilson <[email protected]> > Cc: Michał Winiarski <[email protected]> > Cc: Tvrtko Ursulin <[email protected]> > Cc: [email protected] > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 43 > +++++++++++++++++++++++++++------ > 1 file changed, 36 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c > b/drivers/gpu/drm/i915/i915_gem_userptr.c > index cb367d9f7909..e1965d8c08c8 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; > } > @@ -606,6 +607,20 @@ __i915_gem_userptr_get_pages_worker(struct work_struct > *_work) > wake_up_all(&to_i915(dev)->mm.queue); > } > > +static void > +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, > + bool value) > +{ > +#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 int > i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) > { > @@ -613,6 +628,18 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object > *obj) > struct page **pvec; > int pinned, ret; > > + /* 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. > + */ > + __i915_gem_userptr_set_active(obj, true); > +
This will set mmu_object to active even if we're exiting early from here (because of error handling), creating another possibility for deadlock. -Michał > /* 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 > @@ -732,6 +759,8 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object > *obj) > > sg_free_table(obj->pages); > kfree(obj->pages); > + > + __i915_gem_userptr_set_active(obj, false); > } > > static void > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
