[PATCH v2 0/4] A fix and a few new tests for kprobe tracer

v1:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1427923.html

Changes since v1:
  - Patch 1 is unchanged from v1.
  - Patch 3 drops change to include dot symbols, as suggested by Masami.
  - Patches 2 and 4 implement new test cases to cover these scenarios

Thanks,
Naveen


Masami Hiramatsu (1):
  selftests/ftrace: Add a testcase for kprobe event naming

Naveen N. Rao (3):
  trace/kprobes: Sanitize derived event names
  selftests/ftrace: Add a test to probe module functions
  selftests/ftrace: Update multiple kprobes test for powerpc

 kernel/trace/trace_kprobe.c                        |  9 ++++++
 .../ftrace/test.d/kprobe/kprobe_eventname.tc       | 32 ++++++++++++++++++++++
 .../ftrace/test.d/kprobe/multiple_kprobes.tc       |  4 +--
 .../selftests/ftrace/test.d/kprobe/probe_module.tc | 14 ++++++++++
 4 files changed, 57 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc

-- 
2.13.1

Add a kprobes test to ensure that we are able to add a probe on a
module function using 'p <mod>:<func>' format, without having to
specify a probe name.

Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
---
 .../testing/selftests/ftrace/test.d/kprobe/probe_module.tc | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc
new file mode 100644
index 000000000000..ea7657041ba6
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc

@@ -0,0 +1,14 @@
+#!/bin/sh
+# description: Kprobe dynamic event - probing module
+
+[ -f kprobe_events ] || exit_unsupported # this is configurable
+
+echo 0 > events/enable
+echo > kprobe_events
+export MOD=`lsmod | head -n 2 | tail -n 1 | cut -f1 -d" "`
+export FUNC=`grep -m 1 ".* t .*\\[$MOD\\]" /proc/kallsyms | xargs | cut -f3 -d" "`
+[ "x" != "x$MOD" -a "y" != "y$FUNC" ] || exit_untested
+echo p $MOD:$FUNC > kprobe_events
+grep $MOD kprobe_events
+echo > kprobe_events
+clear_trace

-- 
2.13.1

KPROBES_ON_FTRACE is only available on powerpc64le. Update comment to
clarify this.

Also, we should use an offset of 8 to ensure that the probe does not
fall on ftrace location. The current offset of 4 will fall before the
function local entry point and won't fire, while an offset of 12 or 16
will fall on ftrace location. Offset 8 is currently guaranteed to not be
the ftrace location.

Signed-off-by: Naveen N. Rao <redacted>
---
 tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
index f4d1ff785d67..2a1cb9908746 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc

@@ -2,10 +2,10 @@
 # description: Register/unregister many kprobe events
 
 # ftrace fentry skip size depends on the machine architecture.
-# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc
+# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc64le
 case `uname -m` in
   x86_64|i[3456]86) OFFS=5;;
-  ppc*) OFFS=4;;
+  ppc64le) OFFS=8;;
   *) OFFS=0;;
 esac
 

-- 
2.13.1

From: Masami Hiramatsu <mhiramat@kernel.org>

Add a testcase for kprobe event naming. This testcase
checks whether the kprobe events can automatically ganerate
its event name on normal function and dot-suffixed function.
Also it checks whether the kprobe events can correctly
define new event with given event name and group name.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
[Updated tests to use vfs_read and symbols with '.isra.',
added check for kprobe_events and a command to clear it on exit]
Signed-off-by: Naveen N. Rao <redacted>
---
 .../ftrace/test.d/kprobe/kprobe_eventname.tc       | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
new file mode 100644
index 000000000000..182e5a78ef4b
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc

@@ -0,0 +1,32 @@
+#!/bin/sh
+# description: Kprobe event auto/manual naming
+
+[ -f kprobe_events ] || exit_unsupported # this is configurable
+
+disable_events
+echo > kprobe_events
+
+:;: "Add an event on function without name" ;:
+
+FUNC=`grep " [tT] .*vfs_read$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`
+echo p $FUNC > kprobe_events
+FUNC_NAME=`echo $FUNC | tr ".:" "_"`
+test -d events/kprobes/p_${FUNC_NAME}_0 || exit_failure
+
+:;: "Add an event on function with new name" ;:
+
+echo p:event1 $FUNC > kprobe_events
+test -d events/kprobes/event1 || exit_failure
+
+:;: "Add an event on function with new name and group" ;:
+
+echo p:kprobes2/event2 $FUNC > kprobe_events
+test -d events/kprobes2/event2 || exit_failure
+
+:;: "Add an event on dot function without name" ;:
+
+FUNC=`grep -m 10 " [tT] .*\.isra\..*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`
+echo p $FUNC > kprobe_events
+EVENT=`grep $FUNC kprobe_events | cut -f 1 -d " " | cut -f 2 -d:` || exit_failure
+test -d events/$EVENT || exit_failure
+echo > kprobe_events

-- 
2.13.1

When we derive event names, convert some expected symbols (such as ':'
used to specify module:name and '.' present in some symbols) into
underscores so that the event name is not rejected.

Before this patch:
    # echo 'p kobject_example:foo_store' > kprobe_events
    trace_kprobe: Failed to allocate trace_probe.(-22)
    -sh: write error: Invalid argument

After this patch:
    # echo 'p kobject_example:foo_store' > kprobe_events
    # cat kprobe_events
    p:kprobes/p_kobject_example_foo_store_0 kobject_example:foo_store

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
---
 kernel/trace/trace_kprobe.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index c129fca6ec99..44fd819aa33d 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c

@@ -598,6 +598,14 @@ static struct notifier_block trace_kprobe_module_nb = {
 	.priority = 1	/* Invoked after kprobe module callback */
 };
 
+/* Convert certain expected symbols into '_' when generating event names */
+static inline void sanitize_event_name(char *name)
+{
+	while (*name++ != '\0')
+		if (*name == ':' || *name == '.')
+			*name = '_';
+}
+
 static int create_trace_kprobe(int argc, char **argv)
 {
 	/*

@@ -740,6 +748,7 @@ static int create_trace_kprobe(int argc, char **argv)
 		else
 			snprintf(buf, MAX_EVENT_NAME_LEN, "%c_0x%p",
 				 is_return ? 'r' : 'p', addr);
+		sanitize_event_name(buf);
 		event = buf;
 	}
 	tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive,

-- 
2.13.1

On Thu, 29 Jun 2017 19:05:37 +0530
"Naveen N. Rao" [off-list ref] wrote:

Could you also add below case?

echo p:probe_$MOD/$FUNC $MOD/$FUNC > kprobe_events 

This is for "new event with name on module" case, your one is for "new event without name on module (automatic name generation)"

We should have different test case, because those kicks slightly different parts in kprobe tracer.

Thank you,

+echo p $MOD:$FUNC > kprobe_events
+grep $MOD kprobe_events
+echo > kprobe_events
+clear_trace
-- 
2.13.1

-- 
Masami Hiramatsu [off-list ref]

On Thu, 29 Jun 2017 19:05:38 +0530
"Naveen N. Rao" [off-list ref] wrote:

KPROBES_ON_FTRACE is only available on powerpc64le. Update comment to
clarify this.

Also, we should use an offset of 8 to ensure that the probe does not
fall on ftrace location. The current offset of 4 will fall before the
function local entry point and won't fire, while an offset of 12 or 16
will fall on ftrace location. Offset 8 is currently guaranteed to not be
the ftrace location.

OK, looks good to me.

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

Thanks!

-- 
Masami Hiramatsu [off-list ref]

On Thu, 29 Jun 2017 19:05:39 +0530
"Naveen N. Rao" [off-list ref] wrote:

Since in some case we may not be able to find the function(e.g. build with old gcc or not optimized), we should check this is exist. I suggested to return UNRESOLVED for that case instead of FAIL, as below.

FUNC=`grep -m 10 " [tT] .*\.isra\..*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "` || exit_unresolved

Thank you,

+echo p $FUNC > kprobe_events
+EVENT=`grep $FUNC kprobe_events | cut -f 1 -d " " | cut -f 2 -d:` || exit_failure
+test -d events/$EVENT || exit_failure
+echo > kprobe_events
-- 
2.13.1

-- 
Masami Hiramatsu [off-list ref]

On Mon, 3 Jul 2017 12:27:33 +0900
Masami Hiramatsu [off-list ref] wrote:

On Thu, 29 Jun 2017 19:05:37 +0530
"Naveen N. Rao" [off-list ref] wrote:

quoted

Add a kprobes test to ensure that we are able to add a probe on a
module function using 'p <mod>:<func>' format, without having to
specify a probe name.

Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
---
 .../testing/selftests/ftrace/test.d/kprobe/probe_module.tc | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc
new file mode 100644
index 000000000000..ea7657041ba6
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc

@@ -0,0 +1,14 @@
+#!/bin/sh
+# description: Kprobe dynamic event - probing module
+
+[ -f kprobe_events ] || exit_unsupported # this is configurable
+
+echo 0 > events/enable
+echo > kprobe_events
+export MOD=`lsmod | head -n 2 | tail -n 1 | cut -f1 -d" "`
+export FUNC=`grep -m 1 ".* t .*\\[$MOD\\]" /proc/kallsyms | xargs | cut -f3 -d" "`
+[ "x" != "x$MOD" -a "y" != "y$FUNC" ] || exit_untested

Could you also add below case?

echo p:probe_$MOD/$FUNC $MOD/$FUNC > kprobe_events 

Oops, it should be something like

echo "p:test_${MOD}_${FUNC} $MOD/$FUNC" > kprobe_events

since we would like to avoid adding new group name for it.

(Adding new group name should be a separated one.)

Thank you,

This is for "new event with name on module" case, your one is for "new event without name on module (automatic name generation)"

We should have different test case, because those kicks slightly different parts in kprobe tracer.

Thank you,

quoted

+echo p $MOD:$FUNC > kprobe_events
+grep $MOD kprobe_events
+echo > kprobe_events
+clear_trace
-- 
2.13.1

-- 
Masami Hiramatsu [off-list ref]

-- 
Masami Hiramatsu [off-list ref]

On 2017/07/03 12:51PM, Masami Hiramatsu wrote:

On Mon, 3 Jul 2017 12:27:33 +0900
Masami Hiramatsu [off-list ref] wrote:

quoted

On Thu, 29 Jun 2017 19:05:37 +0530
"Naveen N. Rao" [off-list ref] wrote:

quoted

Add a kprobes test to ensure that we are able to add a probe on a
module function using 'p <mod>:<func>' format, without having to
specify a probe name.

Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Naveen N. Rao <redacted>
---
 .../testing/selftests/ftrace/test.d/kprobe/probe_module.tc | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc
new file mode 100644
index 000000000000..ea7657041ba6
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc

@@ -0,0 +1,14 @@
+#!/bin/sh
+# description: Kprobe dynamic event - probing module
+
+[ -f kprobe_events ] || exit_unsupported # this is configurable
+
+echo 0 > events/enable
+echo > kprobe_events
+export MOD=`lsmod | head -n 2 | tail -n 1 | cut -f1 -d" "`
+export FUNC=`grep -m 1 ".* t .*\\[$MOD\\]" /proc/kallsyms | xargs | cut -f3 -d" "`
+[ "x" != "x$MOD" -a "y" != "y$FUNC" ] || exit_untested

Could you also add below case?

echo p:probe_$MOD/$FUNC $MOD/$FUNC > kprobe_events 

Oops, it should be something like

echo "p:test_${MOD}_${FUNC} $MOD/$FUNC" > kprobe_events

since we would like to avoid adding new group name for it.

(Adding new group name should be a separated one.)

Thank you,

quoted

This is for "new event with name on module" case, your one is for "new event without name on module (automatic name generation)"

We should have different test case, because those kicks slightly different parts in kprobe tracer.

Sure. Will make changes to the two tests here and re-spin.

Thanks,
Naveen