| Message ID | 1407615316-30645-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 09, 2014 at 01:15:16PM -0700, Ben Widawsky wrote: > See the following for many more details. > > commit acc240d41ea1ab9c488a79219fb313b5b46265ae > Author: Daniel Vetter <daniel.vetter@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 <ben@bwidawsk.net> Ok, I convinced myself that the you are fixing the bug you describe and don't seem to be introducing a new one, so Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Sun, Aug 10, 2014 at 09:04:10AM +0100, Chris Wilson wrote: > On Sat, Aug 09, 2014 at 01:15:16PM -0700, Ben Widawsky wrote: > > See the following for many more details. > > > > commit acc240d41ea1ab9c488a79219fb313b5b46265ae > > Author: Daniel Vetter <daniel.vetter@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 Pardon my ignorance (well this stuff is just hard), but can the above still happen with Michel Thierry's patch to rework ppgtt_release? In particular I seem to be too dense to find the ppgtt_release -> gpu_idle step once the forcefull vma unbinding is gone. Doe I miss something? Someone please enlighten me ... Thanks, Daniel > > > > 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 <ben@bwidawsk.net> > > Ok, I convinced myself that the you are fixing the bug you describe and > don't seem to be introducing a new one, so > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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);
See the following for many more details. commit acc240d41ea1ab9c488a79219fb313b5b46265ae Author: Daniel Vetter <daniel.vetter@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 <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_context.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)