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

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

On Tue, 24 Feb 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> 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.

Bah, QA just reopened the bug. Nick, please look into it.

https://bugs.freedesktop.org/show_bug.cgi?id=88652

BR,
Jani.



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

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