On Wed, Jul 24, 2013 at 06:05:07PM +0100, Chris Wilson wrote:
> On Wed, Jul 24, 2013 at 03:49:42PM +0200, Daniel Vetter wrote:
> > This function is called without the dev->struct_mutex held, hence we
> > need to use the _unlocked unreference variants.
> >
> > As soon as the object is registered userspace can sneak in here with a
> > gem_close ioctl call, so the object can (and with my new evil tests
> > actually does) get the final unreference in this place. The lack of
> > locking then results in hilarity and some good leakage.
> >
> > To fix this we simply need to revert
> >
> > Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >
> > v2: We need to make the trace call _before_ we drop our ref - the
> > object might very well be gone by then already.
> >
> > v3: Just revert the original patch as suggested by Chris Wilson.
> >
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> I'm sad to see this go, but we can fix any potential mutex contention
> here by generically fixing _unlocked() - which is the better solution.
>
> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 15 ++++++---------
> > 1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 957e65a..dc7e6de 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -219,18 +219,13 @@ i915_gem_create(struct drm_file *file,
> > return -ENOMEM;
> >
> > ret = drm_gem_handle_create(file, &obj->base, &handle);
> > - if (ret) {
> > - drm_gem_object_release(&obj->base);
> > - i915_gem_info_remove_obj(dev->dev_private, obj->base.size);
> > - i915_gem_object_free(obj);
> > - return ret;
> > - }
> > -
> > /* drop reference from allocate - handle holds it now */
> > - drm_gem_object_unreference(&obj->base);
> > - trace_i915_gem_object_create(obj);
> > + drm_gem_object_unreference_unlocked(&obj->base);
> > + if (ret)
> > + return ret;
> >
> > *handle_p = handle;
> > +
> > return 0;
>
> However, I do like my return parameters clustered - any chance this
> extra whitespace can be erradicated?
Done and queued for -next, thanks for the review.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
