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

[Nick Hoath <nicholas.hoath@intel.com>]

On 19/02/2015 11:23, Gordon, David S wrote:
> 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/ ?

I meant i915_gem_request_unreference, but should probably mention that 
i915_gem_request_free is called when the request reference count reaches 
zero, and it is that function which drops the context reference count.

>
>> 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.
>>
>> 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,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.

That's a lot more understandable than my version :)

>
> .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);
>>   	}
>>
>

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