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

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

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

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 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(-)

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 1/2] tracing/kprobes: add a helper method to return number of probe hits

[Steven Rostedt]

On Fri, 9 Dec 2016 13:25:51 +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.

Can you please add the "why?" to this change log. Who needs this
functionality, and why?

-- Steve

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

[Steven Rostedt]

On Fri, 9 Dec 2016 09:00:52 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 9 Dec 2016 13:25:51 +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.  
> 
> Can you please add the "why?" to this change log. Who needs this
> functionality, and why?
> 

I should have specified, even if the next patch uses the functionality,
each patch's change log should stand on its own. In the future, when
people do git blame, they wont be seeing patch series. They will only
see the change log of the one change.

-- Steve

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

[Marcin Nowakowski]

On 09.12.2016 15:03, Steven Rostedt wrote:
> On Fri, 9 Dec 2016 09:00:52 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> On Fri, 9 Dec 2016 13:25:51 +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.
>>
>> Can you please add the "why?" to this change log. Who needs this
>> functionality, and why?
>>
>
> I should have specified, even if the next patch uses the functionality,
> each patch's change log should stand on its own. In the future, when
> people do git blame, they wont be seeing patch series. They will only
> see the change log of the one change.

Sure, I will send an updated version with a better changelog.

Marcin