Re: [PATCH] drm/i915: Check obj->vma_list under the struct_mutex

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 12, 2015 at 07:53:18AM +0000, Chris Wilson wrote:
> When we walk the list of vma, or even for protecting against concurrent
> framebuffer creation, we must hold the struct_mutex or else a second
> thread can corrupt the list as we walk it.
> 
> Fixes regression from
> commit d7f46fc4e7323887494db13f063a8e59861fefb0
> Author: Ben Widawsky <benjamin.widawsky@intel.com>
> Date:   Fri Dec 6 14:10:55 2013 -0800
> 
>     drm/i915: Make pin count per VMA
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=89085
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_tiling.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 7a24bd1a51f6..6377b22269ad 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -335,9 +335,10 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> +	mutex_lock(&dev->struct_mutex);
>  	if (i915_gem_obj_is_pinned(obj) || obj->framebuffer_references) {

Since the removal of userspace pinning we shouldn't be able to see pinned
objects here which are _not_ framebuffers too. But we still need the lock
for synchronization and to avoid races, but perhaps we could drop the list
walk?

Either way this is

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org (we have some vague evidence that it blows up
at last)

I've also audited all the other callers of is_pinned, the only other
suspicious one is the one in capture_bo. Perhaps we should also move that
over to obj->framebuffer_references?

Cheers, Daniel
> -		drm_gem_object_unreference_unlocked(&obj->base);
> -		return -EBUSY;
> +		ret = -EBUSY;
> +		goto err;
>  	}
>  
>  	if (args->tiling_mode == I915_TILING_NONE) {
> @@ -369,7 +370,6 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  		}
>  	}
>  
> -	mutex_lock(&dev->struct_mutex);
>  	if (args->tiling_mode != obj->tiling_mode ||
>  	    args->stride != obj->stride) {
>  		/* We need to rebind the object if its current allocation
> @@ -424,6 +424,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  		obj->bit_17 = NULL;
>  	}
>  
> +err:
>  	drm_gem_object_unreference(&obj->base);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx