On Tue, 24 Feb 2015, Jani Nikula <[email protected]> wrote: > 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.
Bah, QA just reopened the bug. Nick, please look into it. https://bugs.freedesktop.org/show_bug.cgi?id=88652 BR, Jani. > > BR, > Jani. > > >> >> _______________________________________________ >> Intel-gfx mailing list >> [email protected] >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
