On Tue, Feb 03, 2015 at 03:08:17PM +0000, Chris Wilson wrote:
> On Tue, Feb 03, 2015 at 03:48:17PM +0100, Michał Winiarski wrote:
> > It's possible for invalidate_range_start mmu notifier callback to race
> > against userptr object release. If the gem object was released prior to
> > obtaining the spinlock in invalidate_range_start we're hitting null
> > pointer dereference.
> >
> > Testcase: igt/gem_userptr_blits/stress-mm-invalidate-close*
> > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > Signed-off-by: Michał Winiarski <michal.winiar...@intel.com>
> Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
Since it blows up in the real world already also
Cc: sta...@vger.kernel.org
and for Jani. And Jani can add the comment while applying, I like it -
explaining that kind of weak ref stuff is always good.
-Daniel
>
> Though I would personally remove the extra newline and add an extra comment
> instead:
>
> > ---
> > drivers/gpu/drm/i915/i915_gem_userptr.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
> > b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > index d182058..64b8802 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > @@ -113,7 +113,10 @@ restart:
> > continue;
> >
> > obj = mo->obj;
> > - drm_gem_object_reference(&obj->base);
> > + if (!kref_get_unless_zero(&obj->base.refcount))
> > + continue;
> > +
> > spin_unlock(&mn->lock);
> >
> > cancel_userptr(obj);
> > @@ -149,7 +152,13 @@ static void
> > i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> > it = interval_tree_iter_first(&mn->objects, start, end);
> > if (it != NULL) {
> > obj = container_of(it, struct i915_mmu_object, it)->obj;
> > - drm_gem_object_reference(&obj->base);
> /* The mmu_object is released late when
> * destroying the GEM object so it is entirely
> * possible to gain a reference on an object
> * in the process of being freed since our
> * serialisation is via the spinlock and not 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;
> > + }
> > +
> > serial = mn->serial;
> > }
> > spin_unlock(&mn->lock);
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
