tracing: kprobes: Prohibit probing on notrace functions

PATCH In review Masami Hiramatsu 3 participants 8 messages Jul 30, 2018 – Jul 31, 2018

Subsystems (2)

Kselftest Tracing

lore

Share & tools

lore

Masami Hiramatsu

author maintainer

2018-07-30 10:20 UTC #0 lore raw

v5 cover tracing: kprobes: Prohibit probing on notrace functions

Hi,

This is the 5th version of the series to prohibit kprobe on notrace functions.

This fixes a build error when CONFIG_FUNCTION_TRACER=y but DYNAMIC_FTRACE=n. Finally I decided to recover the approach in the 1st version, made it depends on CONFIG_KPROBES_ON_FTRACE=y. It is the simplest and enough to prohibiting kernel crash.

So, in summary, if CONFIG_KPROBES_ON_FTRACE=y (which depends on CONFIG_DYNAMIC_FTRACE=y, so ftrace_location_range must be there) && CONFIG_KPROBE_EVENTS_ON_NOTRACE=n (default), kprobe events can not be defined on notrace function. Otherwides (means CONFIG_KPROBES_ON_FTRACE=n or CONFIG_KPROBE_EVENTS_ON_NOTRACE=y), we can put kprobe events on notrace functions.

Francis, I dropped your tested-by because I got many kbuild errors and fixed it. If you can, could you test it again?

Thank you,

---

Francis Deslauriers (1):
selftest/ftrace: Move kprobe selftest function to separate compile unit

Masami Hiramatsu (2):
tracing: kprobes: Prohibit probing on notrace function selftests/ftrace: Fix kprobe string testcase to not probe notrace function

 kernel/trace/Kconfig                               |   20 +++++++
 kernel/trace/Makefile                              |    5 ++
 kernel/trace/trace_kprobe.c                        |   59 +++++++++++++-------
 kernel/trace/trace_kprobe_selftest.c               |   10 +++
 kernel/trace/trace_kprobe_selftest.h               |    7 ++
 .../ftrace/test.d/kprobe/kprobe_args_string.tc     |   30 ++++------
 .../selftests/ftrace/test.d/kprobe/probepoint.tc   |    2 -
 7 files changed, 94 insertions(+), 39 deletions(-)
 create mode 100644 kernel/trace/trace_kprobe_selftest.c
 create mode 100644 kernel/trace/trace_kprobe_selftest.h

--
Masami Hiramatsu (Linaro) <[email protected]>

Masami Hiramatsu

author maintainer

2018-07-30 10:20 UTC #1 lore raw

PATCH v5 1/3 tracing: kprobes: Prohibit probing on notrace function

Prohibit kprobe-events probing on notrace function. Since probing on the notrace function can cause recursive event call. In most case those are just skipped, but in some case it falls into infinit recursive call.

This protection can be disabled by the kconfig CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, but it is highly recommended to keep it "n" for normal kernel. Note that this is only available if "kprobes on ftrace" has been implemented on target arch and CONFIG_KPROBES_ON_FTRACE=y.

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes in v2
- Add CONFIG_KPROBE_EVENTS_ON_NOTRACE kconfig for knocking down the protection.
Changes in v3
- Fix to check raw-address (no symbol) probe point correctly.
Changes in v4
- No notrace check if CONFIG_FUNCTION_TRACER=n. In that case notrace is ignored.
Changes in v5
- Change dependency on CONFIG_KPROBES_ON_FTRACE as same as original v1 patch does.

---
 kernel/trace/Kconfig        |   20 ++++++++++++++++++
 kernel/trace/trace_kprobe.c |   47 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 58 insertions(+), 9 deletions(-)

kernel/trace/Kconfig +20 -0

index dcc0166d1997..88b39f91b1e3 100644
@@ -456,6 +456,26 @@ config KPROBE_EVENTS
 	  This option is also required by perf-probe subcommand of perf tools.
 	  If you want to use perf tools, this option is strongly recommended.
 
