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

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>, Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Check obj->vma_list under the struct_mutex
Date: Tue, 24 Feb 2015 16:01:36 +0200	[thread overview]
Message-ID: <8761arpfwf.fsf@intel.com> (raw)
In-Reply-To: <20150212094344.GA24485@phenom.ffwll.local>

On Thu, 12 Feb 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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)

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx