[Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
Jani Nikula
jani.nikula at linux.intel.com
Thu Feb 26 23:31:47 PST 2015
On Tue, 24 Feb 2015, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> On Mon, 23 Feb 2015, "Daniel, Thomas" <thomas.daniel at intel.com> wrote:
>>> -----Original Message-----
>>> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
>>> Nick Hoath
>>> Sent: Thursday, February 19, 2015 4:31 PM
>>> To: intel-gfx at 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 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.
>>>
>>> v3: Cleaned up the language used in the commit & the header
>>> description (Thanks David Gordon)
>>>
>>> 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,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 at 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 at 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
More information about the Intel-gfx
mailing list