[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 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 AuthorDate: Thu Jan 15 13:10:39 2015 + drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request Signed-off-by: Nick Hoath --- 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
[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 AuthorDate: Thu Jan 15 13:10:39 2015 + 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 --- 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
[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 AuthorDate: Thu Jan 15 13:10:39 2015 + 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 --- 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);
Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
On Fri, Feb 13, 2015 at 01:30:35PM +, 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 > AuthorDate: Thu Jan 15 13:10:39 2015 + > > 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 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
Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@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 Testdrm-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: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
> -Original Message- > From: Intel-gfx [mailto:intel-gfx-boun...@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 +, 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 > > AuthorDate: Thu Jan 15 13:10:39 2015 + > > > > 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 > > 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: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@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 Testdrm-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: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
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 > AuthorDate: Thu Jan 15 13:10:39 2015 + > > 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 > --- > 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_reque
Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
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 AuthorDate: Thu Jan 15 13:10:39 2015 + 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 --- 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_
Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@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 Testdrm-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
Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
> -Original Message- > From: Intel-gfx [mailto:intel-gfx-boun...@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 > AuthorDate: Thu Jan 15 13:10:39 2015 + > > 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 > --- > 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; >
Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
On Mon, 23 Feb 2015, "Daniel, Thomas" wrote: >> -Original Message- >> From: Intel-gfx [mailto:intel-gfx-boun...@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 >> AuthorDate: Thu Jan 15 13:10:39 2015 + >> >> 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 >> --- >> 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, >> *
Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting
On Tue, 24 Feb 2015, Jani Nikula wrote: > On Mon, 23 Feb 2015, "Daniel, Thomas" wrote: >>> -Original Message- >>> From: Intel-gfx [mailto:intel-gfx-boun...@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 >>> AuthorDate: Thu Jan 15 13:10:39 2015 + >>> >>> 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 >>> --- >>> 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); >>> + i91