| Message ID | 20191109200823.15636-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Protect context while grabbing its name for the request | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Inside print_request(), we query the context/timeline name. Nothing > immediately protects the context from being freed if the request is > complete -- we rely on serialisation by the caller to keep the name > valid until they finish using it. Inside intel_engine_dump(), we > generally only print the requsts in the execution queue protected by the requests > engine->active.lock, but we also show the pending execlists ports which > are not protected and so require an rcu_read_lock to keep the pointer > valid. s/an/a ? > > [ 1695.700883] BUG: KASAN: use-after-free in i915_fence_get_timeline_name+0x53/0x90 [i915] > [ 1695.700981] Read of size 8 at addr ffff8887344f4d50 by task gem_ctx_persist/2968 > [ 1695.701068] > [ 1695.701156] CPU: 1 PID: 2968 Comm: gem_ctx_persist Tainted: G U 5.4.0-rc6+ #331 > [ 1695.701246] Hardware name: Intel Corporation NUC7i5BNK/NUC7i5BNB, BIOS BNKBL357.86A.0052.2017.0918.1346 09/18/2017 > [ 1695.701334] Call Trace: > [ 1695.701424] dump_stack+0x5b/0x90 > [ 1695.701870] ? i915_fence_get_timeline_name+0x53/0x90 [i915] > [ 1695.701964] print_address_description.constprop.7+0x36/0x50 > [ 1695.702408] ? i915_fence_get_timeline_name+0x53/0x90 [i915] > [ 1695.702856] ? i915_fence_get_timeline_name+0x53/0x90 [i915] > [ 1695.702947] __kasan_report.cold.10+0x1a/0x3a > [ 1695.703390] ? i915_fence_get_timeline_name+0x53/0x90 [i915] > [ 1695.703836] i915_fence_get_timeline_name+0x53/0x90 [i915] > [ 1695.704241] print_request+0x82/0x2e0 [i915] > [ 1695.704638] ? fwtable_read32+0x133/0x360 [i915] > [ 1695.705042] ? write_timestamp+0x110/0x110 [i915] > [ 1695.705133] ? _raw_spin_lock_irqsave+0x79/0xc0 > [ 1695.705221] ? refcount_inc_not_zero_checked+0x91/0x110 > [ 1695.705306] ? refcount_dec_and_mutex_lock+0x50/0x50 > [ 1695.705709] ? intel_engine_find_active_request+0x202/0x230 [i915] > [ 1695.706115] intel_engine_dump+0x2c9/0x900 [i915] > > Fixes: c36eebd9ba5d ("drm/i915/gt: execlists->active is serialised by the tasklet") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > index bf20305a1083..b9613d044393 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > @@ -1373,6 +1373,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, > } > > execlists_active_lock_bh(execlists); > + rcu_read_lock(); For me there is temptation to push the rcu protection into the active lock and remove the _bh from the name. Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > for (port = execlists->active; (rq = *port); port++) { > char hdr[80]; > int len; > @@ -1410,6 +1411,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, > if (tl) > intel_timeline_put(tl); > } > + rcu_read_unlock(); > execlists_active_unlock_bh(execlists); > } else if (INTEL_GEN(dev_priv) > 6) { > drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n", > -- > 2.24.0
Quoting Mika Kuoppala (2019-11-11 10:25:00) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Inside print_request(), we query the context/timeline name. Nothing > > immediately protects the context from being freed if the request is > > complete -- we rely on serialisation by the caller to keep the name > > valid until they finish using it. Inside intel_engine_dump(), we > > generally only print the requsts in the execution queue protected by the > requests > > > engine->active.lock, but we also show the pending execlists ports which > > are not protected and so require an rcu_read_lock to keep the pointer > > valid. > > s/an/a ? > > > > > [ 1695.700883] BUG: KASAN: use-after-free in i915_fence_get_timeline_name+0x53/0x90 [i915] > > [ 1695.700981] Read of size 8 at addr ffff8887344f4d50 by task gem_ctx_persist/2968 > > [ 1695.701068] > > [ 1695.701156] CPU: 1 PID: 2968 Comm: gem_ctx_persist Tainted: G U 5.4.0-rc6+ #331 > > [ 1695.701246] Hardware name: Intel Corporation NUC7i5BNK/NUC7i5BNB, BIOS BNKBL357.86A.0052.2017.0918.1346 09/18/2017 > > [ 1695.701334] Call Trace: > > [ 1695.701424] dump_stack+0x5b/0x90 > > [ 1695.701870] ? i915_fence_get_timeline_name+0x53/0x90 [i915] > > [ 1695.701964] print_address_description.constprop.7+0x36/0x50 > > [ 1695.702408] ? i915_fence_get_timeline_name+0x53/0x90 [i915] > > [ 1695.702856] ? i915_fence_get_timeline_name+0x53/0x90 [i915] > > [ 1695.702947] __kasan_report.cold.10+0x1a/0x3a > > [ 1695.703390] ? i915_fence_get_timeline_name+0x53/0x90 [i915] > > [ 1695.703836] i915_fence_get_timeline_name+0x53/0x90 [i915] > > [ 1695.704241] print_request+0x82/0x2e0 [i915] > > [ 1695.704638] ? fwtable_read32+0x133/0x360 [i915] > > [ 1695.705042] ? write_timestamp+0x110/0x110 [i915] > > [ 1695.705133] ? _raw_spin_lock_irqsave+0x79/0xc0 > > [ 1695.705221] ? refcount_inc_not_zero_checked+0x91/0x110 > > [ 1695.705306] ? refcount_dec_and_mutex_lock+0x50/0x50 > > [ 1695.705709] ? intel_engine_find_active_request+0x202/0x230 [i915] > > [ 1695.706115] intel_engine_dump+0x2c9/0x900 [i915] > > > > Fixes: c36eebd9ba5d ("drm/i915/gt: execlists->active is serialised by the tasklet") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > > index bf20305a1083..b9613d044393 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > > @@ -1373,6 +1373,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, > > } > > > > execlists_active_lock_bh(execlists); > > + rcu_read_lock(); > > For me there is temptation to push the rcu protection > into the active lock and remove the _bh from the name. Primary user is intel_enable_engine_stats; here in debug land our convenience is secondary :) execlists_active_lock_bh_rcu :-p -Chris
Quoting Mika Kuoppala (2019-11-11 10:25:00) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Inside print_request(), we query the context/timeline name. Nothing > > immediately protects the context from being freed if the request is > > complete -- we rely on serialisation by the caller to keep the name > > valid until they finish using it. Inside intel_engine_dump(), we > > generally only print the requsts in the execution queue protected by the > requests > > > engine->active.lock, but we also show the pending execlists ports which > > are not protected and so require an rcu_read_lock to keep the pointer > > valid. > > s/an/a ? Interesting. I used an because I pronounce it R C U read_lock, and since it then starts with the solitary R takes a vowel sound. English is bizarre. -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index bf20305a1083..b9613d044393 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -1373,6 +1373,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, } execlists_active_lock_bh(execlists); + rcu_read_lock(); for (port = execlists->active; (rq = *port); port++) { char hdr[80]; int len; @@ -1410,6 +1411,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, if (tl) intel_timeline_put(tl); } + rcu_read_unlock(); execlists_active_unlock_bh(execlists); } else if (INTEL_GEN(dev_priv) > 6) { drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
Inside print_request(), we query the context/timeline name. Nothing immediately protects the context from being freed if the request is complete -- we rely on serialisation by the caller to keep the name valid until they finish using it. Inside intel_engine_dump(), we generally only print the requsts in the execution queue protected by the engine->active.lock, but we also show the pending execlists ports which are not protected and so require an rcu_read_lock to keep the pointer valid. [ 1695.700883] BUG: KASAN: use-after-free in i915_fence_get_timeline_name+0x53/0x90 [i915] [ 1695.700981] Read of size 8 at addr ffff8887344f4d50 by task gem_ctx_persist/2968 [ 1695.701068] [ 1695.701156] CPU: 1 PID: 2968 Comm: gem_ctx_persist Tainted: G U 5.4.0-rc6+ #331 [ 1695.701246] Hardware name: Intel Corporation NUC7i5BNK/NUC7i5BNB, BIOS BNKBL357.86A.0052.2017.0918.1346 09/18/2017 [ 1695.701334] Call Trace: [ 1695.701424] dump_stack+0x5b/0x90 [ 1695.701870] ? i915_fence_get_timeline_name+0x53/0x90 [i915] [ 1695.701964] print_address_description.constprop.7+0x36/0x50 [ 1695.702408] ? i915_fence_get_timeline_name+0x53/0x90 [i915] [ 1695.702856] ? i915_fence_get_timeline_name+0x53/0x90 [i915] [ 1695.702947] __kasan_report.cold.10+0x1a/0x3a [ 1695.703390] ? i915_fence_get_timeline_name+0x53/0x90 [i915] [ 1695.703836] i915_fence_get_timeline_name+0x53/0x90 [i915] [ 1695.704241] print_request+0x82/0x2e0 [i915] [ 1695.704638] ? fwtable_read32+0x133/0x360 [i915] [ 1695.705042] ? write_timestamp+0x110/0x110 [i915] [ 1695.705133] ? _raw_spin_lock_irqsave+0x79/0xc0 [ 1695.705221] ? refcount_inc_not_zero_checked+0x91/0x110 [ 1695.705306] ? refcount_dec_and_mutex_lock+0x50/0x50 [ 1695.705709] ? intel_engine_find_active_request+0x202/0x230 [i915] [ 1695.706115] intel_engine_dump+0x2c9/0x900 [i915] Fixes: c36eebd9ba5d ("drm/i915/gt: execlists->active is serialised by the tasklet") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 ++ 1 file changed, 2 insertions(+)