drm/i915: Protect context while grabbing its name for the request
diff mbox series

Message ID 20191109200823.15636-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915: Protect context while grabbing its name for the request
Related show

Commit Message

Chris Wilson Nov. 9, 2019, 8:08 p.m. UTC
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(+)

Comments

Mika Kuoppala Nov. 11, 2019, 10:25 a.m. UTC | #1
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
Chris Wilson Nov. 11, 2019, 10:48 a.m. UTC | #2
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
Chris Wilson Nov. 11, 2019, 10:51 a.m. UTC | #3
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

Patch
diff mbox series

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",