[PATCH 0/4] ftrace: Add 'function-fork' trace option (v2)

[PATCH 0/4] ftrace: Add 'function-fork' trace option (v2)

[Namhyung Kim]

Hello,

This patchset add 'function-fork' option to function tracer which
makes pid filter to be inherited like 'event-fork' does.  During the
test, I found a bug of pid filter on an instance directory.  The patch
1 fixes it and maybe it should go to the stable tree.

The function-fork option is disabled by default as event-fork does,
but we might consider changing the default since it seems to be more
natural from an user's perspective IMHO.

v2 changes)
 * use ftrace_clear_pids()  (Steve)
 * add Ack from Masami
 
The code is also available at 'ftrace/function-fork-v2' branch on

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Namhyung Kim (4):
  ftrace: Fix function pid filter on instances
  ftrace: Add 'function-fork' trace option
  selftests: ftrace: Add -l/--logdir option
  selftests: ftrace: Add a testcase for function PID filter

 kernel/trace/ftrace.c                              | 46 ++++++++++
 kernel/trace/trace.c                               |  6 +-
 kernel/trace/trace.h                               |  8 +-
 tools/testing/selftests/ftrace/ftracetest          |  5 ++
 .../ftrace/test.d/ftrace/func-filter-pid.tc        | 98 ++++++++++++++++++++++
 5 files changed, 161 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc

-- 
2.12.2

[PATCH 1/4] ftrace: Fix function pid filter on instances

[Namhyung Kim]

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 <namhyung@kernel.org>
---
 kernel/trace/ftrace.c | 9 +++++++++
 kernel/trace/trace.c  | 1 +
 kernel/trace/trace.h  | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 34f63e78d661..7b27066c5ed0 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5610,6 +5610,15 @@ static void clear_ftrace_pids(struct trace_array *tr)
 	trace_free_pid_list(pid_list);
 }
 
+void ftrace_clear_pids(struct trace_array *tr)
+{
+	mutex_lock(&ftrace_lock);
+
+	clear_ftrace_pids(tr);
+
+	mutex_unlock(&ftrace_lock);
+}
+
 static void ftrace_pid_reset(struct trace_array *tr)
 {
 	mutex_lock(&ftrace_lock);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b5d4b80f2d45..0bf623c182da 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_clear_pids(tr);
 	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..ee27163b7549 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_clear_pids(struct trace_array *tr);
 #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_clear_pids(struct trace_array *tr) { }
 /* 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 */
-- 
2.12.2

[PATCH 2/4] ftrace: Add 'function-fork' trace option

[Namhyung Kim]

The function-fork option is same as event-fork that it tracks task
fork/exit and set the pid filter properly.  This can be useful if user
wants to trace selected tasks including their children only.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 kernel/trace/ftrace.c | 37 +++++++++++++++++++++++++++++++++++++
 kernel/trace/trace.c  |  5 ++++-
 kernel/trace/trace.h  |  6 +++++-
 3 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 7b27066c5ed0..eb0303f8afa0 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5587,6 +5587,43 @@ ftrace_filter_pid_sched_switch_probe(void *data, bool preempt,
 		       trace_ignore_this_task(pid_list, next));
 }
 
+static void
+ftrace_pid_follow_sched_process_fork(void *data,
+				     struct task_struct *self,
+				     struct task_struct *task)
+{
+	struct trace_pid_list *pid_list;
+	struct trace_array *tr = data;
+
+	pid_list = rcu_dereference_sched(tr->function_pids);
+	trace_filter_add_remove_task(pid_list, self, task);
+}
+
+static void
+ftrace_pid_follow_sched_process_exit(void *data, struct task_struct *task)
+{
+	struct trace_pid_list *pid_list;
+	struct trace_array *tr = data;
+
+	pid_list = rcu_dereference_sched(tr->function_pids);
+	trace_filter_add_remove_task(pid_list, NULL, task);
+}
+
+void ftrace_pid_follow_fork(struct trace_array *tr, bool enable)
+{
+	if (enable) {
+		register_trace_sched_process_fork(ftrace_pid_follow_sched_process_fork,
+						  tr);
+		register_trace_sched_process_exit(ftrace_pid_follow_sched_process_exit,
+						  tr);
+	} else {
+		unregister_trace_sched_process_fork(ftrace_pid_follow_sched_process_fork,
+						    tr);
+		unregister_trace_sched_process_exit(ftrace_pid_follow_sched_process_exit,
+						    tr);
+	}
+}
+
 static void clear_ftrace_pids(struct trace_array *tr)
 {
 	struct trace_pid_list *pid_list;
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0bf623c182da..c008bf0f9f93 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -257,7 +257,7 @@ unsigned long long ns2usecs(u64 nsec)
 
 /* trace_flags that are default zero for instances */
 #define ZEROED_TRACE_FLAGS \
-	TRACE_ITER_EVENT_FORK
+	(TRACE_ITER_EVENT_FORK | TRACE_ITER_FUNC_FORK)
 
 /*
  * The global_trace is the descriptor that holds the top-level tracing
@@ -4205,6 +4205,9 @@ int set_tracer_flag(struct trace_array *tr, unsigned int mask, int enabled)
 	if (mask == TRACE_ITER_EVENT_FORK)
 		trace_event_follow_fork(tr, enabled);
 
+	if (mask == TRACE_ITER_FUNC_FORK)
+		ftrace_pid_follow_fork(tr, enabled);
+
 	if (mask == TRACE_ITER_OVERWRITE) {
 		ring_buffer_change_overwrite(tr->trace_buffer.buffer, enabled);
 #ifdef CONFIG_TRACER_MAX_TRACE
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index ee27163b7549..7dedf9cf0a6d 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -898,6 +898,7 @@ void ftrace_init_tracefs_toplevel(struct trace_array *tr,
 				  struct dentry *d_tracer);
 int init_function_trace(void);
 void ftrace_clear_pids(struct trace_array *tr);
+void ftrace_pid_follow_fork(struct trace_array *tr, bool enable);
 #else
 static inline int ftrace_trace_task(struct trace_array *tr)
 {
@@ -918,6 +919,7 @@ 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_clear_pids(struct trace_array *tr) { }
+static inline void ftrace_pid_follow_fork(struct trace_array *tr, bool enable) { }
 /* 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 */
@@ -991,11 +993,13 @@ extern int trace_get_user(struct trace_parser *parser, const char __user *ubuf,
 
 #ifdef CONFIG_FUNCTION_TRACER
 # define FUNCTION_FLAGS						\
-		C(FUNCTION,		"function-trace"),
+		C(FUNCTION,		"function-trace"),	\
+		C(FUNC_FORK,		"function-fork"),
 # define FUNCTION_DEFAULT_FLAGS		TRACE_ITER_FUNCTION
 #else
 # define FUNCTION_FLAGS
 # define FUNCTION_DEFAULT_FLAGS		0UL
+# define TRACE_ITER_FUNC_FORK		0UL
 #endif
 
 #ifdef CONFIG_STACKTRACE
-- 
2.12.2

[PATCH 3/4] selftests: ftrace: Add -l/--logdir option

[Namhyung Kim]

In my virtual machine setup, running ftracetest failed on creating
LOG_DIR on a read-only filesystem.  It'd be convenient to provide an
option to specify a different directory as log directory.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/testing/selftests/ftrace/ftracetest | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/ftrace/ftracetest b/tools/testing/selftests/ftrace/ftracetest
index 52e3c4df28d6..a8631d978725 100755
--- a/tools/testing/selftests/ftrace/ftracetest
+++ b/tools/testing/selftests/ftrace/ftracetest
@@ -16,6 +16,7 @@ echo "		-k|--keep  Keep passed test logs"
 echo "		-v|--verbose Increase verbosity of test messages"
 echo "		-vv        Alias of -v -v (Show all results in stdout)"
 echo "		-d|--debug Debug mode (trace all shell commands)"
+echo "		-l|--logdir <dir> Save logs on the <dir>"
 exit $1
 }
 
@@ -64,6 +65,10 @@ parse_opts() { # opts
       DEBUG=1
       shift 1
     ;;
+    --logdir|-l)
+      LOG_DIR=$2
+      shift 2
+    ;;
     *.tc)
       if [ -f "$1" ]; then
         OPT_TEST_CASES="$OPT_TEST_CASES `abspath $1`"
-- 
2.12.2

[PATCH 4/4] selftests: ftrace: Add a testcase for function PID filter

[Namhyung Kim]

Like event pid filtering test, add function pid filtering test with the
new "function-fork" option.  It also tests it on an instance directory
so that it can verify the bug related pid filtering on instances.

Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 .../ftrace/test.d/ftrace/func-filter-pid.tc        | 98 ++++++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc

diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
new file mode 100644
index 000000000000..cd552f44c3b4
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
@@ -0,0 +1,98 @@
+#!/bin/sh
+# description: ftrace - function pid filters
+
+# Make sure that function pid matching filter works.
+# Also test it on an instance directory
+
+if ! grep -q function available_tracers; then
+    echo "no function tracer configured"
+    exit_unsupported
+fi
+
+if [ ! -f set_ftrace_pid ]; then
+    echo "set_ftrace_pid not found? Is function tracer not set?"
+    exit_unsupported
+fi
+
+if [ ! -f set_ftrace_filter ]; then
+    echo "set_ftrace_filter not found? Is function tracer not set?"
+    exit_unsupported
+fi
+
+read PID _ < /proc/self/stat
+
+# default value of function-fork option
+orig_value=`grep function-fork trace_options`
+
+do_reset() {
+    reset_tracer
+    clear_trace
+    enable_tracing
+    echo > set_ftrace_filter
+    echo > set_ftrace_pid
+
+    echo $orig_value > trace_options
+}
+
+fail() { # msg
+    do_reset
+    echo $1
+    exit $FAIL
+}
+
+yield() {
+    ping localhost -c 1 || sleep .001 || usleep 1 || sleep 1
+}
+
+do_test() {
+    disable_tracing
+
+    echo do_execve* > set_ftrace_filter
+    echo *do_fork >> set_ftrace_filter
+
+    echo $PID > set_ftrace_pid
+    echo function > current_tracer
+
+    # don't allow children to be traced
+    echo nofunction-fork > trace_options
+
+    enable_tracing
+    yield
+
+    count_pid=`cat trace | grep -v ^# | grep $PID | wc -l`
+    count_other=`cat trace | grep -v ^# | grep -v $PID | wc -l`
+
+    # count_other should be 0
+    if [ $count_pid -eq 0 -o $count_other -ne 0 ]; then
+	fail "PID filtering not working?"
+    fi
+
+    disable_tracing
+    clear_trace
+
+    # allow children to be traced
+    echo function-fork > trace_options
+
+    enable_tracing
+    yield
+
+    count_pid=`cat trace | grep -v ^# | grep $PID | wc -l`
+    count_other=`cat trace | grep -v ^# | grep -v $PID | wc -l`
+
+    # count_other should NOT be 0
+    if [ $count_pid -eq 0 -o $count_other -eq 0 ]; then
+	fail "PID filtering not following fork?"
+    fi
+}
+
+do_test
+
+mkdir instances/foo
+cd instances/foo
+do_test
+cd ../../
+rmdir instances/foo
+
+do_reset
+
+exit 0
-- 
2.12.2

Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Masami Hiramatsu]

On Mon, 17 Apr 2017 11:44:27 +0900
Namhyung Kim <namhyung@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
                   ^^^^^^^^^^^^^^^^ ftrace_clear_pids()?

> instance_rmdir() already updated ftrace codes, it can just free the
> filter safely.

And following explanation may not need.

The code looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks,

> 
> Fixes: 0c8916c34203 ("tracing: Add rmdir to remove multibuffer instances")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  kernel/trace/ftrace.c | 9 +++++++++
>  kernel/trace/trace.c  | 1 +
>  kernel/trace/trace.h  | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 34f63e78d661..7b27066c5ed0 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5610,6 +5610,15 @@ static void clear_ftrace_pids(struct trace_array *tr)
>  	trace_free_pid_list(pid_list);
>  }
>  
> +void ftrace_clear_pids(struct trace_array *tr)
> +{
> +	mutex_lock(&ftrace_lock);
> +
> +	clear_ftrace_pids(tr);
> +
> +	mutex_unlock(&ftrace_lock);
> +}
> +
>  static void ftrace_pid_reset(struct trace_array *tr)
>  {
>  	mutex_lock(&ftrace_lock);
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index b5d4b80f2d45..0bf623c182da 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_clear_pids(tr);
>  	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..ee27163b7549 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_clear_pids(struct trace_array *tr);
>  #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_clear_pids(struct trace_array *tr) { }
>  /* 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 */
> -- 
> 2.12.2
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Namhyung Kim]

Hi Masami,

On Mon, Apr 17, 2017 at 8:52 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> On Mon, 17 Apr 2017 11:44:27 +0900
> Namhyung Kim <namhyung@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
>                    ^^^^^^^^^^^^^^^^ ftrace_clear_pids()?

Oops, forgot to update the changelog, sorry.

>
>> instance_rmdir() already updated ftrace codes, it can just free the
>> filter safely.
>
> And following explanation may not need.
>
> The code looks good to me.
>
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!
Namhyung

>>
>> Fixes: 0c8916c34203 ("tracing: Add rmdir to remove multibuffer instances")
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  kernel/trace/ftrace.c | 9 +++++++++
>>  kernel/trace/trace.c  | 1 +
>>  kernel/trace/trace.h  | 2 ++
>>  3 files changed, 12 insertions(+)
>>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 34f63e78d661..7b27066c5ed0 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -5610,6 +5610,15 @@ static void clear_ftrace_pids(struct trace_array *tr)
>>       trace_free_pid_list(pid_list);
>>  }
>>
>> +void ftrace_clear_pids(struct trace_array *tr)
>> +{
>> +     mutex_lock(&ftrace_lock);
>> +
>> +     clear_ftrace_pids(tr);
>> +
>> +     mutex_unlock(&ftrace_lock);
>> +}
>> +
>>  static void ftrace_pid_reset(struct trace_array *tr)
>>  {
>>       mutex_lock(&ftrace_lock);
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index b5d4b80f2d45..0bf623c182da 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_clear_pids(tr);
>>       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..ee27163b7549 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_clear_pids(struct trace_array *tr);
>>  #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_clear_pids(struct trace_array *tr) { }
>>  /* 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 */
>> --
>> 2.12.2
>>
>
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>



-- 
Thanks,
Namhyung

Re: [PATCH 0/4] ftrace: Add 'function-fork' trace option (v2)

[Steven Rostedt]

On Mon, 17 Apr 2017 11:44:26 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> Hello,
> 
> This patchset add 'function-fork' option to function tracer which
> makes pid filter to be inherited like 'event-fork' does.  During the
> test, I found a bug of pid filter on an instance directory.  The patch
> 1 fixes it and maybe it should go to the stable tree.

Hmm, are the other patches dependent on it?

I think I may just push it separately to Linus now, but the other
patches will be on my devel branch which will not be abased off of this
fix. Will that break too much? I just cherry-picked a patch from my
urgent branch as it required to be on my devel branch and go to Linus.

Hmm, I may be able to make a separate branch with this. I have to see
how much it conflicts with my current development.

-- Steve

Re: [PATCH 0/4] ftrace: Add 'function-fork' trace option (v2)

[Namhyung Kim]

Hi Steve,

Sorry for little late,

On Tue, Apr 18, 2017 at 4:18 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 17 Apr 2017 11:44:26 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>
>> Hello,
>>
>> This patchset add 'function-fork' option to function tracer which
>> makes pid filter to be inherited like 'event-fork' does.  During the
>> test, I found a bug of pid filter on an instance directory.  The patch
>> 1 fixes it and maybe it should go to the stable tree.
>
> Hmm, are the other patches dependent on it?

Nop, but there will be a small clash on trace.h for the declaration.

>
> I think I may just push it separately to Linus now, but the other
> patches will be on my devel branch which will not be abased off of this
> fix. Will that break too much? I just cherry-picked a patch from my
> urgent branch as it required to be on my devel branch and go to Linus.

I don't think it breaks much.

>
> Hmm, I may be able to make a separate branch with this. I have to see
> how much it conflicts with my current development.

Thanks,
Namhyung

Re: [PATCH 0/4] ftrace: Add 'function-fork' trace option (v2)

[Steven Rostedt]

On Wed, 19 Apr 2017 09:27:28 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> Hi Steve,
> 
> Sorry for little late,
> 
> On Tue, Apr 18, 2017 at 4:18 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Mon, 17 Apr 2017 11:44:26 +0900
> > Namhyung Kim <namhyung@kernel.org> wrote:
> >  
> >> Hello,
> >>
> >> This patchset add 'function-fork' option to function tracer which
> >> makes pid filter to be inherited like 'event-fork' does.  During the
> >> test, I found a bug of pid filter on an instance directory.  The patch
> >> 1 fixes it and maybe it should go to the stable tree.  
> >
> > Hmm, are the other patches dependent on it?  
> 
> Nop, but there will be a small clash on trace.h for the declaration.

Yep, I push up a merge with mainline with my linux-next branch to cover
the conflicts. I'll let Linus know about it too when I do my pull
request in the merge window.

> 
> >
> > I think I may just push it separately to Linus now, but the other
> > patches will be on my devel branch which will not be abased off of this
> > fix. Will that break too much? I just cherry-picked a patch from my
> > urgent branch as it required to be on my devel branch and go to Linus.  
> 
> I don't think it breaks much.

Except that your test triggers the bug it uncovered ;-)

-- Steve

> 
> >
> > Hmm, I may be able to make a separate branch with this. I have to see
> > how much it conflicts with my current development.  

Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Namhyung Kim]

On Tue, Mar 28, 2017 at 10:58:43PM -0400, Steven Rostedt wrote:
> On Wed, 29 Mar 2017 11:42:27 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
> 
> > On Tue, Mar 28, 2017 at 10:28:55PM -0400, Steven Rostedt wrote:
> > > On Wed, 29 Mar 2017 11:20:37 +0900
> > > Namhyung Kim <namhyung@kernel.org> wrote:
> > > 
> > >   
> > > > > 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?  
> > > 
> > > Well, nothing should be able to get to the set_ftrace_filter file when
> > > there. Because of the tr->ref count. But keeping the lock is safer
> > > regardless, and it's not a fast path, so the extra overhead if the lock
> > > isn't needed is no big deal.  
> > 
> > Oh, I meant if a pid filter was already set when removing the
> > instance.  Function filters should be inactive since function tracer
> > was finished (via tracing_set_nop), but the probe on sched_switch
> > event (for pid filter) is still active and references the tr.
> > 
> 
> I think we are talking about two different things. I was simply talking
> about the need to take the ftrace_lock or not in the
> clear_ftrace_pids() call here. I don't think we have to, because nothing
> should be in contention with it at that point. But it doesn't hurt to
> take it.

Right, I agree with you wrt the locking.

Thanks,
Namhyung

Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Steven Rostedt]

