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

* [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
@ 2015-02-19 16:30 Nick Hoath
  2015-02-20  7:16 ` shuang.he
  2015-02-23 14:10 ` Daniel, Thomas
  0 siblings, 2 replies; 13+ messages in thread
From: Nick Hoath @ 2015-02-19 16:30 UTC (permalink / raw)
  To: intel-gfx

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@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)

Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
---
 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread