Re: [PATCH] drm/i915: Prevent double unref following alloc failure during execbuffer

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, Dec 04, 2013 at 09:52:58AM +0000, Chris Wilson wrote:
> Whilst looking up the objects required for an execbuffer, an untimely
> allocation failure in creating the vma results in the object being
> unreferenced from two lists. The ownership during the lookup is meant to
> be moved from the list of objects being looked to the vma, and this
> double unreference upon error results in a use-after-free.
> 
> Fixes regression from
> commit 27173f1f95db5e74ceb35fe9a2f2f348ea11bac9
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Wed Aug 14 11:38:36 2013 +0200
> 
>     drm/i915: Convert execbuf code to use vmas
> 
> Based on the fix by Ben Widawsky.

A note on why this is an improvement over my fix would have been nice. I
had implemented something similar too, but found my eventual patch to be
a little easier to understand.

My real gripe is, I had already sent off my patch to be tested by QA -
and they give me about a 2d turnaround (not including weekends), which
means the soonest I could get this tested and get results is next Wed.

So if there is no improvement, I'd really appreciate this as a cleanup
on top of my patch.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index c7af37187dee..5663b873a1aa 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -94,7 +94,7 @@ eb_lookup_vmas(struct eb_vmas *eb,
>  	struct drm_i915_private *dev_priv = vm->dev->dev_private;
>  	struct drm_i915_gem_object *obj;
>  	struct list_head objects;
> -	int i, ret = 0;
> +	int i, ret;
>  
>  	INIT_LIST_HEAD(&objects);
>  	spin_lock(&file->table_lock);
> @@ -107,7 +107,7 @@ eb_lookup_vmas(struct eb_vmas *eb,
>  			DRM_DEBUG("Invalid object handle %d at index %d\n",
>  				   exec[i].handle, i);
>  			ret = -ENOENT;
> -			goto out;
> +			goto err;
>  		}
>  
>  		if (!list_empty(&obj->obj_exec_link)) {
> @@ -115,7 +115,7 @@ eb_lookup_vmas(struct eb_vmas *eb,
>  			DRM_DEBUG("Object %p [handle %d, index %d] appears more than once in object list\n",
>  				   obj, exec[i].handle, i);
>  			ret = -EINVAL;
> -			goto out;
> +			goto err;
>  		}
>  
>  		drm_gem_object_reference(&obj->base);
> @@ -124,7 +124,7 @@ eb_lookup_vmas(struct eb_vmas *eb,
>  	spin_unlock(&file->table_lock);
>  
>  	i = 0;
> -	list_for_each_entry(obj, &objects, obj_exec_link) {
> +	while (!list_empty(&objects)) {
>  		struct i915_vma *vma;
>  		struct i915_address_space *bind_vm = vm;
>  
> @@ -136,6 +136,10 @@ eb_lookup_vmas(struct eb_vmas *eb,
>  		    (i == (args->buffer_count - 1))))
>  			bind_vm = &dev_priv->gtt.base;
>  
> +		obj = list_first_entry(&objects,
> +				       struct drm_i915_gem_object,
> +				       obj_exec_link);
> +
>  		/*
>  		 * NOTE: We can leak any vmas created here when something fails
>  		 * later on. But that's no issue since vma_unbind can deal with
> @@ -148,10 +152,12 @@ eb_lookup_vmas(struct eb_vmas *eb,
>  		if (IS_ERR(vma)) {
>  			DRM_DEBUG("Failed to lookup VMA\n");
>  			ret = PTR_ERR(vma);
> -			goto out;
> +			goto err;
>  		}
>  
> +		/* Transfer ownership from objects to the vma */
>  		list_add_tail(&vma->exec_list, &eb->vmas);
> +		list_del_init(&obj->obj_exec_link);
>  
>  		vma->exec_entry = &exec[i];
>  		if (eb->and < 0) {
> @@ -165,16 +171,18 @@ eb_lookup_vmas(struct eb_vmas *eb,
>  		++i;
>  	}
>  
> +	return 0;
>  
> -out:
> +
> +err:
>  	while (!list_empty(&objects)) {
>  		obj = list_first_entry(&objects,
>  				       struct drm_i915_gem_object,
>  				       obj_exec_link);
>  		list_del_init(&obj->obj_exec_link);
> -		if (ret)
> -			drm_gem_object_unreference(&obj->base);
> +		drm_gem_object_unreference(&obj->base);
>  	}
> +	/* objects already transfered to the vma will be reaped by eb_destroy */
>  	return ret;
>  }
>  
> -- 
> 1.8.5
> 

-- 
Ben Widawsky, Intel Open Source Technology Center