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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 23 Feb 2015, "Daniel, Thomas" <thomas.daniel@intel.com> wrote:
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>> Nick Hoath
>> Sent: Thursday, February 19, 2015 4:31 PM
>> To: intel-gfx@lists.freedesktop.org
>> 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 <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
> Reviewed-by: Thomas Daniel <thomas.daniel@intel.com>

Pushed to drm-intel-fixes, with additional r-b from Daniel, thanks for
the patch and review.

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx