public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Reset vma->mm_list after unbinding
@ 2014-02-25 14:23 Chris Wilson
  2014-02-25 23:38 ` Ben Widawsky
  2014-02-27 14:11 ` Ville Syrjälä
  0 siblings, 2 replies; 6+ messages in thread
From: Chris Wilson @ 2014-02-25 14:23 UTC (permalink / raw)
  To: intel-gfx; +Cc: Ben Widawsky

In place of true activity counting, we walk the list of vma associated
with an object managing each on the vm's active/inactive list everytime
we call move-to-inactive. This depends upon the vma->mm_list being
cleared after unbinding, or else we run into difficulty when tracking
the object in multiple vm's - we see a use-after free and corruption of
the mm_list.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 633a8d56e401..4de984e176f5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2874,7 +2874,7 @@ int i915_vma_unbind(struct i915_vma *vma)
 
 	i915_gem_gtt_finish_object(obj);
 
-	list_del(&vma->mm_list);
+	list_del_init(&vma->mm_list);
 	if (i915_is_ggtt(vma->vm))
 		obj->map_and_fenceable = false;
 
-- 
1.9.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding
  2014-02-25 14:23 [PATCH] drm/i915: Reset vma->mm_list after unbinding Chris Wilson
@ 2014-02-25 23:38 ` Ben Widawsky
  2014-03-05 13:05   ` Chris Wilson
  2014-02-27 14:11 ` Ville Syrjälä
  1 sibling, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2014-02-25 23:38 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Tue, Feb 25, 2014 at 02:23:28PM +0000, Chris Wilson wrote:
> In place of true activity counting, we walk the list of vma associated
> with an object managing each on the vm's active/inactive list everytime
> we call move-to-inactive. This depends upon the vma->mm_list being
> cleared after unbinding, or else we run into difficulty when tracking
> the object in multiple vm's - we see a use-after free and corruption of
> the mm_list.

I feel bad because someone in PRC told me about this bug, and I could
have sword you had previously fixed it. So if you're reading this
whomever you were, my apoligies.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>

I think Cc: stable too

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 633a8d56e401..4de984e176f5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2874,7 +2874,7 @@ int i915_vma_unbind(struct i915_vma *vma)
>  
>  	i915_gem_gtt_finish_object(obj);
>  
> -	list_del(&vma->mm_list);
> +	list_del_init(&vma->mm_list);
>  	if (i915_is_ggtt(vma->vm))
>  		obj->map_and_fenceable = false;
>  
> -- 
> 1.9.0
> 

-- 
Ben Widawsky, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding
  2014-02-25 14:23 [PATCH] drm/i915: Reset vma->mm_list after unbinding Chris Wilson
  2014-02-25 23:38 ` Ben Widawsky
@ 2014-02-27 14:11 ` Ville Syrjälä
  2014-02-27 14:28   ` Chris Wilson
  1 sibling, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2014-02-27 14:11 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, Ben Widawsky

On Tue, Feb 25, 2014 at 02:23:28PM +0000, Chris Wilson wrote:
> In place of true activity counting, we walk the list of vma associated
> with an object managing each on the vm's active/inactive list everytime
> we call move-to-inactive. This depends upon the vma->mm_list being
> cleared after unbinding, or else we run into difficulty when tracking
> the object in multiple vm's - we see a use-after free and corruption of
> the mm_list.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 633a8d56e401..4de984e176f5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2874,7 +2874,7 @@ int i915_vma_unbind(struct i915_vma *vma)
>  
>  	i915_gem_gtt_finish_object(obj);
>  
> -	list_del(&vma->mm_list);
> +	list_del_init(&vma->mm_list);

Isn't this just another symptom of the vma unbind recursion bug? I mean
how can someone else be accessing vma->mm_list while we're in the process
of freeing the vma itself (happens just a few lines down from here).

>  	if (i915_is_ggtt(vma->vm))
>  		obj->map_and_fenceable = false;
>  
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding
  2014-02-27 14:11 ` Ville Syrjälä
@ 2014-02-27 14:28   ` Chris Wilson
  0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2014-02-27 14:28 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Ben Widawsky

