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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre