| Message ID | 1422974897-4420-1-git-send-email-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> 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
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
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
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); + + if (!kref_get_unless_zero(&obj->base.refcount)) { + spin_unlock(&mn->lock); + serial = 0; + continue; + } + serial = mn->serial; } spin_unlock(&mn->lock);
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 Testcase: igt/gem_userptr_blits/stress-mm-invalidate-close-overlap Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com> --- drivers/gpu/drm/i915/i915_gem_userptr.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)