On Wed, 29 Mar 2017 11:42:27 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> On Tue, Mar 28, 2017 at 10:28:55PM -0400, Steven Rostedt wrote:
> > On Wed, 29 Mar 2017 11:20:37 +0900
> > Namhyung Kim <namhyung@kernel.org> wrote:
> > 
> >   
> > > > 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?  
> > 
> > Well, nothing should be able to get to the set_ftrace_filter file when
> > there. Because of the tr->ref count. But keeping the lock is safer
> > regardless, and it's not a fast path, so the extra overhead if the lock
> > isn't needed is no big deal.  
> 
> Oh, I meant if a pid filter was already set when removing the
> instance.  Function filters should be inactive since function tracer
> was finished (via tracing_set_nop), but the probe on sched_switch
> event (for pid filter) is still active and references the tr.
> 

I think we are talking about two different things. I was simply talking
about the need to take the ftrace_lock or not in the
clear_ftrace_pids() call here. I don't think we have to, because nothing
should be in contention with it at that point. But it doesn't hurt to
take it.

-- Steve

Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Namhyung Kim]

On Tue, Mar 28, 2017 at 10:28:55PM -0400, Steven Rostedt wrote:
> On Wed, 29 Mar 2017 11:20:37 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
> 
> 
> > > 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?
> 
> Well, nothing should be able to get to the set_ftrace_filter file when
> there. Because of the tr->ref count. But keeping the lock is safer
> regardless, and it's not a fast path, so the extra overhead if the lock
> isn't needed is no big deal.

Oh, I meant if a pid filter was already set when removing the
instance.  Function filters should be inactive since function tracer
was finished (via tracing_set_nop), but the probe on sched_switch
event (for pid filter) is still active and references the tr.

Thanks,
Namhyung


> 
> > 
> > > 
> > > 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?
> 
> Yes, because the bool passed in is confusing. A separate function like
> below is more descriptive.
> 
> -- Steve
> 
> > 
> > Thanks,
> > Namhyung
> > 
> > 
> > > 
> > > void ftrace_clear_pids(struct trace_array *tr)
> > > {
> > > 	mutex_lock(&ftrace_lock);
> > > 
> > > 	clear_ftrace_pids(tr);
> > > 
> > > 	mutex_unlock(&ftrace_lock);
> > > }

Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Steven Rostedt]

On Wed, 29 Mar 2017 11:20:37 +0900
Namhyung Kim <namhyung@kernel.org> wrote:


> > 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?

Well, nothing should be able to get to the set_ftrace_filter file when
there. Because of the tr->ref count. But keeping the lock is safer
regardless, and it's not a fast path, so the extra overhead if the lock
isn't needed is no big deal.

> 
> > 
> > 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?

Yes, because the bool passed in is confusing. A separate function like
below is more descriptive.

-- Steve

> 
> Thanks,
> Namhyung
> 
> 
> > 
> > void ftrace_clear_pids(struct trace_array *tr)
> > {
> > 	mutex_lock(&ftrace_lock);
> > 
> > 	clear_ftrace_pids(tr);
> > 
> > 	mutex_unlock(&ftrace_lock);
> > }

Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Namhyung Kim]

On Tue, Mar 28, 2017 at 10:08:41PM -0400, Steven Rostedt wrote:
> On Wed, 29 Mar 2017 10:46:22 +0900
> Namhyung Kim <namhyung@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 <namhyung@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.

Ah, missed this part.  Ok, I'll make it a separate function.

Thanks,
Namhyung

Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Namhyung Kim]

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 <namhyung@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 <namhyung@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 */
> 

Re: [PATCH 1/4] ftrace: Fix function pid filter on instances

[Steven Rostedt]

On Wed, 29 Mar 2017 10:46:22 +0900
Namhyung Kim <namhyung@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 <namhyung@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.

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.

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 */

[PATCH 1/4] ftrace: Fix function pid filter on instances

[Namhyung Kim]

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 <namhyung@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);
 }
@@ -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);
 	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 */
-- 
2.12.0