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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 03 Feb 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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.

Pushed to drm-intel-next-fixes (and therefore headed for 3.20) with
Chris' comment added.

BR,
Jani.


> -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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx