On Tue, Mar 28, 2017 at 10:08:41PM -0400, Steven Rostedt wrote: > On Wed, 29 Mar 2017 10:46:22 +0900 > Namhyung Kim <[email protected]> wrote: > > > When function tracer has a pid filter, it adds a probe to sched_switch > > to track if current task can be ignored. The probe checks the > > ftrace_ignore_pid from current tr to filter tasks. But it misses to > > delete the probe when removing an instance so that it can cause a crash > > due to the invalid tr pointer (use-after-free). > > > > This is easily reproducible with the following: > > > > # cd /sys/kernel/debug/tracing > > # mkdir instances/buggy > > # echo $$ > instances/buggy/set_ftrace_pid > > # rmdir instances/buggy > > > > > > ============================================================================ > > BUG: KASAN: use-after-free in > > ftrace_filter_pid_sched_switch_probe+0x3d/0x90 > > Read of size 8 by task kworker/0:1/17 > > CPU: 0 PID: 17 Comm: kworker/0:1 Tainted: G B 4.11.0-rc3 > > #198 > > Call Trace: > > dump_stack+0x68/0x9f > > kasan_object_err+0x21/0x70 > > kasan_report.part.1+0x22b/0x500 > > ? ftrace_filter_pid_sched_switch_probe+0x3d/0x90 > > kasan_report+0x25/0x30 > > __asan_load8+0x5e/0x70 > > ftrace_filter_pid_sched_switch_probe+0x3d/0x90 > > ? fpid_start+0x130/0x130 > > __schedule+0x571/0xce0 > > ... > > > > To fix it, use ftrace_pid_reset() to unregister the probe. As > > instance_rmdir() already updated ftrace codes, it can just free the > > filter safely. > > > > Fixes: 0c8916c34203 ("tracing: Add rmdir to remove multibuffer instances") > > Signed-off-by: Namhyung Kim <[email protected]> > > --- > > kernel/trace/ftrace.c | 10 ++++++---- > > kernel/trace/trace.c | 1 + > > kernel/trace/trace.h | 2 ++ > > 3 files changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index 0556a202c055..b451a860e885 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -5598,13 +5598,15 @@ static void clear_ftrace_pids(struct trace_array > > *tr) > > trace_free_pid_list(pid_list); > > } > > > > -static void ftrace_pid_reset(struct trace_array *tr) > > +void ftrace_pid_reset(struct trace_array *tr, bool update) > > { > > mutex_lock(&ftrace_lock); > > clear_ftrace_pids(tr); > > > > - ftrace_update_pid_func(); > > - ftrace_startup_all(0); > > + if (update) { > > + ftrace_update_pid_func(); > > + ftrace_startup_all(0); > > + } > > > > mutex_unlock(&ftrace_lock); > > } > > I think it is better to create a new function here. I mean, you just > added a bool, that removes 2 thirds of the code when false.
Ah, missed this part. Ok, I'll make it a separate function. Thanks, Namhyung
