On Tue, 03 Feb 2015, Daniel Vetter <[email protected]> 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 <[email protected]>
>> > Signed-off-by: Michał Winiarski <[email protected]>
>> Reviewed-by: Chris Wilson <[email protected]>
>
> Since it blows up in the real world already also
>
> Cc: [email protected]
>
> 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
>> [email protected]
>> 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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
