[PATCH v2 1/2] tracing/kprobes: add a helper method to return number of probe hits

[PATCH v2 1/2] tracing/kprobes: add a helper method to return number of probe hits

[Marcin Nowakowski]

The number of probe hits is stored in a percpu variable and therefore
can't be read directly. Add a helper method trace_kprobe_nhit() that
performs the required calculation.

It will be used in a follow-up commit that changes kprobe selftests to
verify the number of probe hits.

Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
---
 kernel/trace/trace_kprobe.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

 v2: improved commit message as suggested by Steven Rostedt

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index eb6c9f1..a2af1bc 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -73,6 +73,17 @@ static nokprobe_inline bool trace_kprobe_is_on_module(struct trace_kprobe *tk)
 	return !!strchr(trace_kprobe_symbol(tk), ':');
 }
 
+static nokprobe_inline unsigned long trace_kprobe_nhit(struct trace_kprobe *tk)
+{
+	unsigned long nhit = 0;
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		nhit += *per_cpu_ptr(tk->nhit, cpu);
+
+	return nhit;
+}
+
 static int register_kprobe_event(struct trace_kprobe *tk);
 static int unregister_kprobe_event(struct trace_kprobe *tk);
 
@@ -882,14 +893,10 @@ static const struct file_operations kprobe_events_ops = {
 static int probes_profile_seq_show(struct seq_file *m, void *v)
 {
 	struct trace_kprobe *tk = v;
-	unsigned long nhit = 0;
-	int cpu;
-
-	for_each_possible_cpu(cpu)
-		nhit += *per_cpu_ptr(tk->nhit, cpu);
 
 	seq_printf(m, "  %-44s %15lu %15lu\n",
-		   trace_event_name(&tk->tp.call), nhit,
+		   trace_event_name(&tk->tp.call),
+		   trace_kprobe_nhit(tk),
 		   tk->rp.kp.nmissed);
 
 	return 0;
-- 
2.7.4

[PATCH v2 2/2] kprobes/trace: Fix kprobe selftest for newer gcc

[Marcin Nowakowski]

Commit 265a5b7ee3eb ("kprobes/trace: Fix kprobe selftest for gcc 4.6")
has added __used attribute to kprobe_trace_selftest_target to ensure
that the method is listed in kallsyms table.

However, even though the method remains in the kernel image, the actual
call is optimised away as there are no side efects and the return value
is never checked.

Add a return value check and a 'noinline' attribute to ensure that an
inlined copy of the method is not used by the caller. Also add checks
that verify that the kprobe was really hit, as at the moment the tests
show positive results despite the test method being optimised away.

Finally, add __init annotations to find_trace_probe_file() and
kprobe_trace_selftest_target() as they are only called from within an
__init method.

Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
---
 kernel/trace/trace_kprobe.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

 v2: no changes

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index a2af1bc..a133ecd 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1361,18 +1361,18 @@ fs_initcall(init_kprobe_trace);
 
 
 #ifdef CONFIG_FTRACE_STARTUP_TEST
-
 /*
  * The "__used" keeps gcc from removing the function symbol
- * from the kallsyms table.
+ * from the kallsyms table. 'noinline' makes sure that there
+ * isn't an inlined version used by the test method below
  */
-static __used int kprobe_trace_selftest_target(int a1, int a2, int a3,
-					       int a4, int a5, int a6)
+static __used __init noinline int
+kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6)
 {
 	return a1 + a2 + a3 + a4 + a5 + a6;
 }
 
-static struct trace_event_file *
+static struct __init trace_event_file *
 find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
 {
 	struct trace_event_file *file;
@@ -1450,12 +1450,25 @@ static __init int kprobe_trace_self_tests_init(void)
 
 	ret = target(1, 2, 3, 4, 5, 6);
 
+	/*
+	 * Not expecting an error here, the check is only to prevent the
+	 * optimizer from removing the call to target() as otherwise there
+	 * are no side-effects and the call is never performed.
+	 */
+	if (ret != 21)
+		warn++;
+
 	/* Disable trace points before removing it */
 	tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
 	if (WARN_ON_ONCE(tk == NULL)) {
 		pr_warn("error on getting test probe.\n");
 		warn++;
 	} else {
+		if (trace_kprobe_nhit(tk) != 1) {
+			pr_warn("incorrect number of testprobe hits\n");
+			warn++;
+		}
+
 		file = find_trace_probe_file(tk, top_trace_array());
 		if (WARN_ON_ONCE(file == NULL)) {
 			pr_warn("error on getting probe file.\n");
@@ -1469,6 +1482,11 @@ static __init int kprobe_trace_self_tests_init(void)
 		pr_warn("error on getting 2nd test probe.\n");
 		warn++;
 	} else {
+		if (trace_kprobe_nhit(tk) != 1) {
+			pr_warn("incorrect number of testprobe2 hits\n");
+			warn++;
+		}
+
 		file = find_trace_probe_file(tk, top_trace_array());
 		if (WARN_ON_ONCE(file == NULL)) {
 			pr_warn("error on getting probe file.\n");
-- 
2.7.4

Re: [PATCH v2 1/2] tracing/kprobes: add a helper method to return number of probe hits

[Steven Rostedt]

Masami,

I just noticed that you were not Cc'd. Can you give your ack on this
and the other patch. I'll bounce both of them to you.

Thanks!

-- Steve


On Fri, 9 Dec 2016 15:19:37 +0100
Marcin Nowakowski <marcin.nowakowski@imgtec.com> wrote:

> The number of probe hits is stored in a percpu variable and therefore
> can't be read directly. Add a helper method trace_kprobe_nhit() that
> performs the required calculation.
> 
> It will be used in a follow-up commit that changes kprobe selftests to
> verify the number of probe hits.
> 
> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
> ---
>  kernel/trace/trace_kprobe.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
>  v2: improved commit message as suggested by Steven Rostedt
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index eb6c9f1..a2af1bc 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -73,6 +73,17 @@ static nokprobe_inline bool trace_kprobe_is_on_module(struct trace_kprobe *tk)
>  	return !!strchr(trace_kprobe_symbol(tk), ':');
>  }
>  
> +static nokprobe_inline unsigned long trace_kprobe_nhit(struct trace_kprobe *tk)
> +{
> +	unsigned long nhit = 0;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu)
> +		nhit += *per_cpu_ptr(tk->nhit, cpu);
> +
> +	return nhit;
> +}
> +
>  static int register_kprobe_event(struct trace_kprobe *tk);
>  static int unregister_kprobe_event(struct trace_kprobe *tk);
>  
> @@ -882,14 +893,10 @@ static const struct file_operations kprobe_events_ops = {
>  static int probes_profile_seq_show(struct seq_file *m, void *v)
>  {
>  	struct trace_kprobe *tk = v;
> -	unsigned long nhit = 0;
> -	int cpu;
> -
> -	for_each_possible_cpu(cpu)
> -		nhit += *per_cpu_ptr(tk->nhit, cpu);
>  
>  	seq_printf(m, "  %-44s %15lu %15lu\n",
> -		   trace_event_name(&tk->tp.call), nhit,
> +		   trace_event_name(&tk->tp.call),
> +		   trace_kprobe_nhit(tk),
>  		   tk->rp.kp.nmissed);
>  
>  	return 0;

Re: [PATCH v2 1/2] tracing/kprobes: add a helper method to return number of probe hits

[Masami Hiramatsu]

Hello Marcin,

On Fri, 9 Dec 2016 15:19:37 +0100
Marcin Nowakowski <marcin.nowakowski@imgtec.com> wrote:

> The number of probe hits is stored in a percpu variable and therefore
> can't be read directly. Add a helper method trace_kprobe_nhit() that
> performs the required calculation.
> 
> It will be used in a follow-up commit that changes kprobe selftests to
> verify the number of probe hits.

Ah, that's nicer & neet :)

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

Thanks!

> 
> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
> ---
>  kernel/trace/trace_kprobe.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
>  v2: improved commit message as suggested by Steven Rostedt
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index eb6c9f1..a2af1bc 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -73,6 +73,17 @@ static nokprobe_inline bool trace_kprobe_is_on_module(struct trace_kprobe *tk)
>  	return !!strchr(trace_kprobe_symbol(tk), ':');
>  }
>  
> +static nokprobe_inline unsigned long trace_kprobe_nhit(struct trace_kprobe *tk)
> +{
> +	unsigned long nhit = 0;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu)
> +		nhit += *per_cpu_ptr(tk->nhit, cpu);
> +
> +	return nhit;
> +}
> +
>  static int register_kprobe_event(struct trace_kprobe *tk);
>  static int unregister_kprobe_event(struct trace_kprobe *tk);
>  
> @@ -882,14 +893,10 @@ static const struct file_operations kprobe_events_ops = {
>  static int probes_profile_seq_show(struct seq_file *m, void *v)
>  {
>  	struct trace_kprobe *tk = v;
> -	unsigned long nhit = 0;
> -	int cpu;
> -
> -	for_each_possible_cpu(cpu)
> -		nhit += *per_cpu_ptr(tk->nhit, cpu);
>  
>  	seq_printf(m, "  %-44s %15lu %15lu\n",
> -		   trace_event_name(&tk->tp.call), nhit,
> +		   trace_event_name(&tk->tp.call),
> +		   trace_kprobe_nhit(tk),
>  		   tk->rp.kp.nmissed);
>  
>  	return 0;
> -- 
> 2.7.4
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 2/2] kprobes/trace: Fix kprobe selftest for newer gcc

[Masami Hiramatsu]

On Fri, 9 Dec 2016 15:19:38 +0100
Marcin Nowakowski <marcin.nowakowski@imgtec.com> wrote:

> Commit 265a5b7ee3eb ("kprobes/trace: Fix kprobe selftest for gcc 4.6")
> has added __used attribute to kprobe_trace_selftest_target to ensure
> that the method is listed in kallsyms table.
> 
> However, even though the method remains in the kernel image, the actual
> call is optimised away as there are no side efects and the return value
> is never checked.

Nice catch :)

> 
> Add a return value check and a 'noinline' attribute to ensure that an
> inlined copy of the method is not used by the caller. Also add checks
> that verify that the kprobe was really hit, as at the moment the tests
> show positive results despite the test method being optimised away.
> 
> Finally, add __init annotations to find_trace_probe_file() and
> kprobe_trace_selftest_target() as they are only called from within an
> __init method.

Right.

Looks good to me.

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

Thanks!

> 
> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
> ---
>  kernel/trace/trace_kprobe.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
>  v2: no changes
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index a2af1bc..a133ecd 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1361,18 +1361,18 @@ fs_initcall(init_kprobe_trace);
>  
>  
>  #ifdef CONFIG_FTRACE_STARTUP_TEST
> -
>  /*
>   * The "__used" keeps gcc from removing the function symbol
> - * from the kallsyms table.
> + * from the kallsyms table. 'noinline' makes sure that there
> + * isn't an inlined version used by the test method below
>   */
> -static __used int kprobe_trace_selftest_target(int a1, int a2, int a3,
> -					       int a4, int a5, int a6)
> +static __used __init noinline int
> +kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6)
>  {
>  	return a1 + a2 + a3 + a4 + a5 + a6;
>  }
>  
> -static struct trace_event_file *
> +static struct __init trace_event_file *
>  find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
>  {
>  	struct trace_event_file *file;
> @@ -1450,12 +1450,25 @@ static __init int kprobe_trace_self_tests_init(void)
>  
>  	ret = target(1, 2, 3, 4, 5, 6);
>  
> +	/*
> +	 * Not expecting an error here, the check is only to prevent the
> +	 * optimizer from removing the call to target() as otherwise there
> +	 * are no side-effects and the call is never performed.
> +	 */
> +	if (ret != 21)
> +		warn++;
> +
>  	/* Disable trace points before removing it */
>  	tk = find_trace_kprobe("testprobe", KPROBE_EVENT_SYSTEM);
>  	if (WARN_ON_ONCE(tk == NULL)) {
>  		pr_warn("error on getting test probe.\n");
>  		warn++;
>  	} else {
> +		if (trace_kprobe_nhit(tk) != 1) {
> +			pr_warn("incorrect number of testprobe hits\n");
> +			warn++;
> +		}
> +
>  		file = find_trace_probe_file(tk, top_trace_array());
>  		if (WARN_ON_ONCE(file == NULL)) {
>  			pr_warn("error on getting probe file.\n");
> @@ -1469,6 +1482,11 @@ static __init int kprobe_trace_self_tests_init(void)
>  		pr_warn("error on getting 2nd test probe.\n");
>  		warn++;
>  	} else {
> +		if (trace_kprobe_nhit(tk) != 1) {
> +			pr_warn("incorrect number of testprobe2 hits\n");
> +			warn++;
> +		}
> +
>  		file = find_trace_probe_file(tk, top_trace_array());
>  		if (WARN_ON_ONCE(file == NULL)) {
>  			pr_warn("error on getting probe file.\n");
> -- 
> 2.7.4
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>