Re: [PATCH] drm/i915: Prevent use-after-free in invalidate_range_start callback

[Daniel Vetter <daniel@ffwll.ch>]

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 <chris@chris-wilson.co.uk>
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Since it blows up in the real world already also

Cc: stable@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