[PATCH] drm/i915: Fix unsafe loop iteration over vma whilst unbinding them

[PATCH] drm/i915: Fix unsafe loop iteration over vma whilst unbinding them

[Chris Wilson]

On non-LLC platforms, when changing the cache level of an object, we may
need to unbind it show that prefetching across page boundaries does not
cross into a different memory domain. This requires us to unbind
conflicting vma, but we did so was iterating over the objects vma in an
unsafe manner (as the list was being modified as we iterated).

The regression was introduced in
commit 3089c6f239d7d2c4cb2dd5c353e8984cf79af1d7
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Wed Jul 31 17:00:03 2013 -0700

    drm/i915: make caching operate on all address spaces
apparently as far back as v3.12-rc1, but it has only just begun to
trigger real world bug reports.

Reported-and-tested-by: Nikolay Martynov <mar.kolya@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76384
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 13fc490d1f62..4f71125493fd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3676,7 +3676,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				    enum i915_cache_level cache_level)
 {
 	struct drm_device *dev = obj->base.dev;
-	struct i915_vma *vma;
+	struct i915_vma *vma, *next;
 	int ret;
 
 	if (obj->cache_level == cache_level)
@@ -3687,7 +3687,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		return -EBUSY;
 	}
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
 		if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
 			ret = i915_vma_unbind(vma);
 			if (ret)
-- 
1.9.1

Re: [PATCH] drm/i915: Fix unsafe loop iteration over vma whilst unbinding them

[Chris Wilson]

On Fri, Mar 21, 2014 at 07:40:56AM +0000, Chris Wilson wrote:
> On non-LLC platforms, when changing the cache level of an object, we may
> need to unbind it show that prefetching across page boundaries does not
s/show/so/
> cross into a different memory domain. This requires us to unbind
> conflicting vma, but we did so was iterating over the objects vma in an
s/was//
> unsafe manner (as the list was being modified as we iterated).

Coffee will kick in in about 5 minutes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix unsafe loop iteration over vma whilst unbinding them

[Daniel Vetter]

On Fri, Mar 21, 2014 at 07:51:58AM +0000, Chris Wilson wrote:
> On Fri, Mar 21, 2014 at 07:40:56AM +0000, Chris Wilson wrote:
> > On non-LLC platforms, when changing the cache level of an object, we may
> > need to unbind it show that prefetching across page boundaries does not
> s/show/so/
> > cross into a different memory domain. This requires us to unbind
> > conflicting vma, but we did so was iterating over the objects vma in an
> s/was//
> > unsafe manner (as the list was being modified as we iterated).
> 
> Coffee will kick in in about 5 minutes.

Fixed and applied, 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: Fix unsafe loop iteration over vma whilst unbinding them

[Jani Nikula]

On Fri, 21 Mar 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Mar 21, 2014 at 07:51:58AM +0000, Chris Wilson wrote:
>> On Fri, Mar 21, 2014 at 07:40:56AM +0000, Chris Wilson wrote:
>> > On non-LLC platforms, when changing the cache level of an object, we may
>> > need to unbind it show that prefetching across page boundaries does not
>> s/show/so/
>> > cross into a different memory domain. This requires us to unbind
>> > conflicting vma, but we did so was iterating over the objects vma in an
>> s/was//
>> > unsafe manner (as the list was being modified as we iterated).
>> 
>> Coffee will kick in in about 5 minutes.
>
> Fixed and applied, thanks for the patch.

Given the regression was introduced in v3.12 I thought this would've
been fixes & cc: stable.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Fix unsafe loop iteration over vma whilst unbinding them

[Daniel Vetter]

On Fri, Mar 21, 2014 at 01:09:48PM +0200, Jani Nikula wrote:
> On Fri, 21 Mar 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Mar 21, 2014 at 07:51:58AM +0000, Chris Wilson wrote:
> >> On Fri, Mar 21, 2014 at 07:40:56AM +0000, Chris Wilson wrote:
> >> > On non-LLC platforms, when changing the cache level of an object, we may
> >> > need to unbind it show that prefetching across page boundaries does not
> >> s/show/so/
> >> > cross into a different memory domain. This requires us to unbind
> >> > conflicting vma, but we did so was iterating over the objects vma in an
> >> s/was//
> >> > unsafe manner (as the list was being modified as we iterated).
> >> 
> >> Coffee will kick in in about 5 minutes.
> >
> > Fixed and applied, thanks for the patch.
> 
> Given the regression was introduced in v3.12 I thought this would've
> been fixes & cc: stable.

Oops, thanks for catching this. cc: stable added. My Linus weather forcast
tells me that -fixes is closed for 3.14 already, so dinq is good enough.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch