| Message ID | 20170316204235.27786-1-chris@chris-wilson.co.uk |
|---|---|
| State | Accepted |
| Commit | 60367132a21449c2119f0bb27eef907bc95828af |
| Headers | show |
| Series |
"drm/i915: Avoid use-after-free of ctx in request tracepoints"
( rev:
1
)
in
Intel GFX |
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 5503f5ab1e98..66404c5aee82 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -590,7 +590,7 @@ TRACE_EVENT(i915_gem_request_queue, TP_fast_assign( __entry->dev = req->i915->drm.primary->index; __entry->ring = req->engine->id; - __entry->ctx = req->ctx->hw_id; + __entry->ctx = req->fence.context; __entry->seqno = req->fence.seqno; __entry->flags = flags; ), @@ -637,8 +637,8 @@ DECLARE_EVENT_CLASS(i915_gem_request, TP_fast_assign( __entry->dev = req->i915->drm.primary->index; - __entry->ctx = req->ctx->hw_id; __entry->ring = req->engine->id; + __entry->ctx = req->fence.context; __entry->seqno = req->fence.seqno; __entry->global = req->global_seqno; ), @@ -681,7 +681,7 @@ DECLARE_EVENT_CLASS(i915_gem_request_hw, TP_fast_assign( __entry->dev = req->i915->drm.primary->index; __entry->ring = req->engine->id; - __entry->ctx = req->ctx->hw_id; + __entry->ctx = req->fence.context; __entry->seqno = req->fence.seqno; __entry->global_seqno = req->global_seqno; __entry->port = port; @@ -776,7 +776,7 @@ TRACE_EVENT(i915_gem_request_wait_begin, TP_fast_assign( __entry->dev = req->i915->drm.primary->index; __entry->ring = req->engine->id; - __entry->ctx = req->ctx->hw_id; + __entry->ctx = req->fence.context; __entry->seqno = req->fence.seqno; __entry->global = req->global_seqno; __entry->flags = flags;
On 16/03/2017 20:42, Chris Wilson wrote: > trace_i915_gem_request_out may be used after the request is completed, > and so the request may have been retired on another thread, invalidating > the rq->ctx. Avoid dereferencing rq->ctx in the tracepoint by switching > to the fence context id instead, updating all tracepoints to match. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_trace.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > index 5503f5ab1e98..66404c5aee82 100644 > --- a/drivers/gpu/drm/i915/i915_trace.h > +++ b/drivers/gpu/drm/i915/i915_trace.h > @@ -590,7 +590,7 @@ TRACE_EVENT(i915_gem_request_queue, > TP_fast_assign( > __entry->dev = req->i915->drm.primary->index; > __entry->ring = req->engine->id; > - __entry->ctx = req->ctx->hw_id; > + __entry->ctx = req->fence.context; > __entry->seqno = req->fence.seqno; > __entry->flags = flags; > ), > @@ -637,8 +637,8 @@ DECLARE_EVENT_CLASS(i915_gem_request, > > TP_fast_assign( > __entry->dev = req->i915->drm.primary->index; > - __entry->ctx = req->ctx->hw_id; > __entry->ring = req->engine->id; > + __entry->ctx = req->fence.context; > __entry->seqno = req->fence.seqno; > __entry->global = req->global_seqno; > ), > @@ -681,7 +681,7 @@ DECLARE_EVENT_CLASS(i915_gem_request_hw, > TP_fast_assign( > __entry->dev = req->i915->drm.primary->index; > __entry->ring = req->engine->id; > - __entry->ctx = req->ctx->hw_id; > + __entry->ctx = req->fence.context; > __entry->seqno = req->fence.seqno; > __entry->global_seqno = req->global_seqno; > __entry->port = port; > @@ -776,7 +776,7 @@ TRACE_EVENT(i915_gem_request_wait_begin, > TP_fast_assign( > __entry->dev = req->i915->drm.primary->index; > __entry->ring = req->engine->id; > - __entry->ctx = req->ctx->hw_id; > + __entry->ctx = req->fence.context; > __entry->seqno = req->fence.seqno; > __entry->global = req->global_seqno; > __entry->flags = flags; > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> req->engine is fine? No chance of request re-use in the window? Regards, Tvrtko
On Fri, Mar 17, 2017 at 07:47:23AM +0000, Tvrtko Ursulin wrote: > > On 16/03/2017 20:42, Chris Wilson wrote: > >trace_i915_gem_request_out may be used after the request is completed, > >and so the request may have been retired on another thread, invalidating > >the rq->ctx. Avoid dereferencing rq->ctx in the tracepoint by switching > >to the fence context id instead, updating all tracepoints to match. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >--- > > drivers/gpu/drm/i915/i915_trace.h | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > >index 5503f5ab1e98..66404c5aee82 100644 > >--- a/drivers/gpu/drm/i915/i915_trace.h > >+++ b/drivers/gpu/drm/i915/i915_trace.h > >@@ -590,7 +590,7 @@ TRACE_EVENT(i915_gem_request_queue, > > TP_fast_assign( > > __entry->dev = req->i915->drm.primary->index; > > __entry->ring = req->engine->id; > >- __entry->ctx = req->ctx->hw_id; > >+ __entry->ctx = req->fence.context; > > __entry->seqno = req->fence.seqno; > > __entry->flags = flags; > > ), > >@@ -637,8 +637,8 @@ DECLARE_EVENT_CLASS(i915_gem_request, > > > > TP_fast_assign( > > __entry->dev = req->i915->drm.primary->index; > >- __entry->ctx = req->ctx->hw_id; > > __entry->ring = req->engine->id; > >+ __entry->ctx = req->fence.context; > > __entry->seqno = req->fence.seqno; > > __entry->global = req->global_seqno; > > ), > >@@ -681,7 +681,7 @@ DECLARE_EVENT_CLASS(i915_gem_request_hw, > > TP_fast_assign( > > __entry->dev = req->i915->drm.primary->index; > > __entry->ring = req->engine->id; > >- __entry->ctx = req->ctx->hw_id; > >+ __entry->ctx = req->fence.context; > > __entry->seqno = req->fence.seqno; > > __entry->global_seqno = req->global_seqno; > > __entry->port = port; > >@@ -776,7 +776,7 @@ TRACE_EVENT(i915_gem_request_wait_begin, > > TP_fast_assign( > > __entry->dev = req->i915->drm.primary->index; > > __entry->ring = req->engine->id; > >- __entry->ctx = req->ctx->hw_id; > >+ __entry->ctx = req->fence.context; > > __entry->seqno = req->fence.seqno; > > __entry->global = req->global_seqno; > > __entry->flags = flags; > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > req->engine is fine? No chance of request re-use in the window? We still hold a reference on the rq, so we don't need to play rcu games to acquire a stable rq for the tracepoint. We only need to worry about objects whose lifetime is coupled to rq retirement. engine/i915 outlive all requests. -Chris
On 17/03/2017 07:56, Chris Wilson wrote: > On Fri, Mar 17, 2017 at 07:47:23AM +0000, Tvrtko Ursulin wrote: >> >> On 16/03/2017 20:42, Chris Wilson wrote: >>> trace_i915_gem_request_out may be used after the request is completed, >>> and so the request may have been retired on another thread, invalidating >>> the rq->ctx. Avoid dereferencing rq->ctx in the tracepoint by switching >>> to the fence context id instead, updating all tracepoints to match. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_trace.h | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h >>> index 5503f5ab1e98..66404c5aee82 100644 >>> --- a/drivers/gpu/drm/i915/i915_trace.h >>> +++ b/drivers/gpu/drm/i915/i915_trace.h >>> @@ -590,7 +590,7 @@ TRACE_EVENT(i915_gem_request_queue, >>> TP_fast_assign( >>> __entry->dev = req->i915->drm.primary->index; >>> __entry->ring = req->engine->id; >>> - __entry->ctx = req->ctx->hw_id; >>> + __entry->ctx = req->fence.context; >>> __entry->seqno = req->fence.seqno; >>> __entry->flags = flags; >>> ), >>> @@ -637,8 +637,8 @@ DECLARE_EVENT_CLASS(i915_gem_request, >>> >>> TP_fast_assign( >>> __entry->dev = req->i915->drm.primary->index; >>> - __entry->ctx = req->ctx->hw_id; >>> __entry->ring = req->engine->id; >>> + __entry->ctx = req->fence.context; >>> __entry->seqno = req->fence.seqno; >>> __entry->global = req->global_seqno; >>> ), >>> @@ -681,7 +681,7 @@ DECLARE_EVENT_CLASS(i915_gem_request_hw, >>> TP_fast_assign( >>> __entry->dev = req->i915->drm.primary->index; >>> __entry->ring = req->engine->id; >>> - __entry->ctx = req->ctx->hw_id; >>> + __entry->ctx = req->fence.context; >>> __entry->seqno = req->fence.seqno; >>> __entry->global_seqno = req->global_seqno; >>> __entry->port = port; >>> @@ -776,7 +776,7 @@ TRACE_EVENT(i915_gem_request_wait_begin, >>> TP_fast_assign( >>> __entry->dev = req->i915->drm.primary->index; >>> __entry->ring = req->engine->id; >>> - __entry->ctx = req->ctx->hw_id; >>> + __entry->ctx = req->fence.context; >>> __entry->seqno = req->fence.seqno; >>> __entry->global = req->global_seqno; >>> __entry->flags = flags; >>> >> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> req->engine is fine? No chance of request re-use in the window? > > We still hold a reference on the rq, so we don't need to play rcu games > to acquire a stable rq for the tracepoint. We only need to worry about > objects whose lifetime is coupled to rq retirement. engine/i915 outlive > all requests. Yes I know, but I thought the problem was request re-cycling. Becuase how does context go away, dont' we have it pinned until the following request completes any longer? (To avoid the unpin during context save.) Regards, Tvrtko
On Fri, Mar 17, 2017 at 08:05:47AM +0000, Tvrtko Ursulin wrote: > > On 17/03/2017 07:56, Chris Wilson wrote: > >On Fri, Mar 17, 2017 at 07:47:23AM +0000, Tvrtko Ursulin wrote: > >> > >>On 16/03/2017 20:42, Chris Wilson wrote: > >>>trace_i915_gem_request_out may be used after the request is completed, > >>>and so the request may have been retired on another thread, invalidating > >>>the rq->ctx. Avoid dereferencing rq->ctx in the tracepoint by switching > >>>to the fence context id instead, updating all tracepoints to match. > >>> > >>>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>--- > >>>drivers/gpu/drm/i915/i915_trace.h | 8 ++++---- > >>>1 file changed, 4 insertions(+), 4 deletions(-) > >>> > >>>diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > >>>index 5503f5ab1e98..66404c5aee82 100644 > >>>--- a/drivers/gpu/drm/i915/i915_trace.h > >>>+++ b/drivers/gpu/drm/i915/i915_trace.h > >>>@@ -590,7 +590,7 @@ TRACE_EVENT(i915_gem_request_queue, > >>> TP_fast_assign( > >>> __entry->dev = req->i915->drm.primary->index; > >>> __entry->ring = req->engine->id; > >>>- __entry->ctx = req->ctx->hw_id; > >>>+ __entry->ctx = req->fence.context; > >>> __entry->seqno = req->fence.seqno; > >>> __entry->flags = flags; > >>> ), > >>>@@ -637,8 +637,8 @@ DECLARE_EVENT_CLASS(i915_gem_request, > >>> > >>> TP_fast_assign( > >>> __entry->dev = req->i915->drm.primary->index; > >>>- __entry->ctx = req->ctx->hw_id; > >>> __entry->ring = req->engine->id; > >>>+ __entry->ctx = req->fence.context; > >>> __entry->seqno = req->fence.seqno; > >>> __entry->global = req->global_seqno; > >>> ), > >>>@@ -681,7 +681,7 @@ DECLARE_EVENT_CLASS(i915_gem_request_hw, > >>> TP_fast_assign( > >>> __entry->dev = req->i915->drm.primary->index; > >>> __entry->ring = req->engine->id; > >>>- __entry->ctx = req->ctx->hw_id; > >>>+ __entry->ctx = req->fence.context; > >>> __entry->seqno = req->fence.seqno; > >>> __entry->global_seqno = req->global_seqno; > >>> __entry->port = port; > >>>@@ -776,7 +776,7 @@ TRACE_EVENT(i915_gem_request_wait_begin, > >>> TP_fast_assign( > >>> __entry->dev = req->i915->drm.primary->index; > >>> __entry->ring = req->engine->id; > >>>- __entry->ctx = req->ctx->hw_id; > >>>+ __entry->ctx = req->fence.context; > >>> __entry->seqno = req->fence.seqno; > >>> __entry->global = req->global_seqno; > >>> __entry->flags = flags; > >>> > >> > >>Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>req->engine is fine? No chance of request re-use in the window? > > > >We still hold a reference on the rq, so we don't need to play rcu games > >to acquire a stable rq for the tracepoint. We only need to worry about > >objects whose lifetime is coupled to rq retirement. engine/i915 outlive > >all requests. > > Yes I know, but I thought the problem was request re-cycling. > Becuase how does context go away, dont' we have it pinned until the > following request completes any longer? (To avoid the unpin during > context save.) Correct it is pinned and referenced until the next request/context retires. That just makes the race less likely... Didn't stop CI from hitting it 10% of the time :) -Chris
On Thu, Mar 16, 2017 at 09:01:11PM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Avoid use-after-free of ctx in request tracepoints > URL : https://patchwork.freedesktop.org/series/21398/ > State : success > > == Summary == > > Series 21398v1 drm/i915: Avoid use-after-free of ctx in request tracepoints > https://patchwork.freedesktop.org/api/1.0/series/21398/revisions/1/mbox/ > > Test gem_busy: > Subgroup basic-hang-default: > pass -> FAIL (fi-hsw-4770r) fdo#100046 > Test kms_pipe_crc_basic: > Subgroup read-crc-pipe-a: > dmesg-warn -> PASS (fi-byt-n2820) > > fdo#100046 https://bugs.freedesktop.org/show_bug.cgi?id=100046 Thanks for the review, pushed to keep exercising CI and chalking up green. -Chris
trace_i915_gem_request_out may be used after the request is completed, and so the request may have been retired on another thread, invalidating the rq->ctx. Avoid dereferencing rq->ctx in the tracepoint by switching to the fence context id instead, updating all tracepoints to match. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_trace.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)