> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of > Nick Hoath > Sent: Thursday, February 19, 2015 4:31 PM > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: [PATCH] drm/i915: Fix a use after free, and unbalanced > refcounting > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88652 > > When converting from implicitly tracked execlist queue items to ref counted > requests, not all frees of requests were replaced with unrefs, and extraneous > refs/unrefs of contexts were added. > Correct the unbalanced refcount & replace the frees. > Remove a noisy warning when hitting the request creation path. > > drm_i915_gem_request and intel_context are both kref reference counted > structures. Upon allocation, drm_i915_gem_request's ref count should be > bumped using kref_init. When a context is assigned to the request, > the context's reference count should be bumped using > i915_gem_context_reference. > i915_gem_request_reference will reduce the context reference count when > the request is freed. > > Problem introduced in > commit 6d3d8274bc45de4babb62d64562d92af984dd238 > Author: Nick Hoath <nicholas.hoath@xxxxxxxxx> > AuthorDate: Thu Jan 15 13:10:39 2015 +0000 > > drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request > > v2: Added comments explaining how the ctx pointer and the request object > should > be ref-counted. Removed noisy warning. > > v3: Cleaned up the language used in the commit & the header > description (Thanks David Gordon) > > Signed-off-by: Nick Hoath <nicholas.hoath@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.h | 11 ++++++++++- > drivers/gpu/drm/i915/i915_gem.c | 3 +-- > drivers/gpu/drm/i915/intel_lrc.c | 8 ++++---- > 4 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2dedd43..956fe26 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2121,6 +2121,9 @@ void i915_gem_track_fb(struct drm_i915_gem_object > *old, > * number comparisons on buffer last_read|write_seqno. It also allows an > * emission time to be associated with the request for tracking how far ahead > * of the GPU the submission is. > + * > + * The requests are reference counted, so upon creation they should have an > + * initial reference taken using kref_init > */ > struct drm_i915_gem_request { > struct kref ref; > @@ -2144,7 +2147,16 @@ struct drm_i915_gem_request { > /** Position in the ringbuffer of the end of the whole request */ > u32 tail; > > - /** Context related to this request */ > + /** > + * Context related to this request > + * Contexts are refcounted, so when this request is associated with a > + * context, we must increment the context's refcount, to guarantee that > + * it persists while any request is linked to it. Requests themselves > + * are also refcounted, so the request will only be freed when the last > + * reference to it is dismissed, and the code in > + * i915_gem_request_free() will then decrement the refcount on the > + * context. > + */ > struct intel_context *ctx; > > /** Batch buffer related to this request if any */ > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c > index 61134ab..996f60f 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2664,8 +2664,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..62a2b2a 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -512,18 +512,19 @@ static int execlists_context_queue(struct > intel_engine_cs *ring, > * If there isn't a request associated with this submission, > * create one as a temporary holder. > */ > - WARN(1, "execlist context submission without request"); > request = kzalloc(sizeof(*request), GFP_KERNEL); > if (request == NULL) > return -ENOMEM; > request->ring = ring; > request->ctx = to; > + kref_init(&request->ref); > + request->uniq = dev_priv->request_uniq++; > + i915_gem_context_reference(request->ctx); > } else { > + i915_gem_request_reference(request); > 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 +741,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 Reviewed-by: Thomas Daniel <thomas.daniel@xxxxxxxxx> _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
