> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
> Daniel Vetter
> Sent: Friday, February 13, 2015 1:50 PM
> To: Hoath, Nicholas
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and
> unbalanced
> refcounting
>
> On Fri, Feb 13, 2015 at 01:30:35PM +0000, Nick Hoath wrote:
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88652
> >
> > When converting from implicitly tracked execlist queue items to ref counted
> > requests, not all free's of requests were replaced with unrefs, and
> > extraneous
> > refs/unrefs of contexts were added.
> > Correct the unbalanced refcount & replace the free's.
> >
> > Problem introduced in:
> > commit 6d3d8274bc45de4babb62d64562d92af984dd238
> > Author: Nick Hoath <nicholas.ho...@intel.com>
> > AuthorDate: Thu Jan 15 13:10:39 2015 +0000
> >
> > drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request
>
> Imo the commit message should be ammended with a short paragraph explainig
> the various pointers and implied and explicit references we now have
> around requests and contexts. That way review of this will get a bit
> easier and we'll avoid another misunderstanding.
>
> I even think we should add a comment in the header to request.ctx to
> explain the rules since apparently they've not been fully clear.
Agree that more documentation around these ctx refs would be good to have to
clear up confusion.
For example, I initially thought that this patch introduced a new
use-after-free because of the removal of the ctx ref in
execlists_context_queue().
>
> > Signed-off-by: Nick Hoath <nicholas.ho...@intel.com>
>
> But yeah this makes a lot more sense imo. Please feed this to QA for
> stress-testing in all the relevant bugs. Today I have my head full with
> kms code so not a good time for a full in-depth review. But I think it'd
> be good if other people take a look anyway, so please throw this at a few
> ppl from the vpg core team too.
I guess that would be me...
The code changes look OK, would like to see the updated comments and QA results.
Cheers,
Thomas.
> Thanks, Daniel
>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 3 +--
> > drivers/gpu/drm/i915/intel_lrc.c | 3 +--
> > 2 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> > index 1765989..79e48b2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2660,8 +2660,7 @@ static void i915_gem_reset_ring_cleanup(struct
> drm_i915_private *dev_priv,
> > if (submit_req->ctx != ring->default_context)
> > intel_lr_context_unpin(ring, submit_req->ctx);
> >
> > - i915_gem_context_unreference(submit_req->ctx);
> > - kfree(submit_req);
> > + i915_gem_request_unreference(submit_req);
> > }
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index aafcef3..a18925d 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -518,12 +518,12 @@ static int execlists_context_queue(struct
> intel_engine_cs *ring,
> > return -ENOMEM;
> > request->ring = ring;
> > request->ctx = to;
> > + i915_gem_context_reference(request->ctx);
> > } else {
> > WARN_ON(to != request->ctx);
> > }
> > request->tail = tail;
> > i915_gem_request_reference(request);
> > - i915_gem_context_reference(request->ctx);
> >
> > intel_runtime_pm_get(dev_priv);
> >
> > @@ -740,7 +740,6 @@ void intel_execlists_retire_requests(struct
> intel_engine_cs *ring)
> > if (ctx_obj && (ctx != ring->default_context))
> > intel_lr_context_unpin(ring, ctx);
> > intel_runtime_pm_put(dev_priv);
> > - i915_gem_context_unreference(ctx);
> > list_del(&req->execlist_link);
> > i915_gem_request_unreference(req);
> > }
> > --
> > 2.1.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
