[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

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

[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 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.  

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

[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.

The code is also available at 'ftrace/function-fork-v1' 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                              | 47 ++++++++++-
 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, 158 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc

-- 
2.12.0

[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 b451a860e885..7ca2ed8d0a48 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5575,6 +5575,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 b92489dfa829..291d8fc9dc36 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 4d9804fd9a2d..bacf5e845c55 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_pid_reset(struct trace_array *tr, bool update);
+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_pid_reset(struct trace_array *tr, bool update) { }
+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.0

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

[Steven Rostedt]

On Wed, 29 Mar 2017 10:46:23 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

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

Actually, I had every set, but it appears I stopped at this step. You
just finished it ;-)

Although, I think there was some issues with function tracing. I'll
have to do some more testing an analysis tomorrow. But looking at this
code, it currently appears fine.

-- Steve


> 
> 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 b451a860e885..7ca2ed8d0a48 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5575,6 +5575,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 b92489dfa829..291d8fc9dc36 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 4d9804fd9a2d..bacf5e845c55 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_pid_reset(struct trace_array *tr, bool update);
> +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_pid_reset(struct trace_array *tr, bool update) { }
> +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

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

[Steven Rostedt]

On Tue, 28 Mar 2017 22:18:52 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 29 Mar 2017 10:46:23 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
> 
> > 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.  
> 
> Actually, I had every set, but it appears I stopped at this step. You
> just finished it ;-)
> 

It's good to know that if I ever did get hit by a bus, my unfinished
upstream work would actually get completed. :-)

-- Steve