[Intel-gfx] [PATCH v2] drm/i915: Fix userptr deadlock with MAP_FIXED

[Michał Winiarski]

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 <michal.winiarski at intel.com>
> Testcase: igt/gem_userptr_blits/map-fixed*
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: stable at vger.kernel.org
> ---
>  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
>