Hi Steve, On Tue, Mar 28, 2017 at 10:08:41PM -0400, Steven Rostedt wrote: > On Wed, 29 Mar 2017 10:46:22 +0900 > Namhyung Kim <namhy...@kernel.org> 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 <namhy...@kernel.org> > > --- > > 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. > > > > @@ -5676,7 +5678,7 @@ ftrace_pid_open(struct inode *inode, struct file > > *file) > > > > if ((file->f_mode & FMODE_WRITE) && > > (file->f_flags & O_TRUNC)) > > - ftrace_pid_reset(tr); > > + ftrace_pid_reset(tr, true); > > > > ret = seq_open(file, &ftrace_pid_sops); > > if (ret < 0) { > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index b5d4b80f2d45..b92489dfa829 100644 > > --- a/kernel/trace/trace.c > > +++ b/kernel/trace/trace.c > > @@ -7485,6 +7485,7 @@ static int instance_rmdir(const char *name) > > > > tracing_set_nop(tr); > > event_trace_del_tracer(tr); > > + ftrace_pid_reset(tr, false); > > Actually, if this is called after event_trace_del_tracer(), the tr is > already invisible and nothing new should change.
I don't follow. After event_trace_del_tracer(), the tr is invisible from the probe of event tracing but still is visible from the probe of function tracing, right? > > Make a wrapper around clear_ftrace_pids() and call that instead. We > don't even need to take a lock, but as I see there's a lockdep test for > ftrace_lock, we should still do so just to be safe. Right, that's why I call ftrace_pid_reset() instead of clear_ftrace_pids(). So do you prefer adding a new wrapper like below rather than reusing ftrace_pid_reset() with a new argument? Thanks, Namhyung > > void ftrace_clear_pids(struct trace_array *tr) > { > mutex_lock(&ftrace_lock); > > clear_ftrace_pids(tr); > > mutex_unlock(&ftrace_lock); > } > > -- Steve > > > > ftrace_destroy_function_files(tr); > > tracefs_remove_recursive(tr->dir); > > free_trace_buffers(tr); > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > > index 571acee52a32..4d9804fd9a2d 100644 > > --- a/kernel/trace/trace.h > > +++ b/kernel/trace/trace.h > > @@ -897,6 +897,7 @@ void ftrace_init_tracefs(struct trace_array *tr, struct > > dentry *d_tracer); > > void ftrace_init_tracefs_toplevel(struct trace_array *tr, > > struct dentry *d_tracer); > > int init_function_trace(void); > > +void ftrace_pid_reset(struct trace_array *tr, bool update); > > #else > > static inline int ftrace_trace_task(struct trace_array *tr) > > { > > @@ -916,6 +917,7 @@ static inline void ftrace_reset_array_ops(struct > > trace_array *tr) { } > > static inline void ftrace_init_tracefs(struct trace_array *tr, struct > > dentry *d) { } > > static inline void ftrace_init_tracefs_toplevel(struct trace_array *tr, > > struct dentry *d) { } > > static inline int init_function_trace(void) { return 0; } > > +static inline void ftrace_pid_reset(struct trace_array *tr, bool update) { > > } > > /* ftace_func_t type is not defined, use macro instead of static inline */ > > #define ftrace_init_array_ops(tr, func) do { } while (0) > > #endif /* CONFIG_FUNCTION_TRACER */ >