+config KPROBE_EVENTS_ON_NOTRACE
+	bool "Do NOT protect notrace function from kprobe events"
+	depends on KPROBE_EVENTS
+	depends on KPROBES_ON_FTRACE
+	default n
+	help
+	  This is only for the developers who want to debug ftrace itself
+	  using kprobe events.
+
+	  If kprobes can use ftrace instead of breakpoint, ftrace related
+	  functions are protected from kprobe-events to prevent an infinit
+	  recursion or any unexpected execution path which leads to a kernel
+	  crash.
+
+	  This option disables such protection and allows you to put kprobe
+	  events on ftrace functions for debugging ftrace by itself.
+	  Note that this might let you shoot yourself in the foot.
+
+	  If unsure, say N.
+
 config UPROBE_EVENTS
 	bool "Enable uprobes-based dynamic events"
 	depends on ARCH_SUPPORTS_UPROBES

kernel/trace/trace_kprobe.c +38 -9

72 lines not on this page · raw message

Steven Rostedt

maintainer Tested-by Francis Deslauriers on v5 1/3

2018-07-30 22:40 UTC #2 lore raw

Re: [PATCH v5 1/3] tracing: kprobes: Prohibit probing on notrace function

Quoted 5 lines · On Mon, 30 Jul 2018 19:20:14 +0900 Masami Hiramatsu <[email protected]> wrote:

Prohibit kprobe-events probing on notrace function. Since probing on the notrace function can cause recursive event call. In most case those are just skipped, but in some case it falls into infinit recursive call.

Quoted 9 lines

This protection can be disabled by the kconfig CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, but it is highly recommended to keep it "n" for normal kernel. Note that this is only available if "kprobes on ftrace" has been implemented on target arch and CONFIG_KPROBES_ON_FTRACE=y.
Signed-off-by Masami Hiramatsu <[email protected]>

Hi Masami,

Note, I made the following changes for grammar. For the Change log I have this:

tracing: kprobes: Prohibit probing on notrace function

Prohibit kprobe-events probing on notrace functions. Since probing on a notrace function can cause a recursive event call. In most cases those are just skipped, but in some cases it falls into an infinite recursive call.

This protection can be disabled by the kconfig CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, but it is highly recommended to keep it "n" for normal kernel builds. Note that this is only available if "kprobes on ftrace" has been implemented on the target arch and CONFIG_KPROBES_ON_FTRACE=y.

Link http://lkml.kernel.org/r/153294601436.32740.10557881188933661239.stgit@devbox open here

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Francis Deslauriers <[email protected]>

[ Slight grammar and spelling fixes ]

Signed-off-by Steven Rostedt (VMware) <[email protected]>

And this change to the patch:

kernel/trace/Kconfig +6 -6

index 4d4eb15cc7fd..23fc7c9abedb 100644
@@ -457,7 +457,7 @@ config KPROBE_EVENTS
 	  If you want to use perf tools, this option is strongly recommended.
 
 config KPROBE_EVENTS_ON_NOTRACE
-	bool "Do NOT protect notrace function from kprobe events"
+	bool "Do NOT protect notrace functions from kprobe events"
 	depends on KPROBE_EVENTS
 	depends on KPROBES_ON_FTRACE
 	default n

@@ -465,13 +465,13 @@ config KPROBE_EVENTS_ON_NOTRACE
 	  This is only for the developers who want to debug ftrace itself
 	  using kprobe events.
 
-	  If kprobes can use ftrace instead of breakpoint, ftrace related
-	  functions are protected from kprobe-events to prevent an infinit
-	  recursion or any unexpected execution path which leads to a kernel
-	  crash.
+	  If kprobes is using ftrace to hook to a function instead of a
+	  breakpoint, ftrace related functions are protected from
+	  kprobe-events to prevent an infinite recursion or any unexpected
+	  execution path which would lead to a kernel crash.
 
 	  This option disables such protection and allows you to put kprobe
-	  events on ftrace functions for debugging ftrace by itself.
+	  events on ftrace functions for debugging ftrace itself.
 	  Note that this might let you shoot yourself in the foot.
 
 	  If unsure, say N.

Are you OK with this?

-- Steve

Masami Hiramatsu

author maintainer

2018-07-31 01:04 UTC #3 lore raw

Re: [PATCH v5 1/3] tracing: kprobes: Prohibit probing on notrace function

Quoted 34 lines · On Mon, 30 Jul 2018 18:40:10 -0400 Steven Rostedt <[email protected]> wrote:

Quoted 5 lines · On Mon, 30 Jul 2018 19:20:14 +0900 Masami Hiramatsu <[email protected]> wrote:
Prohibit kprobe-events probing on notrace function. Since probing on the notrace function can cause recursive event call. In most case those are just skipped, but in some case it falls into infinit recursive call.
Quoted 9 lines
This protection can be disabled by the kconfig CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, but it is highly recommended to keep it "n" for normal kernel. Note that this is only available if "kprobes on ftrace" has been implemented on target arch and CONFIG_KPROBES_ON_FTRACE=y.
Signed-off-by Masami Hiramatsu <[email protected]>
Hi Masami,
Note, I made the following changes for grammar. For the Change log I have this:
tracing: kprobes: Prohibit probing on notrace function
Prohibit kprobe-events probing on notrace functions. Since probing on a notrace function can cause a recursive event call. In most cases those are just skipped, but in some cases it falls into an infinite recursive call.
This protection can be disabled by the kconfig CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, but it is highly recommended to keep it "n" for normal kernel builds. Note that this is only available if "kprobes on ftrace" has been implemented on the target arch and CONFIG_KPROBES_ON_FTRACE=y.

Quoted 6 lines

Link http://lkml.kernel.org/r/153294601436.32740.10557881188933661239.stgit@devbox open here
Signed-off-by Masami Hiramatsu <[email protected]>
Tested-by Francis Deslauriers <[email protected]>
[ Slight grammar and spelling fixes ]
Signed-off-by Steven Rostedt (VMware) <[email protected]>

Looks good to me.
Thanks for fixing it! :)

Quoted 41 lines

And this change to the patch:

kernel/trace/Kconfig +6 -6

index 4d4eb15cc7fd..23fc7c9abedb 100644
@@ -457,7 +457,7 @@ config KPROBE_EVENTS
 	  If you want to use perf tools, this option is strongly recommended.
 
 config KPROBE_EVENTS_ON_NOTRACE
-	bool "Do NOT protect notrace function from kprobe events"
+	bool "Do NOT protect notrace functions from kprobe events"
 	depends on KPROBE_EVENTS
 	depends on KPROBES_ON_FTRACE
 	default n

@@ -465,13 +465,13 @@ config KPROBE_EVENTS_ON_NOTRACE
 	  This is only for the developers who want to debug ftrace itself
 	  using kprobe events.
 
-	  If kprobes can use ftrace instead of breakpoint, ftrace related
-	  functions are protected from kprobe-events to prevent an infinit
-	  recursion or any unexpected execution path which leads to a kernel
-	  crash.
+	  If kprobes is using ftrace to hook to a function instead of a
+	  breakpoint, ftrace related functions are protected from
+	  kprobe-events to prevent an infinite recursion or any unexpected
+	  execution path which would lead to a kernel crash.
 
 	  This option disables such protection and allows you to put kprobe
-	  events on ftrace functions for debugging ftrace by itself.
+	  events on ftrace functions for debugging ftrace itself.
 	  Note that this might let you shoot yourself in the foot.
 
 	  If unsure, say N.

Are you OK with this?

-- Steve

Signature · 1 line

Masami Hiramatsu <[email protected]>

Masami Hiramatsu

author maintainer

2018-07-30 10:21 UTC #4 lore raw

PATCH v5 2/3 selftest/ftrace: Move kprobe selftest function to separate compile unit

In reply to Masami Hiramatsu

From: Francis Deslauriers <[email protected]>

Move selftest function to its own compile unit so it can be compiled with the ftrace cflags (CC_FLAGS_FTRACE) allowing it to be probed during the ftrace startup tests.

Signed-off-by Francis Deslauriers <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/trace/Makefile                |    5 +++++
 kernel/trace/trace_kprobe.c          |   12 +-----------
 kernel/trace/trace_kprobe_selftest.c |   10 ++++++++++
 kernel/trace/trace_kprobe_selftest.h |    7 +++++++
 4 files changed, 23 insertions(+), 11 deletions(-)
 create mode 100644 kernel/trace/trace_kprobe_selftest.c
 create mode 100644 kernel/trace/trace_kprobe_selftest.h

kernel/trace/Makefile +5 -0

index e2538c7638d4..e38771eccb2f 100644
@@ -13,6 +13,11 @@ obj-y += trace_selftest_dynamic.o
 endif
 endif
 
+ifdef CONFIG_FTRACE_STARTUP_TEST
+CFLAGS_trace_kprobe_selftest.o = $(CC_FLAGS_FTRACE)
+obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe_selftest.o
+endif
+
 # If unlikely tracing is enabled, do not trace these files
 ifdef CONFIG_TRACING_BRANCHES
 KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING

kernel/trace/trace_kprobe.c +1 ~~-11~~

index 10fdf1605e01..d314b843beca 100644
@@ -23,6 +23,7 @@
 #include <linux/rculist.h>
 #include <linux/error-injection.h>
 
+#include "trace_kprobe_selftest.h"
 #include "trace_probe.h"
 
 #define KPROBE_EVENT_SYSTEM "kprobes"

@@ -1565,17 +1566,6 @@ fs_initcall(init_kprobe_trace);
 
 
 #ifdef CONFIG_FTRACE_STARTUP_TEST
-/*
- * The "__used" keeps gcc from removing the function symbol
- * from the kallsyms table. 'noinline' makes sure that there
- * isn't an inlined version used by the test method below
- */
-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 __init struct trace_event_file *
 find_trace_probe_file(struct trace_kprobe *tk, struct trace_array *tr)
 {

kernel/trace/trace_kprobe_selftest.c new file +10 -0

new file mode 100644
index 000000000000..16548ee4c8c6
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Function used during the kprobe self test. This function is in a separate
+ * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
+ * can be probed by the selftests.
+ */
+int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6)
+{
+	return a1 + a2 + a3 + a4 + a5 + a6;
+}

kernel/trace/trace_kprobe_selftest.h new file +7 -0

new file mode 100644
index 000000000000..4e10ec41c013
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Function used during the kprobe self test. This function is in a separate
+ * compile unit so it can be compile with CC_FLAGS_FTRACE to ensure that it
+ * can be probed by the selftests.
+ */
+int kprobe_trace_selftest_target(int a1, int a2, int a3, int a4, int a5, int a6);

Masami Hiramatsu

author maintainer

2018-07-30 10:21 UTC #5 lore raw

PATCH v5 3/3 selftests/ftrace: Fix kprobe string testcase to not probe notrace function

In reply to Masami Hiramatsu

Fix kprobe string argument testcase to not probe notrace function. Instead, it probes tracefs function which must be available with ftrace.

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes in v3:
- Fix probepoint testcase too.

---
 .../ftrace/test.d/kprobe/kprobe_args_string.tc     |   30 ++++++++------------
 .../selftests/ftrace/test.d/kprobe/probepoint.tc   |    2 +
 2 files changed, 13 insertions(+), 19 deletions(-)

tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_string.tc +12 ~~-18~~

index a0002563e9ee..1ad70cdaf442 100644
@@ -9,28 +9,22 @@ echo > kprobe_events
 
 case `uname -m` in
 x86_64)
-  ARG2=%si
-  OFFS=8
+  ARG1=%di
 ;;
 i[3456]86)
-  ARG2=%cx
-  OFFS=4
+  ARG1=%ax
 ;;
 aarch64)
-  ARG2=%x1
-  OFFS=8
+  ARG1=%x0
 ;;
 arm*)
-  ARG2=%r1
-  OFFS=4
+  ARG1=%r0
 ;;
 ppc64*)
-  ARG2=%r4
-  OFFS=8
+  ARG1=%r3
 ;;
 ppc*)
-  ARG2=%r4
-  OFFS=4
+  ARG1=%r3
 ;;
 *)
   echo "Please implement other architecture here"

@@ -38,17 +32,17 @@ ppc*)
 esac
 
 : "Test get argument (1)"
-echo "p:testprobe create_trace_kprobe arg1=+0(+0(${ARG2})):string" > kprobe_events
+echo "p:testprobe tracefs_create_dir arg1=+0(${ARG1}):string" > kprobe_events
 echo 1 > events/kprobes/testprobe/enable
-! echo test >> kprobe_events
-tail -n 1 trace | grep -qe "testprobe.* arg1=\"test\""
+echo "p:test _do_fork" >> kprobe_events
+grep -qe "testprobe.* arg1=\"test\"" trace
 
 echo 0 > events/kprobes/testprobe/enable
 : "Test get argument (2)"
-echo "p:testprobe create_trace_kprobe arg1=+0(+0(${ARG2})):string arg2=+0(+${OFFS}(${ARG2})):string" > kprobe_events
+echo "p:testprobe tracefs_create_dir arg1=+0(${ARG1}):string arg2=+0(${ARG1}):string" > kprobe_events
 echo 1 > events/kprobes/testprobe/enable
-! echo test1 test2 >> kprobe_events
-tail -n 1 trace | grep -qe "testprobe.* arg1=\"test1\" arg2=\"test2\""
+echo "p:test _do_fork" >> kprobe_events
+grep -qe "testprobe.* arg1=\"test\" arg2=\"test\"" trace
 
 echo 0 > events/enable
 echo > kprobe_events

tools/testing/selftests/ftrace/test.d/kprobe/probepoint.tc +1 -1

index 4fda01a08da4..519d2763f5d2 100644
@@ -4,7 +4,7 @@
 
 [ -f kprobe_events ] || exit_unsupported # this is configurable
 
-TARGET_FUNC=create_trace_kprobe
+TARGET_FUNC=tracefs_create_dir
 
 dec_addr() { # hexaddr
   printf "%d" "0x"`echo $1 | tail -c 8`

Francis Deslauriers

Tested-by on the v5 series

2018-07-30 16:06 UTC #6 lore raw

In reply to Masami Hiramatsu

Hi Masami,
I just tested the patch-set and it still prevents the crash I was witnessing. You can add my tested-by.

Tested-by Francis Deslauriers <[email protected]>

Thank you for pushing this forward!
Francis

Quoted 45 lines · Masami Hiramatsu

Hi,
This is the 5th version of the series to prohibit kprobe on notrace functions.
This fixes a build error when CONFIG_FUNCTION_TRACER=y but DYNAMIC_FTRACE=n. Finally I decided to recover the approach in the 1st version, made it depends on CONFIG_KPROBES_ON_FTRACE=y. It is the simplest and enough to prohibiting kernel crash.
So, in summary, if CONFIG_KPROBES_ON_FTRACE=y (which depends on CONFIG_DYNAMIC_FTRACE=y, so ftrace_location_range must be there) && CONFIG_KPROBE_EVENTS_ON_NOTRACE=n (default), kprobe events can not be defined on notrace function. Otherwides (means CONFIG_KPROBES_ON_FTRACE=n or CONFIG_KPROBE_EVENTS_ON_NOTRACE=y), we can put kprobe events on notrace functions.
Francis, I dropped your tested-by because I got many kbuild errors and fixed it. If you can, could you test it again?
Thank you,
---
Francis Deslauriers (1):
selftest/ftrace: Move kprobe selftest function to separate compile unit
Masami Hiramatsu (2):
tracing: kprobes: Prohibit probing on notrace function selftests/ftrace: Fix kprobe string testcase to not probe notrace function
 kernel/trace/Kconfig                               |   20 +++++++
 kernel/trace/Makefile                              |    5 ++
 kernel/trace/trace_kprobe.c                        |   59 +++++++++++++-------
 kernel/trace/trace_kprobe_selftest.c               |   10 +++
 kernel/trace/trace_kprobe_selftest.h               |    7 ++
 .../ftrace/test.d/kprobe/kprobe_args_string.tc     |   30 ++++------
 .../selftests/ftrace/test.d/kprobe/probepoint.tc   |    2 -
 7 files changed, 94 insertions(+), 39 deletions(-)
 create mode 100644 kernel/trace/trace_kprobe_selftest.c
 create mode 100644 kernel/trace/trace_kprobe_selftest.h

Quoted 2 lines

--
Masami Hiramatsu (Linaro) <[email protected]>

--
Francis Deslauriers
Software developer
EfficiOS inc.

Masami Hiramatsu

author maintainer

2018-07-30 23:01 UTC #7 lore raw

Quoted 4 lines · On Mon, 30 Jul 2018 12:06:17 -0400 Francis Deslauriers…

Hi Masami,
I just tested the patch-set and it still prevents the crash I was witnessing. You can add my tested-by.
Tested-by Francis Deslauriers <[email protected]>

Thanks Francis!

Quoted 59 lines

Thank you for pushing this forward!
Francis
Quoted 45 lines · Masami Hiramatsu
Hi,
This is the 5th version of the series to prohibit kprobe on notrace functions.
This fixes a build error when CONFIG_FUNCTION_TRACER=y but DYNAMIC_FTRACE=n. Finally I decided to recover the approach in the 1st version, made it depends on CONFIG_KPROBES_ON_FTRACE=y. It is the simplest and enough to prohibiting kernel crash.
So, in summary, if CONFIG_KPROBES_ON_FTRACE=y (which depends on CONFIG_DYNAMIC_FTRACE=y, so ftrace_location_range must be there) && CONFIG_KPROBE_EVENTS_ON_NOTRACE=n (default), kprobe events can not be defined on notrace function. Otherwides (means CONFIG_KPROBES_ON_FTRACE=n or CONFIG_KPROBE_EVENTS_ON_NOTRACE=y), we can put kprobe events on notrace functions.
Francis, I dropped your tested-by because I got many kbuild errors and fixed it. If you can, could you test it again?
Thank you,
---
Francis Deslauriers (1):
selftest/ftrace: Move kprobe selftest function to separate compile unit
Masami Hiramatsu (2):
tracing: kprobes: Prohibit probing on notrace function selftests/ftrace: Fix kprobe string testcase to not probe notrace function
 kernel/trace/Kconfig                               |   20 +++++++
 kernel/trace/Makefile                              |    5 ++
 kernel/trace/trace_kprobe.c                        |   59 +++++++++++++-------
 kernel/trace/trace_kprobe_selftest.c               |   10 +++
 kernel/trace/trace_kprobe_selftest.h               |    7 ++
 .../ftrace/test.d/kprobe/kprobe_args_string.tc     |   30 ++++------
 .../selftests/ftrace/test.d/kprobe/probepoint.tc   |    2 -
 7 files changed, 94 insertions(+), 39 deletions(-)
 create mode 100644 kernel/trace/trace_kprobe_selftest.c
 create mode 100644 kernel/trace/trace_kprobe_selftest.h
Quoted 2 lines
--
Masami Hiramatsu (Linaro) <[email protected]>
--
Francis Deslauriers
Software developer
EfficiOS inc.

Signature · 1 line

Masami Hiramatsu <[email protected]>