A fix and a few new tests for kprobe tracer

PATCH Superseded Naveen N. Rao 2 participants 10 messages Jun 29, 2017 – Jul 7, 2017

Subsystems (2)

Kselftest Tracing

lore

Share & tools

lore

Naveen N. Rao

author

2017-06-29 13:36 UTC #0 lore raw

v2 cover A fix and a few new tests for kprobe tracer

v1:
https://www.mail-archive.com/[email protected]/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

Signature · 1 line

2.13.1

Naveen N. Rao

author

2017-06-29 13:36 UTC #1 lore raw

PATCH v2 1/4 trace/kprobes: Sanitize derived event names

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 <[email protected]>

Signed-off-by Naveen N. Rao <[email protected]>

---
 kernel/trace/trace_kprobe.c | 9 +++++++++
 1 file changed, 9 insertions(+)

kernel/trace/trace_kprobe.c +9 -0

index c129fca6ec99..44fd819aa33d 100644
@@ -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,

Signature · 1 line

2.13.1

Naveen N. Rao

author

2017-06-29 13:36 UTC #2 lore raw

PATCH v2 2/4 selftests/ftrace: Add a test to probe module functions

In reply to Naveen N. Rao

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.

Acked-by Masami Hiramatsu <[email protected]>

Signed-off-by Naveen N. Rao <[email protected]>

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

tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc new file +14 -0

new file mode 100644
index 000000000000..ea7657041ba6
@@ -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

Signature · 1 line

2.13.1

Masami Hiramatsu

maintainer

2017-07-03 03:27 UTC #3 lore raw

Re: [PATCH v2 2/4] selftests/ftrace: Add a test to probe module functions

Quoted 18 lines · On Thu, 29 Jun 2017 19:05:37 +0530 "Naveen N. Rao"…

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 <[email protected]>
Acked-by Masami Hiramatsu <[email protected]>
Signed-off-by Naveen N. Rao <[email protected]>
---
 .../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
tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc new file +0 -0
new file mode 100644
index 000000000000..ea7657041ba6

tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc line 10

Quoted 10 lines

tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc new file +10 -0

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

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

tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc new file +4 -0
+echo p $MOD:$FUNC > kprobe_events
+grep $MOD kprobe_events
+echo > kprobe_events
+clear_trace
Signature · 1 line
2.13.1

Signature · 1 line

Masami Hiramatsu <[email protected]>

Masami Hiramatsu

maintainer

2017-07-03 03:51 UTC #4 lore raw

Re: [PATCH v2 2/4] selftests/ftrace: Add a test to probe module functions

Quoted 34 lines · On Mon, 3 Jul 2017 12:27:33 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 18 lines · On Thu, 29 Jun 2017 19:05:37 +0530 "Naveen N. Rao"…
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 <[email protected]>
Acked-by Masami Hiramatsu <[email protected]>
Signed-off-by Naveen N. Rao <[email protected]>
---
 .../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
tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc new file +0 -0
new file mode 100644
index 000000000000..ea7657041ba6
tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc line 10
Quoted 10 lines
tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc new file +10 -0
@@ -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?

Quoted 1 line

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 18 lines

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 7 lines
tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc new file +4 -0
+echo p $MOD:$FUNC > kprobe_events
+grep $MOD kprobe_events
+echo > kprobe_events
+clear_trace
Signature · 1 line
2.13.1
Signature · 1 line
Masami Hiramatsu <[email protected]>

Signature · 1 line

Masami Hiramatsu <[email protected]>

Naveen N. Rao

author

2017-07-07 16:09 UTC #5 lore raw

Re: [PATCH v2 2/4] selftests/ftrace: Add a test to probe module functions

Quoted 41 lines · Masami Hiramatsu

Quoted 34 lines · On Mon, 3 Jul 2017 12:27:33 +0900 Masami Hiramatsu <[email protected]> wrote:
Quoted 18 lines · On Thu, 29 Jun 2017 19:05:37 +0530 "Naveen N. Rao"…
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 <[email protected]>
Acked-by Masami Hiramatsu <[email protected]>
Signed-off-by Naveen N. Rao <[email protected]>
---
 .../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
tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc new file +0 -0
new file mode 100644
index 000000000000..ea7657041ba6
tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc line 10
Quoted 10 lines
tools/testing/selftests/ftrace/test.d/kprobe/probe_module.tc new file +10 -0
@@ -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?
Quoted 1 line
echo p:probe_$MOD/$FUNC $MOD/$FUNC > kprobe_events
Oops, it should be something like

Quoted 12 lines

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 4 lines
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

Naveen N. Rao

author

2017-06-29 13:36 UTC #6 lore raw

PATCH v2 3/4 selftests/ftrace: Update multiple kprobes test for powerpc

In reply to Naveen N. Rao

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 <[email protected]>

---
 tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc +2 -2

index f4d1ff785d67..2a1cb9908746 100644
@@ -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

Signature · 1 line

2.13.1

Masami Hiramatsu

maintainer Acked-by on v2 3/4

2017-07-03 03:28 UTC #7 lore raw

Re: [PATCH v2 3/4] selftests/ftrace: Update multiple kprobes test for powerpc

Quoted 8 lines · On Thu, 29 Jun 2017 19:05:38 +0530 "Naveen N. Rao"…

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 <[email protected]>

Thanks!

Quoted 26 lines

Signed-off-by Naveen N. Rao <[email protected]>

---
 tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc +2 -2

index f4d1ff785d67..2a1cb9908746 100644
@@ -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

Signature · 1 line

2.13.1

Signature · 1 line

Masami Hiramatsu <[email protected]>

Naveen N. Rao

author

2017-06-29 13:36 UTC #8 lore raw

PATCH v2 4/4 selftests/ftrace: Add a testcase for kprobe event naming

In reply to Naveen N. Rao

From: Masami Hiramatsu <[email protected]>

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 <[email protected]>

[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 <[email protected]>

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

tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc new file +32 -0

new file mode 100644
index 000000000000..182e5a78ef4b
@@ -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

Signature · 1 line

2.13.1

Masami Hiramatsu

maintainer

2017-07-03 03:38 UTC #9 lore raw

Re: [PATCH v2 4/4] selftests/ftrace: Add a testcase for kprobe event naming

Quoted 41 lines · On Thu, 29 Jun 2017 19:05:39 +0530 "Naveen N. Rao"…

From: Masami Hiramatsu <[email protected]>
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 <[email protected]>
[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 <[email protected]>
---
 .../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
tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc new file +18 -0
new file mode 100644
index 000000000000..182e5a78ef4b
@@ -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

tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc line 28

Quoted 10 lines

tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc new file +10 -0

+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 " "`

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,

Quoted 7 lines

tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc new file +4 -0
+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
Signature · 1 line
2.13.1

Signature · 1 line

Masami Hiramatsu <[email protected]>