[Intel-gfx] [PATCH 1/2] drm/i915: fix reference counting in i915_gem_create

[Daniel Vetter]

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 at chris-wilson.co.uk>
> > 
> > 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 at chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> 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 at chris-wilson.co.uk>
> 
> > ---
> >  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