[PATCH] drm/i915: Fixed bad refcounting on execbuf failures

[PATCH] drm/i915: Fixed bad refcounting on execbuf failures

[Ben Widawsky]

eb_destroy currently cleans up the refcounts for all the VMAs done at
lookup. Currently eb_lookup_vmas cleans up all the *objects* we've
looked up. There exists a period of time when we under severe memory
pressure, the VMA creation will fail, and fall into our exit path.

When the above event occurs, the object list, and eb->vma list are not
equal, the latter being a subset of the former. As we attempt to clean
up the refcounts on the error path we have the potential to decrement
the refcount by one extra here.

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

Cc: stable@vger.kernel.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index c7af371..b5c940a 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -167,6 +167,13 @@ eb_lookup_vmas(struct eb_vmas *eb,
 
 
 out:
+	/* eb_vmas are cleaned up by destroy. Others are not */
+	if (ret) {
+		struct i915_vma *vma;
+		list_for_each_entry(vma, &eb->vmas, exec_list)
+			list_del(&vma->obj->obj_exec_link);
+	}
+
 	while (!list_empty(&objects)) {
 		obj = list_first_entry(&objects,
 				       struct drm_i915_gem_object,
-- 
1.8.4.2

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

[Chris Wilson]

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.

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

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

[Ben Widawsky]

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

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

[Chris Wilson]

On Wed, Dec 04, 2013 at 09:23:24AM -0800, Ben Widawsky wrote:
> 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.

It all lies in the transfer of ownership comment. With that expressed,
it is no longer an object residing on two lists that we must untangle,
but a temporary list that holds the lookups which we convert into
eb_vma. It is clear then we only need to clean up the temporary list
upon failure.

> 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.

Your changelog belies why not.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Daniel Vetter]

On Wed, Dec 04, 2013 at 05:37:14PM +0000, Chris Wilson wrote:
> On Wed, Dec 04, 2013 at 09:23:24AM -0800, Ben Widawsky wrote:
> > 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.
> 
> It all lies in the transfer of ownership comment. With that expressed,
> it is no longer an object residing on two lists that we must untangle,
> but a temporary list that holds the lookups which we convert into
> eb_vma. It is clear then we only need to clean up the temporary list
> upon failure.
> 
> > 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.
> 
> Your changelog belies why not.

Picked up for -fixes (with the comment bikeshed as discussd on irc),
thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch