| Message ID | 1424363447-16296-1-git-send-email-nicholas.hoath@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 '*'
> -----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>
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
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
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); }
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(-)