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

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

[Chris Wilson]

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

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

[Daniel Vetter]

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

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

[Chris Wilson]

On Thu, Feb 12, 2015 at 10:43:44AM +0100, Daniel Vetter 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?

It would be possible for us to catch an object in the process of being
executed. More so, we *only* care about GTT pinning here, but still we
need to the lock to prevent that disappearing underneath us.

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

We killed that over a year ago in the conversion of error capture over
to vma for full-ppgtt prepartion... Right?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Feb 12, 2015 at 09:48:23AM +0000, Chris Wilson wrote:
> On Thu, Feb 12, 2015 at 10:43:44AM +0100, Daniel Vetter 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?
> 
> It would be possible for us to catch an object in the process of being
> executed. More so, we *only* care about GTT pinning here, but still we
> need to the lock to prevent that disappearing underneath us.

We still need to grab dev->struct_mutex of course to avoid seeing bo
pinned for execbuf. Just thought we could avoid the list walk in
set_tiling as a super-micro-opt.

> > 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?
> 
> We killed that over a year ago in the conversion of error capture over
> to vma for full-ppgtt prepartion... Right?

No, that was left semantically unchanged in the switch. So I guess we
should dump vma->pin_count and obj->framebuffer_references?
-Daniel
-- 
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

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

[Chris Wilson]

On Fri, Feb 13, 2015 at 10:03:49AM +0100, Daniel Vetter wrote:
> On Thu, Feb 12, 2015 at 09:48:23AM +0000, Chris Wilson wrote:
> > On Thu, Feb 12, 2015 at 10:43:44AM +0100, Daniel Vetter wrote:
> > > On Thu, Feb 12, 2015 at 07:53:18AM +0000, Chris Wilson wrote:
> We still need to grab dev->struct_mutex of course to avoid seeing bo
> pinned for execbuf. Just thought we could avoid the list walk in
> set_tiling as a super-micro-opt.

When we have an igt that combines thousands of vm with set-tiling,
then we might notice! :)

> > > 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?
> > 
> > We killed that over a year ago in the conversion of error capture over
> > to vma for full-ppgtt prepartion... Right?
> 
> No, that was left semantically unchanged in the switch. So I guess we
> should dump vma->pin_count and obj->framebuffer_references?

I meant we sent patches to improve error states for full-ppgtt.
vma->pin_count is not interesting, since that is only done for execbuf,
I only care about the GTT pinned objects since they are what we have
pinned on behalf of hardware (and so are useful for crosschecking
against register state), and that is what we specifically dump. Adding
obj->framebuffer_references would be interesting, as well as the list of
current framebuffers i.e.  i915_gem_framebuffers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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

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

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5765
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              282/282              281/282
ILK                                  313/313              313/313
SNB                                  309/323              309/323
IVB                                  380/380              380/380
BYT                                  296/296              296/296
HSW                 -1              425/425              424/425
BDW                 -1              318/318              317/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gen3_render_linear_blits      PASS(5)      CRASH(1)
*HSW  igt_gem_storedw_loop_blt      PASS(3)      DMESG_WARN(1)
*BDW  igt_gem_gtt_hog      PASS(8)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx