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

[Chris Wilson]

On Wed, Jul 01, 2015 at 12:14:34PM +0100, Tvrtko Ursulin wrote:
> >  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
> 
> Would this be more obvious as atomic_t? Since I suspect spinlock is
> just for the memory barrier, if that.

Yes, you could probably get away with a little thought and just a memory
barrier. But since one side is guarded by the spinlock, I think it is
easier to reuse that. Especially when the expression becomes a little
more complicated.

> Hm.. What if we get invalidate
> while get_pages is running, before it set active but after gup has
> succeeded?

Yeah, that actually is a probably. Especially thinking about the
gup_worker which we need to cancel.

Back to setting active earlier and clearing it along error paths.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre