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@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx>
> Cc: Ben Widawsky <ben@xxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
