[PATCH] drm/i915: Reset vma->mm_list after unbinding

[PATCH] drm/i915: Reset vma->mm_list after unbinding

[Chris Wilson]

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

Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding

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

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

Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding

[Chris Wilson]

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

Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding

[Daniel Vetter]

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

Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Reset vma->mm_list after unbinding

[Chris Wilson]

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