[Intel-gfx] [PATCH] [v2] drm/i915: Fix another another use-after-free in do_switch

[Ben Widawsky]

See the following for many more details.

commit acc240d41ea1ab9c488a79219fb313b5b46265ae
Author: Daniel Vetter <daniel.vet...@ffwll.ch>
Date:   Thu Dec 5 15:42:34 2013 +0100

    drm/i915: Fix use-after-free in do_switch

In this case, the issue is only for full PPGTT:
do_switch
  context_unref
    ppgtt_release
      i915_gpu_idle
        switch_to_default
        from changes to default context

This could be backported to the pre do_switch cleanup I did in this
series. However, it's much cleaner and more obvious as a patch on top,
so I'd really like to do this as a post cleanup patch.

v2: There was a bug in the original patch where the ring->last_context
was set too early. I am not sure how this wasn't being hit when I sent
this previously. Perhaps I tested the wrong patch previously.

Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index c9aa3e6..0ce8fc9 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -609,14 +609,18 @@ mi_set_context(struct intel_engine_cs *ring,
        return ret;
 }
 
-static void do_switch_fini_common(struct intel_engine_cs *ring,
-                                 struct intel_context *from,
-                                 struct intel_context *to)
+static struct intel_context *do_switch_fini_common(struct intel_engine_cs 
*ring,
+                                                  struct intel_context *from,
+                                                  struct intel_context *to)
 {
+       struct intel_context *ret;
        if (likely(from))
                i915_gem_context_unreference(from);
        i915_gem_context_reference(to);
+       ret = ring->last_context;
        ring->last_context = to;
+
+       return ret;
 }
 
 static int do_switch_xcs(struct intel_engine_cs *ring,
@@ -762,14 +766,20 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
                 */
                from->legacy_hw_ctx.rcs_state->dirty = 1;
                BUG_ON(from->legacy_hw_ctx.rcs_state->ring != ring);
-
-               /* obj is kept alive until the next request by its active ref */
-               i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state);
        }
 
        uninitialized = !to->legacy_hw_ctx.initialized && from == NULL;
        to->legacy_hw_ctx.initialized = true;
-       do_switch_fini_common(ring, from, to);
+       /* From may have disappeared again after the context unref */
+       from = do_switch_fini_common(ring, from, to);
+       if (from != NULL) {
+               /* obj is kept alive until the next request by its active ref.
+                * XXX: The context needs to be unpinned last, or else we risk
+                * hitting evict/idle on the ppgtt free, which will call back
+                * into this, and we'll get a double unpin on this context
+                */
+               i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state);
+       }
 
        if (uninitialized) {
                ret = i915_gem_render_state_init(ring);
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx