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

[Dave Gordon]

On 18/02/15 14:01, Nick Hoath wrote:
> 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

s/i915_gem_request_reference/i915_gem_request_free/ ?

> the request is freed.
> 
> Problem introduced in:
> commit 6d3d8274bc45de4babb62d64562d92af984dd238
> Author:     Nick Hoath <nicholas.hoath at 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.
> 
> Signed-off-by: Nick Hoath <nicholas.hoath at 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,13 @@ 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
> +	 * intel_context is reference counted, so on attachment to this
> +	 * request, the context should be i915_gem_context_reference'd.
> +	 * When this request has its reference count cleared, the cleanup
> +	 * code in i915_gem_request_free will deference the context.

s/deference/unreference/

Or maybe replace the comment with this version, if you think it's nicer:

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.

.Dave.

> +	 */
>  	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);
>  	}
>