On Mon, 23 Feb 2015, "Daniel, Thomas" <[email protected]> wrote: >> -----Original Message----- >> From: Intel-gfx [mailto:[email protected]] On Behalf Of >> Nick Hoath >> Sent: Thursday, February 19, 2015 4:31 PM >> To: [email protected] >> Subject: [Intel-gfx] [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 <[email protected]> >> 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 <[email protected]> >> --- >> 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 <[email protected]>
Pushed to drm-intel-fixes, with additional r-b from Daniel, thanks for the patch and review. BR, Jani. > > _______________________________________________ > Intel-gfx mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