On Thu, Feb 27, 2014 at 04:11:39PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 25, 2014 at 02:23:28PM +0000, Chris Wilson wrote:
> > In place of true activity counting, we walk the list of vma associated
> > with an object managing each on the vm's active/inactive list everytime
> > we call move-to-inactive. This depends upon the vma->mm_list being
> > cleared after unbinding, or else we run into difficulty when tracking
> > the object in multiple vm's - we see a use-after free and corruption of
> > the mm_list.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 633a8d56e401..4de984e176f5 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2874,7 +2874,7 @@ int i915_vma_unbind(struct i915_vma *vma)
> >  
> >  	i915_gem_gtt_finish_object(obj);
> >  
> > -	list_del(&vma->mm_list);
> > +	list_del_init(&vma->mm_list);
> 
> Isn't this just another symptom of the vma unbind recursion bug? I mean
> how can someone else be accessing vma->mm_list while we're in the process
> of freeing the vma itself (happens just a few lines down from here).

We don't always free the vma along this path. Then we try to bind the
vma in the second VM which hits the stale list entry.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding
  2014-02-25 23:38 ` Ben Widawsky
@ 2014-03-05 13:05   ` Chris Wilson
  2014-03-05 14:15     ` Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2014-03-05 13:05 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Tue, Feb 25, 2014 at 03:38:18PM -0800, Ben Widawsky wrote:
> On Tue, Feb 25, 2014 at 02:23:28PM +0000, Chris Wilson wrote:
> > In place of true activity counting, we walk the list of vma associated
> > with an object managing each on the vm's active/inactive list everytime
> > we call move-to-inactive. This depends upon the vma->mm_list being
> > cleared after unbinding, or else we run into difficulty when tracking
> > the object in multiple vm's - we see a use-after free and corruption of
> > the mm_list.
> 
> I feel bad because someone in PRC told me about this bug, and I could
> have sword you had previously fixed it. So if you're reading this
> whomever you were, my apoligies.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> 
> I think Cc: stable too
> 
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Daniel?
-Chris

> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 633a8d56e401..4de984e176f5 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2874,7 +2874,7 @@ int i915_vma_unbind(struct i915_vma *vma)
> >  
> >  	i915_gem_gtt_finish_object(obj);
> >  
> > -	list_del(&vma->mm_list);
> > +	list_del_init(&vma->mm_list);
> >  	if (i915_is_ggtt(vma->vm))
> >  		obj->map_and_fenceable = false;
> >  
> > -- 
> > 1.9.0

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding
  2014-03-05 13:05   ` Chris Wilson
@ 2014-03-05 14:15     ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-03-05 14:15 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, intel-gfx

On Wed, Mar 05, 2014 at 01:05:29PM +0000, Chris Wilson wrote:
> On Tue, Feb 25, 2014 at 03:38:18PM -0800, Ben Widawsky wrote:
> > On Tue, Feb 25, 2014 at 02:23:28PM +0000, Chris Wilson wrote:
> > > In place of true activity counting, we walk the list of vma associated
> > > with an object managing each on the vm's active/inactive list everytime
> > > we call move-to-inactive. This depends upon the vma->mm_list being
> > > cleared after unbinding, or else we run into difficulty when tracking
> > > the object in multiple vm's - we see a use-after free and corruption of
> > > the mm_list.
> > 
> > I feel bad because someone in PRC told me about this bug, and I could
> > have sword you had previously fixed it. So if you're reading this
> > whomever you were, my apoligies.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > 
> > I think Cc: stable too
> > 
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Daniel?
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-05 14:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-25 14:23 [PATCH] drm/i915: Reset vma->mm_list after unbinding Chris Wilson
2014-02-25 23:38 ` Ben Widawsky
2014-03-05 13:05   ` Chris Wilson
2014-03-05 14:15     ` Daniel Vetter
2014-02-27 14:11 ` Ville Syrjälä
2014-02-27 14:28   ` Chris Wilson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox