[PATCH] drm/i915: Fix list corruption in vma_unbind

[PATCH] drm/i915: Fix list corruption in vma_unbind

[Daniel Vetter]

The saga around the breadcrumb vmas used by execbuf continues ...

This time around we've managed to unconditionally move the object to
the unbound list on the last vma unbind even though it might never
have been on either the bound or unbound list. Hilarity ensued.

Chris Wilson tracked this one down but compared to his patches I've
simply opted to completely separate the unbound case for not-yet bound
vmas. Otherwise we imo end up with semantically hard to parse checks
around the list_move_tail(global_list, ...).

This is exercised by the new swapping variants of
igt/tests/gem_evict_everything.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
Bugzilla: https://bugs.freedesktop.org/attachment.cgi?id=84818
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d9eee14..d12dfc3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2624,8 +2624,11 @@ int i915_vma_unbind(struct i915_vma *vma)
 	if (list_empty(&vma->vma_link))
 		return 0;
 
-	if (!drm_mm_node_allocated(&vma->node))
-		goto destroy;
+	if (!drm_mm_node_allocated(&vma->node)) {
+		i915_gem_vma_destroy(vma);
+
+		return 0;
+	}
 
 	if (obj->pin_count)
 		return -EBUSY;
@@ -2665,7 +2668,6 @@ int i915_vma_unbind(struct i915_vma *vma)
 
 	drm_mm_remove_node(&vma->node);
 
-destroy:
 	i915_gem_vma_destroy(vma);
 
 	/* Since the unbound list is global, only move to that list if
-- 
1.8.4.rc3

Re: [PATCH] drm/i915: Fix list corruption in vma_unbind

[Chris Wilson]

On Thu, Aug 29, 2013 at 07:50:31PM +0200, Daniel Vetter wrote:
> The saga around the breadcrumb vmas used by execbuf continues ...
> 
> This time around we've managed to unconditionally move the object to
> the unbound list on the last vma unbind even though it might never
> have been on either the bound or unbound list. Hilarity ensued.
> 
> Chris Wilson tracked this one down but compared to his patches I've
> simply opted to completely separate the unbound case for not-yet bound
> vmas. Otherwise we imo end up with semantically hard to parse checks
> around the list_move_tail(global_list, ...).
> 
> This is exercised by the new swapping variants of
> igt/tests/gem_evict_everything.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Bugzilla: https://bugs.freedesktop.org/attachment.cgi?id=84818
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d9eee14..d12dfc3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2624,8 +2624,11 @@ int i915_vma_unbind(struct i915_vma *vma)
>  	if (list_empty(&vma->vma_link))
>  		return 0;
>  
> -	if (!drm_mm_node_allocated(&vma->node))
> -		goto destroy;
> +	if (!drm_mm_node_allocated(&vma->node)) {
> +		i915_gem_vma_destroy(vma);
> +

I'm equivocal on this particular whitespace. For such short branches, it
looks neater with a tight return.

> +		return 0;
> +	}

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix list corruption in vma_unbind

[Daniel Vetter]

On Thu, Aug 29, 2013 at 07:57:38PM +0100, Chris Wilson wrote:
> On Thu, Aug 29, 2013 at 07:50:31PM +0200, Daniel Vetter wrote:
> > The saga around the breadcrumb vmas used by execbuf continues ...
> > 
> > This time around we've managed to unconditionally move the object to
> > the unbound list on the last vma unbind even though it might never
> > have been on either the bound or unbound list. Hilarity ensued.
> > 
> > Chris Wilson tracked this one down but compared to his patches I've
> > simply opted to completely separate the unbound case for not-yet bound
> > vmas. Otherwise we imo end up with semantically hard to parse checks
> > around the list_move_tail(global_list, ...).
> > 
> > This is exercised by the new swapping variants of
> > igt/tests/gem_evict_everything.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Bugzilla: https://bugs.freedesktop.org/attachment.cgi?id=84818
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Merged, but I've dropped the paragraph about the igt tests again - I just
can't hit the bug any more :( Also I've fixed the bugzilla link, it
pointed at an attachment instead of the bug.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d9eee14..d12dfc3 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2624,8 +2624,11 @@ int i915_vma_unbind(struct i915_vma *vma)
> >  	if (list_empty(&vma->vma_link))
> >  		return 0;
> >  
> > -	if (!drm_mm_node_allocated(&vma->node))
> > -		goto destroy;
> > +	if (!drm_mm_node_allocated(&vma->node)) {
> > +		i915_gem_vma_destroy(vma);
> > +
> 
> I'm equivocal on this particular whitespace. For such short branches, it
> looks neater with a tight return.
> 
> > +		return 0;
> > +	}
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Fix list corruption in vma_unbind

[Chris Wilson]

On Thu, Aug 29, 2013 at 11:06:24PM +0200, Daniel Vetter wrote:
> On Thu, Aug 29, 2013 at 07:57:38PM +0100, Chris Wilson wrote:
> > On Thu, Aug 29, 2013 at 07:50:31PM +0200, Daniel Vetter wrote:
> > > The saga around the breadcrumb vmas used by execbuf continues ...
> > > 
> > > This time around we've managed to unconditionally move the object to
> > > the unbound list on the last vma unbind even though it might never
> > > have been on either the bound or unbound list. Hilarity ensued.
> > > 
> > > Chris Wilson tracked this one down but compared to his patches I've
> > > simply opted to completely separate the unbound case for not-yet bound
> > > vmas. Otherwise we imo end up with semantically hard to parse checks
> > > around the list_move_tail(global_list, ...).
> > > 
> > > This is exercised by the new swapping variants of
> > > igt/tests/gem_evict_everything.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > > Bugzilla: https://bugs.freedesktop.org/attachment.cgi?id=84818
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Merged, but I've dropped the paragraph about the igt tests again - I just
> can't hit the bug any more :( Also I've fixed the bugzilla link, it
> pointed at an attachment instead of the bug.

It fixed the tests for me, just got a whole new trace when using GL.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix list corruption in vma_unbind

[Ben Widawsky]

On Thu, Aug 29, 2013 at 10:19:24PM +0100, Chris Wilson wrote:
> On Thu, Aug 29, 2013 at 11:06:24PM +0200, Daniel Vetter wrote:
> > On Thu, Aug 29, 2013 at 07:57:38PM +0100, Chris Wilson wrote:
> > > On Thu, Aug 29, 2013 at 07:50:31PM +0200, Daniel Vetter wrote:
> > > > The saga around the breadcrumb vmas used by execbuf continues ...
> > > > 
> > > > This time around we've managed to unconditionally move the object to
> > > > the unbound list on the last vma unbind even though it might never
> > > > have been on either the bound or unbound list. Hilarity ensued.
> > > > 
> > > > Chris Wilson tracked this one down but compared to his patches I've
> > > > simply opted to completely separate the unbound case for not-yet bound
> > > > vmas. Otherwise we imo end up with semantically hard to parse checks
> > > > around the list_move_tail(global_list, ...).
> > > > 
> > > > This is exercised by the new swapping variants of
> > > > igt/tests/gem_evict_everything.
> > > > 
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > > > Bugzilla: https://bugs.freedesktop.org/attachment.cgi?id=84818
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Merged, but I've dropped the paragraph about the igt tests again - I just
> > can't hit the bug any more :( Also I've fixed the bugzilla link, it
> > pointed at an attachment instead of the bug.
> 
> It fixed the tests for me, just got a whole new trace when using GL.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

I'm not really sure why the assertion I added blew up :/
Tested-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center