Commit b3a38998 authored by Nick Hoath's avatar Nick Hoath Committed by Jani Nikula

drm/i915: Fix a use after free, and unbalanced refcounting

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 6d3d8274
Author:     Nick Hoath <nicholas.hoath@intel.com>
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)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88652Signed-off-by: default avatarNick Hoath <nicholas.hoath@intel.com>
Reviewed-by: default avatarThomas Daniel <thomas.daniel@intel.com>
Reviewed-by: default avatarDaniel Vetter <daniel@ffwll.ch>
Signed-off-by: default avatarJani Nikula <jani.nikula@intel.com>
parent cf6f0af9
...@@ -2114,6 +2114,9 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old, ...@@ -2114,6 +2114,9 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old,
* number comparisons on buffer last_read|write_seqno. It also allows an * 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 * emission time to be associated with the request for tracking how far ahead
* of the GPU the submission is. * 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 drm_i915_gem_request {
struct kref ref; struct kref ref;
...@@ -2137,7 +2140,16 @@ struct drm_i915_gem_request { ...@@ -2137,7 +2140,16 @@ struct drm_i915_gem_request {
/** Position in the ringbuffer of the end of the whole request */ /** Position in the ringbuffer of the end of the whole request */
u32 tail; 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; struct intel_context *ctx;
/** Batch buffer related to this request if any */ /** Batch buffer related to this request if any */
......
...@@ -2659,8 +2659,7 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, ...@@ -2659,8 +2659,7 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
if (submit_req->ctx != ring->default_context) if (submit_req->ctx != ring->default_context)
intel_lr_context_unpin(ring, submit_req->ctx); intel_lr_context_unpin(ring, submit_req->ctx);
i915_gem_context_unreference(submit_req->ctx); i915_gem_request_unreference(submit_req);
kfree(submit_req);
} }
/* /*
......
...@@ -503,18 +503,19 @@ static int execlists_context_queue(struct intel_engine_cs *ring, ...@@ -503,18 +503,19 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
* If there isn't a request associated with this submission, * If there isn't a request associated with this submission,
* create one as a temporary holder. * create one as a temporary holder.
*/ */
WARN(1, "execlist context submission without request");
request = kzalloc(sizeof(*request), GFP_KERNEL); request = kzalloc(sizeof(*request), GFP_KERNEL);
if (request == NULL) if (request == NULL)
return -ENOMEM; return -ENOMEM;
request->ring = ring; request->ring = ring;
request->ctx = to; request->ctx = to;
kref_init(&request->ref);
request->uniq = dev_priv->request_uniq++;
i915_gem_context_reference(request->ctx);
} else { } else {
i915_gem_request_reference(request);
WARN_ON(to != request->ctx); WARN_ON(to != request->ctx);
} }
request->tail = tail; request->tail = tail;
i915_gem_request_reference(request);
i915_gem_context_reference(request->ctx);
intel_runtime_pm_get(dev_priv); intel_runtime_pm_get(dev_priv);
...@@ -731,7 +732,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring) ...@@ -731,7 +732,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
if (ctx_obj && (ctx != ring->default_context)) if (ctx_obj && (ctx != ring->default_context))
intel_lr_context_unpin(ring, ctx); intel_lr_context_unpin(ring, ctx);
intel_runtime_pm_put(dev_priv); intel_runtime_pm_put(dev_priv);
i915_gem_context_unreference(ctx);
list_del(&req->execlist_link); list_del(&req->execlist_link);
i915_gem_request_unreference(req); i915_gem_request_unreference(req);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment