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