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

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

[Nick Hoath]

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.

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.
+	 */
 	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

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

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5790
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -7              277/277              270/277
ILK                                  313/313              313/313
SNB                                  309/309              309/309
IVB                                  382/382              382/382
BYT                                  296/296              296/296
HSW                                  425/425              425/425
BDW                 -1              318/318              317/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_fence_thrash_bo-write-verify-none      NRUN(1)PASS(4)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-x      PASS(5)      FAIL(1)PASS(1)
*PNV  igt_gem_fence_thrash_bo-write-verify-y      NO_RESULT(1)PASS(4)      FAIL(1)PASS(1)
 PNV  igt_gem_userptr_blits_coherency-sync      NO_RESULT(1)CRASH(2)PASS(3)      CRASH(2)
*PNV  igt_gem_userptr_blits_coherency-unsync      CRASH(2)PASS(3)      NRUN(1)PASS(1)
*PNV  igt_gem_userptr_blits_create-destroy-sync      PASS(2)      NRUN(1)PASS(1)
*PNV  igt_gem_userptr_blits_dmabuf-unsync      PASS(2)      NRUN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(9)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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.

.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

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

[Nick Hoath]

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

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

[Jani Nikula]

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

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

[Jani Nikula]

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

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

[Daniel, Thomas]

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

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

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

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5797
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              277/277              275/277
ILK                                  313/313              313/313
SNB                                  309/309              309/309
IVB                                  382/382              382/382
BYT                                  296/296              296/296
HSW                                  425/425              425/425
BDW                 -1              318/318              317/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_fence_thrash_bo-write-verify-y      NO_RESULT(1)PASS(6)      FAIL(1)PASS(1)
 PNV  igt_gem_userptr_blits_coherency-sync      NO_RESULT(1)CRASH(5)PASS(6)      CRASH(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(17)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Nick Hoath]

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

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

[Daniel, Thomas]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Daniel Vetter
> Sent: Friday, February 13, 2015 1:50 PM
> To: Hoath, Nicholas
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced
> refcounting
> 
> On Fri, Feb 13, 2015 at 01:30:35PM +0000, 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 free's of requests were replaced with unrefs, and extraneous
> > refs/unrefs of contexts were added.
> > Correct the unbalanced refcount & replace the free's.
> >
> > 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
> 
> Imo the commit message should be ammended with a short paragraph explainig
> the various pointers and implied and explicit references we now have
> around requests and contexts. That way review of this will get a bit
> easier and we'll avoid another misunderstanding.
> 
> I even think we should add a comment in the header to request.ctx to
> explain the rules since apparently they've not been fully clear.
Agree that more documentation around these ctx refs would be good to have to clear up confusion.
For example, I initially thought that this patch introduced a new use-after-free because of the removal of the ctx ref in execlists_context_queue().

> 
> > Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
> 
> But yeah this makes a lot more sense imo. Please feed this to QA for
> stress-testing in all the relevant bugs. Today I have my head full with
> kms code so not a good time for a full in-depth review. But I think it'd
> be good if other people take a look anyway, so please throw this at a few
> ppl from the vpg core team too.
I guess that would be me...
The code changes look OK, would like to see the updated comments and QA results.

Cheers,
Thomas.

> Thanks, Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c  | 3 +--
> >  drivers/gpu/drm/i915/intel_lrc.c | 3 +--
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> > index 1765989..79e48b2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2660,8 +2660,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..a18925d 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -518,12 +518,12 @@ static int execlists_context_queue(struct
> intel_engine_cs *ring,
> >  			return -ENOMEM;
> >  		request->ring = ring;
> >  		request->ctx = to;
> > +		i915_gem_context_reference(request->ctx);
> >  	} else {
> >  		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 +740,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
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5771
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  281/281              281/281
ILK                                  313/313              313/313
SNB                                  309/309              309/309
IVB                                  382/382              382/382
BYT                                  296/296              296/296
HSW                 -1              426/426              425/426
BDW                 -1              318/318              317/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*HSW  igt_kms_flip_dpms-off-confusion      PASS(1)      TIMEOUT(1)
*BDW  igt_gem_gtt_hog      PASS(1)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Fri, Feb 13, 2015 at 01:30:35PM +0000, 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 free's of requests were replaced with unrefs, and extraneous
> refs/unrefs of contexts were added.
> Correct the unbalanced refcount & replace the free's.
> 
> 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

Imo the commit message should be ammended with a short paragraph explainig
the various pointers and implied and explicit references we now have
around requests and contexts. That way review of this will get a bit
easier and we'll avoid another misunderstanding.

I even think we should add a comment in the header to request.ctx to
explain the rules since apparently they've not been fully clear.

> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>

But yeah this makes a lot more sense imo. Please feed this to QA for
stress-testing in all the relevant bugs. Today I have my head full with
kms code so not a good time for a full in-depth review. But I think it'd
be good if other people take a look anyway, so please throw this at a few
ppl from the vpg core team too.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c  | 3 +--
>  drivers/gpu/drm/i915/intel_lrc.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1765989..79e48b2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2660,8 +2660,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..a18925d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -518,12 +518,12 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
>  			return -ENOMEM;
>  		request->ring = ring;
>  		request->ctx = to;
> +		i915_gem_context_reference(request->ctx);
>  	} else {
>  		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 +740,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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Nick Hoath]

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

When converting from implicitly tracked execlist queue items to ref counted
requests, not all free's of requests were replaced with unrefs, and extraneous
refs/unrefs of contexts were added.
Correct the unbalanced refcount & replace the free's.

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

Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c  | 3 +--
 drivers/gpu/drm/i915/intel_lrc.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1765989..79e48b2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2660,8 +2660,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..a18925d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -518,12 +518,12 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
 			return -ENOMEM;
 		request->ring = ring;
 		request->ctx = to;
+		i915_gem_context_reference(request->ctx);
 	} else {
 		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 +740,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