* [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist @ 2017-07-14 14:58 Francis Deslauriers 2017-07-14 14:58 ` [PATCH 1/2] kprobe: fix: Add _ASM_NOKPROBE to x86 apic interrupt macro Francis Deslauriers ` (3 more replies) 0 siblings, 4 replies; 35+ messages in thread From: Francis Deslauriers @ 2017-07-14 14:58 UTC (permalink / raw) To: rostedt, mhiramat, peterz Cc: mathieu.desnoyers, linux-kernel, Francis Deslauriers Hi all, While fuzzing the Perf kprobe and kretprobe interfaces, I found some inputs that trigger crashes of a 4.12 kernel(6f7da290413ba713f0cdd9ff1a2a9bb129ef4f6c) on a x86-64 VM. I know that K(ret)probes can crash the kernel in multiple ways but should Perf be allowed to do it? To do this analysis, I used the symbols reported by /proc/kallsyms in conjonction with the Perf debugfs interface. Using this technique, I was able to find two instrumentation configurations that could crash the kernel. I am suggesting changes that fixed both issues for me by blacklisting the symbols in question. Kprobe on apic_timer_interrupt: I believe that this is caused by the fact that kprobe adds a INT3 in a apic interrupt routine. How to reproduce: echo 'p:event1 apic_timer_interrupt ' > kprobe_events <Generate kernel activity. e.g. launch bash> Crash log:[1] This can be fixed by blacklisting the apicinterrupt3 symbols directly in the assembly macro. See patch[1/2]. I am not sure that blacklisting all apicinterrupt symbols is the right solution. Kretprobe on ftrace_ops_assist_func and another function: Those crashes are triggered when hooking a kretprobe on the ftrace_ops_assist_func symbol and some other functions to make the this first function reacheable. From my understanding, ftrace_ops_assist_func is the function called directly when the kprobe is hit. Thus it should be marked with NOKPROBE_SYMBOL. Here are some configurations that can easily reproduce this bug. Those other functions are called during the fork of a process so they are easy to control. Enable the following kprobes and launch a process to trigger a fork to see the kernel crash. Conf #1 echo 'r:event1 ftrace_ops_assist_func' > kprobe_events echo 'r:event2 clear_all_latency_tracing' > kprobe_events Crash log:[2] Conf #2 echo 'r:event1 ftrace_ops_assist_func' > kprobe_events echo 'r:event2 acct_clear_integrals' > kprobe_events Crash log:[3] Conf #3 echo 'r:event1 ftrace_ops_assist_func' > kprobe_events echo 'r:event2 arch_dup_task_struct' > kprobe_events Crash log:[4] The ftrace_ops_assist_func should be included in the kprobe blacklist using NOKPROBE_SYMBOL. See patch [2/2]. Since those were found using fuzzing, it's not an exhaustive analysis. Here is the .config I am using[5]. Thanks, Francis Deslauriers EfficiOS inc. [1]: https://pastebin.com/Mpp9Yzqb [2]: https://pastebin.com/CtsfzUwG [3]: https://pastebin.com/txxuJXrz [4]: https://pastebin.com/8qrJvzD3 [5]: https://pastebin.com/x5q0sgyK Francis Deslauriers (2): kprobe: fix: Add _ASM_NOKPROBE to x86 apic interrupt macro kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist arch/x86/entry/entry_64.S | 1 + kernel/trace/ftrace.c | 2 ++ 2 files changed, 3 insertions(+) -- 2.7.4 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/2] kprobe: fix: Add _ASM_NOKPROBE to x86 apic interrupt macro 2017-07-14 14:58 [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist Francis Deslauriers @ 2017-07-14 14:58 ` Francis Deslauriers 2017-07-14 14:58 ` [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist Francis Deslauriers ` (2 subsequent siblings) 3 siblings, 0 replies; 35+ messages in thread From: Francis Deslauriers @ 2017-07-14 14:58 UTC (permalink / raw) To: rostedt, mhiramat, peterz Cc: mathieu.desnoyers, linux-kernel, Francis Deslauriers Adding a Kprobe on the apic_timer_interrupt symbol can lead to a kernel crash. This symbol is defined by the apicinterrupt3 macro and adding the symbol to the kprobe blacklist in this macro prevents this issue. Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com> --- arch/x86/entry/entry_64.S | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index 4a4c083..67cf702 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -659,6 +659,7 @@ ENTRY(\sym) interrupt \do_sym jmp ret_from_intr END(\sym) +_ASM_NOKPROBE(\sym) .endm #ifdef CONFIG_TRACING -- 2.7.4 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2017-07-14 14:58 [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist Francis Deslauriers 2017-07-14 14:58 ` [PATCH 1/2] kprobe: fix: Add _ASM_NOKPROBE to x86 apic interrupt macro Francis Deslauriers @ 2017-07-14 14:58 ` Francis Deslauriers 2017-07-14 18:29 ` Steven Rostedt 2018-07-12 17:54 ` [PATCH 0/2] tracing: kprobes: Prohibit probing on notrace functions Francis Deslauriers 2017-07-14 18:27 ` [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist Steven Rostedt 2017-07-16 14:37 ` Masami Hiramatsu 3 siblings, 2 replies; 35+ messages in thread From: Francis Deslauriers @ 2017-07-14 14:58 UTC (permalink / raw) To: rostedt, mhiramat, peterz Cc: mathieu.desnoyers, linux-kernel, Francis Deslauriers This function is called when a kprobe is hit. Thus it should be blacklisted to prevent kprobe to be triggered by kprobes. Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com> --- kernel/trace/ftrace.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index b308be3..c473d9b 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -36,6 +36,7 @@ #include <trace/events/sched.h> +#include <asm/kprobes.h> #include <asm/sections.h> #include <asm/setup.h> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip, preempt_enable_notrace(); trace_clear_recursion(bit); } +NOKPROBE_SYMBOL(ftrace_ops_assist_func); /** * ftrace_ops_get_func - get the function a trampoline should call -- 2.7.4 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2017-07-14 14:58 ` [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist Francis Deslauriers @ 2017-07-14 18:29 ` Steven Rostedt 2018-03-16 15:18 ` Francis Deslauriers 2018-07-12 17:54 ` [PATCH 0/2] tracing: kprobes: Prohibit probing on notrace functions Francis Deslauriers 1 sibling, 1 reply; 35+ messages in thread From: Steven Rostedt @ 2017-07-14 18:29 UTC (permalink / raw) To: Francis Deslauriers; +Cc: mhiramat, peterz, mathieu.desnoyers, linux-kernel On Fri, 14 Jul 2017 10:58:35 -0400 Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > This function is called when a kprobe is hit. Thus it should be > blacklisted to prevent kprobe to be triggered by kprobes. > > Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com> > --- > kernel/trace/ftrace.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index b308be3..c473d9b 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -36,6 +36,7 @@ > > #include <trace/events/sched.h> > > +#include <asm/kprobes.h> > #include <asm/sections.h> > #include <asm/setup.h> > > @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip, > preempt_enable_notrace(); > trace_clear_recursion(bit); > } > +NOKPROBE_SYMBOL(ftrace_ops_assist_func); Continuing from what I said in the other email, this is fixing a symptom and not the problem. The real fix will be much more involved. I have a good idea on how to accomplish it too. -- Steve > > /** > * ftrace_ops_get_func - get the function a trampoline should call ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2017-07-14 18:29 ` Steven Rostedt @ 2018-03-16 15:18 ` Francis Deslauriers 2018-03-16 15:25 ` Steven Rostedt 0 siblings, 1 reply; 35+ messages in thread From: Francis Deslauriers @ 2018-03-16 15:18 UTC (permalink / raw) To: Steven Rostedt; +Cc: Masami Hiramatsu, peterz, Mathieu Desnoyers, linux-kernel Hi Steven, I completely forgot about this issue until recently when I encountered it again. Instrumenting the ftrace_ops_assist_func symbol and some other symbol seems to be causing problems. Placing kretprobes like in the following configuration crashes my kernel (4.16.0-rc5) on a Qemu/KVM virtual machine: config 1: echo "r:event_1 __fdget" >> kprobe_events echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events config 2: echo "r:event_1 __fdget_pos" >> kprobe_events echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events config 3: echo 'r:event_1 arch_dup_task_struct' >> kprobe_events echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events config 4: echo 'r:event_1 sys_open' >> kprobe_events echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events Here is my kernel config [1]: In a previous email [2], you mentioned that you would like to add the ftrace-related symbols to a section to un-blacklist them all at once on demand but wanted to discuss it at Linux Plumbers. Do you still think that it's the right approach? I can easily test any patch regarding this issue. [1] http://paste.ubuntu.com/p/BJWvgMnW8z/ [2] https://lkml.org/lkml/2017/7/14/568 Thank you, 2017-07-14 14:29 GMT-04:00 Steven Rostedt <rostedt@goodmis.org>: > On Fri, 14 Jul 2017 10:58:35 -0400 > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > >> This function is called when a kprobe is hit. Thus it should be >> blacklisted to prevent kprobe to be triggered by kprobes. >> >> Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com> >> --- >> kernel/trace/ftrace.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> index b308be3..c473d9b 100644 >> --- a/kernel/trace/ftrace.c >> +++ b/kernel/trace/ftrace.c >> @@ -36,6 +36,7 @@ >> >> #include <trace/events/sched.h> >> >> +#include <asm/kprobes.h> >> #include <asm/sections.h> >> #include <asm/setup.h> >> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip, >> preempt_enable_notrace(); >> trace_clear_recursion(bit); >> } >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func); > > Continuing from what I said in the other email, this is fixing a > symptom and not the problem. The real fix will be much more involved. I > have a good idea on how to accomplish it too. > > -- Steve > > >> >> /** >> * ftrace_ops_get_func - get the function a trampoline should call > -- Francis Deslauriers Software developer EfficiOS inc. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-16 15:18 ` Francis Deslauriers @ 2018-03-16 15:25 ` Steven Rostedt 2018-03-16 16:28 ` Mathieu Desnoyers 0 siblings, 1 reply; 35+ messages in thread From: Steven Rostedt @ 2018-03-16 15:25 UTC (permalink / raw) To: Francis Deslauriers Cc: Masami Hiramatsu, peterz, Mathieu Desnoyers, linux-kernel On Fri, 16 Mar 2018 11:18:25 -0400 Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > Hi Steven, > > I completely forgot about this issue until recently when I encountered it again. > Instrumenting the ftrace_ops_assist_func symbol and some other symbol > seems to be causing problems. > > Placing kretprobes like in the following configuration crashes my > kernel (4.16.0-rc5) on a Qemu/KVM virtual machine: > > config 1: > echo "r:event_1 __fdget" >> kprobe_events > echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events > > config 2: > echo "r:event_1 __fdget_pos" >> kprobe_events > echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events > > config 3: > echo 'r:event_1 arch_dup_task_struct' >> kprobe_events > echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events > > config 4: > echo 'r:event_1 sys_open' >> kprobe_events > echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events > > Here is my kernel config [1]: > > In a previous email [2], you mentioned that you would like to add the > ftrace-related symbols to a section to un-blacklist them all at once > on demand but wanted to discuss it at Linux Plumbers. Do you still > think that it's the right approach? We probably didn't discuss it (as there was a lot to discuss, and this was probably overshadowed by that). But yes, you should not probe ftrace called functions. That is guaranteed to crash and that crash is not a bug, but a feature. The ftrace and ring buffer files should be blacklisted from being probed. Perhaps the entire directory. Anyway, I don't see this as much of an urgent matter, as it's one of those "Patient: Doc, it hurts when I do this. Doc: Don't do that" cases. And there's a lot of urgent issues that currently need to be dealt with. -- Steve > > I can easily test any patch regarding this issue. > > [1] http://paste.ubuntu.com/p/BJWvgMnW8z/ > [2] https://lkml.org/lkml/2017/7/14/568 > > Thank you, > > 2017-07-14 14:29 GMT-04:00 Steven Rostedt <rostedt@goodmis.org>: > > On Fri, 14 Jul 2017 10:58:35 -0400 > > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > > > >> This function is called when a kprobe is hit. Thus it should be > >> blacklisted to prevent kprobe to be triggered by kprobes. > >> > >> Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com> > >> --- > >> kernel/trace/ftrace.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > >> index b308be3..c473d9b 100644 > >> --- a/kernel/trace/ftrace.c > >> +++ b/kernel/trace/ftrace.c > >> @@ -36,6 +36,7 @@ > >> > >> #include <trace/events/sched.h> > >> > >> +#include <asm/kprobes.h> > >> #include <asm/sections.h> > >> #include <asm/setup.h> > >> > >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip, > >> preempt_enable_notrace(); > >> trace_clear_recursion(bit); > >> } > >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func); > > > > Continuing from what I said in the other email, this is fixing a > > symptom and not the problem. The real fix will be much more involved. I > > have a good idea on how to accomplish it too. > > > > -- Steve > > > > > >> > >> /** > >> * ftrace_ops_get_func - get the function a trampoline should call > > > > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-16 15:25 ` Steven Rostedt @ 2018-03-16 16:28 ` Mathieu Desnoyers 2018-03-16 16:41 ` Steven Rostedt 2018-03-17 0:08 ` Masami Hiramatsu 0 siblings, 2 replies; 35+ messages in thread From: Mathieu Desnoyers @ 2018-03-16 16:28 UTC (permalink / raw) To: rostedt Cc: Francis Deslauriers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, Linus Torvalds ----- On Mar 16, 2018, at 11:25 AM, rostedt rostedt@goodmis.org wrote: > On Fri, 16 Mar 2018 11:18:25 -0400 > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > >> Hi Steven, >> >> I completely forgot about this issue until recently when I encountered it again. >> Instrumenting the ftrace_ops_assist_func symbol and some other symbol >> seems to be causing problems. >> >> Placing kretprobes like in the following configuration crashes my >> kernel (4.16.0-rc5) on a Qemu/KVM virtual machine: >> >> config 1: >> echo "r:event_1 __fdget" >> kprobe_events >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events >> >> config 2: >> echo "r:event_1 __fdget_pos" >> kprobe_events >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events >> >> config 3: >> echo 'r:event_1 arch_dup_task_struct' >> kprobe_events >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events >> >> config 4: >> echo 'r:event_1 sys_open' >> kprobe_events >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events >> >> Here is my kernel config [1]: >> >> In a previous email [2], you mentioned that you would like to add the >> ftrace-related symbols to a section to un-blacklist them all at once >> on demand but wanted to discuss it at Linux Plumbers. Do you still >> think that it's the right approach? > > We probably didn't discuss it (as there was a lot to discuss, and this > was probably overshadowed by that). But yes, you should not probe > ftrace called functions. That is guaranteed to crash and that crash is > not a bug, but a feature. Are you really arguing that crashing the kernel from an ABI visible from userspace (even if it's only root user) is not a bug ? You are joking right ? Is there an EXPERIMENTAL config option that people need to select in order to make sure those ftrace interfaces don't end up on production systems ? > > The ftrace and ring buffer files should be blacklisted from being > probed. Perhaps the entire directory. All code reachable from a kprobe handler should be blacklisted from kprobes, yes. > > Anyway, I don't see this as much of an urgent matter, as it's one of > those "Patient: Doc, it hurts when I do this. Doc: Don't do that" > cases. And there's a lot of urgent issues that currently need to be > dealt with. OK, short-term we'll remove everything related to ftrace functions from our CI fuzzer coverage. Arguably, the fact that a root user can crash the kernel through tracefs files is not that great security-wise though. Considering that our current focus is to test the kprobe instrumentation layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI instead, which should take care of removing crashes introduced by ftrace from our fuzzing results. Thanks, Mathieu > > -- Steve > > >> >> I can easily test any patch regarding this issue. >> >> [1] http://paste.ubuntu.com/p/BJWvgMnW8z/ >> [2] https://lkml.org/lkml/2017/7/14/568 >> >> Thank you, >> >> 2017-07-14 14:29 GMT-04:00 Steven Rostedt <rostedt@goodmis.org>: >> > On Fri, 14 Jul 2017 10:58:35 -0400 >> > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: >> > >> >> This function is called when a kprobe is hit. Thus it should be >> >> blacklisted to prevent kprobe to be triggered by kprobes. >> >> >> >> Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com> >> >> --- >> >> kernel/trace/ftrace.c | 2 ++ >> >> 1 file changed, 2 insertions(+) >> >> >> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> >> index b308be3..c473d9b 100644 >> >> --- a/kernel/trace/ftrace.c >> >> +++ b/kernel/trace/ftrace.c >> >> @@ -36,6 +36,7 @@ >> >> >> >> #include <trace/events/sched.h> >> >> >> >> +#include <asm/kprobes.h> >> >> #include <asm/sections.h> >> >> #include <asm/setup.h> >> >> >> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, >> >> unsigned long parent_ip, >> >> preempt_enable_notrace(); >> >> trace_clear_recursion(bit); >> >> } >> >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func); >> > >> > Continuing from what I said in the other email, this is fixing a >> > symptom and not the problem. The real fix will be much more involved. I >> > have a good idea on how to accomplish it too. >> > >> > -- Steve >> > >> > >> >> >> >> /** >> >> * ftrace_ops_get_func - get the function a trampoline should call >> > >> >> -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-16 16:28 ` Mathieu Desnoyers @ 2018-03-16 16:41 ` Steven Rostedt 2018-03-16 16:48 ` Steven Rostedt 2018-03-17 0:08 ` Masami Hiramatsu 1 sibling, 1 reply; 35+ messages in thread From: Steven Rostedt @ 2018-03-16 16:41 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Francis Deslauriers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, Linus Torvalds On Fri, 16 Mar 2018 12:28:59 -0400 (EDT) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > We probably didn't discuss it (as there was a lot to discuss, and this > > was probably overshadowed by that). But yes, you should not probe > > ftrace called functions. That is guaranteed to crash and that crash is > > not a bug, but a feature. > > Are you really arguing that crashing the kernel from an ABI visible from > userspace (even if it's only root user) is not a bug ? You are joking right ? > Is there an EXPERIMENTAL config option that people need to select in order to > make sure those ftrace interfaces don't end up on production systems ? No I'm not. And yes there is. Disable kprobes. kprobes is much more dangerous than ftrace, and its kprobes that is crashing not ftrace. Heck we have "echo c > /proc/sysrq-trigger" So yes, you can easily crash the kernel via root. If you can load a module, you can crash the kernel. There's a thousand ways to crash a kernel. This is why most of the fuzzer testing is done as non-root, because doing it as root will do more than just crash the system, it may corrupt it enough that you can no longer boot it. I see below you are doing fuzzing testing too as root. Hopefully you limit those tests because yes, things can get really bad. > > > > > The ftrace and ring buffer files should be blacklisted from being > > probed. Perhaps the entire directory. > > All code reachable from a kprobe handler should be blacklisted from > kprobes, yes. The problem is that that list constantly changes. There's been cases we try to prevent things called by nmi do not get called, but it ended up being every helper utility can be called in that context. > > > > > Anyway, I don't see this as much of an urgent matter, as it's one of > > those "Patient: Doc, it hurts when I do this. Doc: Don't do that" > > cases. And there's a lot of urgent issues that currently need to be > > dealt with. > > OK, short-term we'll remove everything related to ftrace functions > from our CI fuzzer coverage. Arguably, the fact that a root user can > crash the kernel through tracefs files is not that great security-wise > though. Note, probes are not a normal API. I test the hell out of the other ftrace interfaces and if it blows up I fix it. But adding probes into random parts of the kernel is very dangerous, and not something I care to test. And sure, if you are worried about root killing the system, disable kprobes. > > Considering that our current focus is to test the kprobe instrumentation > layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI > instead, which should take care of removing crashes introduced by ftrace > from our fuzzing results. Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm saying that I don't have time to fix it now, but would be happy to accept patches if someone else does so. -- Steve ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-16 16:41 ` Steven Rostedt @ 2018-03-16 16:48 ` Steven Rostedt 2018-03-16 17:53 ` Mathieu Desnoyers 0 siblings, 1 reply; 35+ messages in thread From: Steven Rostedt @ 2018-03-16 16:48 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Francis Deslauriers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, Linus Torvalds On Fri, 16 Mar 2018 12:41:34 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm > saying that I don't have time to fix it now, but would be happy to > accept patches if someone else does so. And looking at what I replied before for the original patch. It would probably be a good idea to blacklist directories. Like we do with function tracing. We probably should black list both kernel/tracing and kernel/events from being probed. Did this come up at plumbers? You were there too, I don't remember discussing it there. -- Steve ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-16 16:48 ` Steven Rostedt @ 2018-03-16 17:53 ` Mathieu Desnoyers 2018-03-16 19:02 ` Steven Rostedt 2018-03-17 0:13 ` Masami Hiramatsu 0 siblings, 2 replies; 35+ messages in thread From: Mathieu Desnoyers @ 2018-03-16 17:53 UTC (permalink / raw) To: rostedt Cc: Francis Deslauriers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, Linus Torvalds ----- On Mar 16, 2018, at 12:48 PM, rostedt rostedt@goodmis.org wrote: > On Fri, 16 Mar 2018 12:41:34 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm >> saying that I don't have time to fix it now, but would be happy to >> accept patches if someone else does so. > > And looking at what I replied before for the original patch. It would > probably be a good idea to blacklist directories. Like we do with > function tracing. We probably should black list both kernel/tracing and > kernel/events from being probed. > > Did this come up at plumbers? You were there too, I don't remember > discussing it there. I don't remember this coming up last Plumbers nor KS neither, given that we were focused on other topics. Would the general approach you envision be based on emitting all code generated by compilation of all objects under kernel/tracing and kernel/events into a specific "nokprobes" text section of the kernel ? Perhaps we could create a specific linker scripts for those directories, or do you have in mind a neater way to do this ? Thanks, Mathieu > > -- Steve -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-16 17:53 ` Mathieu Desnoyers @ 2018-03-16 19:02 ` Steven Rostedt 2018-03-17 0:13 ` Masami Hiramatsu 1 sibling, 0 replies; 35+ messages in thread From: Steven Rostedt @ 2018-03-16 19:02 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Francis Deslauriers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, Linus Torvalds On Fri, 16 Mar 2018 13:53:01 -0400 (EDT) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > Would the general approach you envision be based on emitting all code > generated by compilation of all objects under kernel/tracing and > kernel/events into a specific "nokprobes" text section of the kernel ? > Perhaps we could create a specific linker scripts for those directories, > or do you have in mind a neater way to do this ? I was thinking of adding it to the objtool work. I need to consolidate the recordmcount code and objtool for doing this, and that is on my agenda (it has to do with some of the current "urgent" needs). But this will take a bit of effort. In the mean time and not against just adding the whack-a-mole approach and add the functions that the original patch selected as nokprobes. -- Steve ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-16 17:53 ` Mathieu Desnoyers 2018-03-16 19:02 ` Steven Rostedt @ 2018-03-17 0:13 ` Masami Hiramatsu 2018-03-17 1:22 ` Masami Hiramatsu 1 sibling, 1 reply; 35+ messages in thread From: Masami Hiramatsu @ 2018-03-17 0:13 UTC (permalink / raw) To: Mathieu Desnoyers Cc: rostedt, Francis Deslauriers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, Linus Torvalds On Fri, 16 Mar 2018 13:53:01 -0400 (EDT) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > ----- On Mar 16, 2018, at 12:48 PM, rostedt rostedt@goodmis.org wrote: > > > On Fri, 16 Mar 2018 12:41:34 -0400 > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm > >> saying that I don't have time to fix it now, but would be happy to > >> accept patches if someone else does so. > > > > And looking at what I replied before for the original patch. It would > > probably be a good idea to blacklist directories. Like we do with > > function tracing. We probably should black list both kernel/tracing and > > kernel/events from being probed. > > > > Did this come up at plumbers? You were there too, I don't remember > > discussing it there. > > I don't remember this coming up last Plumbers nor KS neither, given > that we were focused on other topics. > > Would the general approach you envision be based on emitting all code > generated by compilation of all objects under kernel/tracing and > kernel/events into a specific "nokprobes" text section of the kernel ? > Perhaps we could create a specific linker scripts for those directories, > or do you have in mind a neater way to do this ? .kprobes.text section still exists. As I pointed in previous mail, I don't think we have to put all those code into that section. But if you want, it is acceptable to have a kconfig which push most of those ftrace related code into .kprobes.text section. Thank, > > Thanks, > > Mathieu > > > > > -- Steve > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-17 0:13 ` Masami Hiramatsu @ 2018-03-17 1:22 ` Masami Hiramatsu 2018-03-17 3:01 ` Steven Rostedt 2018-07-03 22:30 ` Steven Rostedt 0 siblings, 2 replies; 35+ messages in thread From: Masami Hiramatsu @ 2018-03-17 1:22 UTC (permalink / raw) To: Masami Hiramatsu Cc: Mathieu Desnoyers, rostedt, Francis Deslauriers, Peter Zijlstra, linux-kernel, Linus Torvalds On Sat, 17 Mar 2018 09:13:34 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT) > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > ----- On Mar 16, 2018, at 12:48 PM, rostedt rostedt@goodmis.org wrote: > > > > > On Fri, 16 Mar 2018 12:41:34 -0400 > > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm > > >> saying that I don't have time to fix it now, but would be happy to > > >> accept patches if someone else does so. > > > > > > And looking at what I replied before for the original patch. It would > > > probably be a good idea to blacklist directories. Like we do with > > > function tracing. We probably should black list both kernel/tracing and > > > kernel/events from being probed. > > > > > > Did this come up at plumbers? You were there too, I don't remember > > > discussing it there. > > > > I don't remember this coming up last Plumbers nor KS neither, given > > that we were focused on other topics. > > > > Would the general approach you envision be based on emitting all code > > generated by compilation of all objects under kernel/tracing and > > kernel/events into a specific "nokprobes" text section of the kernel ? > > Perhaps we could create a specific linker scripts for those directories, > > or do you have in mind a neater way to do this ? > > .kprobes.text section still exists. As I pointed in previous mail, I don't > think we have to put all those code into that section. But if you want, > it is acceptable to have a kconfig which push most of those ftrace related > code into .kprobes.text section. Or, we can check it by ftrace_location_range() as below patch. Note that as a side-effect, we can not trace functions in trace_kprobe.c anymore, and this means it is hard to me to make a testcase of kprobe events. It was the easiest (and maintainable) way to make test cases... sigh. ----- tracing: kprobes: Prohibit probing on notrace function From: Masami Hiramatsu <mhiramat@kernel.org> 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. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> --- kernel/trace/trace_kprobe.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 5ce9b8cf7be3..7404b012e51a 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file) return ret; } +#ifdef CONFIG_KPROBES_ON_FTRACE +static bool within_notrace_func(struct trace_kprobe *tk) +{ + unsigned long offset, size, addr; + + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk)); + addr += trace_kprobe_offset(tk); + + if (!kallsyms_lookup_size_offset(addr, &size, &offset)) + return true; /* Out of range. */ + + return !ftrace_location_range(addr - offset, addr - offset + size); +} +#else +#define within_notrace_func(tk) (false) +#endif + /* Internal register function - just handle k*probes and flags */ static int __register_trace_kprobe(struct trace_kprobe *tk) { @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk) if (trace_probe_is_registered(&tk->tp)) return -EINVAL; + if (within_notrace_func(tk)) { + pr_warn("Could not probe notrace function %s\n", + trace_kprobe_symbol(tk)); + return -EINVAL; + } + for (i = 0; i < tk->tp.nr_args; i++) traceprobe_update_arg(&tk->tp.args[i]); -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-17 1:22 ` Masami Hiramatsu @ 2018-03-17 3:01 ` Steven Rostedt 2018-03-17 7:57 ` Masami Hiramatsu 2018-07-03 22:30 ` Steven Rostedt 1 sibling, 1 reply; 35+ messages in thread From: Steven Rostedt @ 2018-03-17 3:01 UTC (permalink / raw) To: Masami Hiramatsu Cc: Mathieu Desnoyers, Francis Deslauriers, Peter Zijlstra, linux-kernel, Linus Torvalds On Sat, 17 Mar 2018 10:22:11 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > Or, we can check it by ftrace_location_range() as below patch. Cute ;-) > > Note that as a side-effect, we can not trace functions in trace_kprobe.c > anymore, and this means it is hard to me to make a testcase of kprobe events. > It was the easiest (and maintainable) way to make test cases... sigh. > > ----- > tracing: kprobes: Prohibit probing on notrace function > > From: Masami Hiramatsu <mhiramat@kernel.org> > > 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. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > --- > kernel/trace/trace_kprobe.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 5ce9b8cf7be3..7404b012e51a 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file) > return ret; > } > > +#ifdef CONFIG_KPROBES_ON_FTRACE > +static bool within_notrace_func(struct trace_kprobe *tk) > +{ > + unsigned long offset, size, addr; > + > + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk)); > + addr += trace_kprobe_offset(tk); > + > + if (!kallsyms_lookup_size_offset(addr, &size, &offset)) > + return true; /* Out of range. */ > + > + return !ftrace_location_range(addr - offset, addr - offset + size); > +} > +#else > +#define within_notrace_func(tk) (false) > +#endif > + > /* Internal register function - just handle k*probes and flags */ > static int __register_trace_kprobe(struct trace_kprobe *tk) > { > @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk) > if (trace_probe_is_registered(&tk->tp)) > return -EINVAL; > > + if (within_notrace_func(tk)) { > + pr_warn("Could not probe notrace function %s\n", > + trace_kprobe_symbol(tk)); > + return -EINVAL; > + } This will prevent probing assembly code. Do we want to do that? Or is kprobes already prohibited from doing so? -- Steve > + > for (i = 0; i < tk->tp.nr_args; i++) > traceprobe_update_arg(&tk->tp.args[i]); > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-17 3:01 ` Steven Rostedt @ 2018-03-17 7:57 ` Masami Hiramatsu 0 siblings, 0 replies; 35+ messages in thread From: Masami Hiramatsu @ 2018-03-17 7:57 UTC (permalink / raw) To: Steven Rostedt Cc: Mathieu Desnoyers, Francis Deslauriers, Peter Zijlstra, linux-kernel, Linus Torvalds On Fri, 16 Mar 2018 23:01:54 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Sat, 17 Mar 2018 10:22:11 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > Or, we can check it by ftrace_location_range() as below patch. > > Cute ;-) > > > > > Note that as a side-effect, we can not trace functions in trace_kprobe.c > > anymore, and this means it is hard to me to make a testcase of kprobe events. > > It was the easiest (and maintainable) way to make test cases... sigh. > > > > ----- > > tracing: kprobes: Prohibit probing on notrace function > > > > From: Masami Hiramatsu <mhiramat@kernel.org> > > > > 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. > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > --- > > kernel/trace/trace_kprobe.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 5ce9b8cf7be3..7404b012e51a 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file) > > return ret; > > } > > > > +#ifdef CONFIG_KPROBES_ON_FTRACE > > +static bool within_notrace_func(struct trace_kprobe *tk) > > +{ > > + unsigned long offset, size, addr; > > + > > + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk)); > > + addr += trace_kprobe_offset(tk); > > + > > + if (!kallsyms_lookup_size_offset(addr, &size, &offset)) > > + return true; /* Out of range. */ > > + > > + return !ftrace_location_range(addr - offset, addr - offset + size); > > +} > > +#else > > +#define within_notrace_func(tk) (false) > > +#endif > > + > > /* Internal register function - just handle k*probes and flags */ > > static int __register_trace_kprobe(struct trace_kprobe *tk) > > { > > @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk) > > if (trace_probe_is_registered(&tk->tp)) > > return -EINVAL; > > > > + if (within_notrace_func(tk)) { > > + pr_warn("Could not probe notrace function %s\n", > > + trace_kprobe_symbol(tk)); > > + return -EINVAL; > > + } > > This will prevent probing assembly code. Do we want to do that? Or is > kprobes already prohibited from doing so? No, kprobes itself can probe any assembly code except for the area where can cause recursive hit as I explained. So anyway we still need to mark those functions NOKPROBE_SYMBOL. And note that this just prevent to probe those notrace code *via kprobe_events* interface. So pure kprobe user (like systemtap) is not effected by this change. So, maybe we would better to provide another debug knob which skips above check. This is only for non-experienced admins :) Thanks, > > -- Steve > > > > + > > for (i = 0; i < tk->tp.nr_args; i++) > > traceprobe_update_arg(&tk->tp.args[i]); > > > > > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-17 1:22 ` Masami Hiramatsu 2018-03-17 3:01 ` Steven Rostedt @ 2018-07-03 22:30 ` Steven Rostedt 2018-07-11 19:34 ` Francis Deslauriers 1 sibling, 1 reply; 35+ messages in thread From: Steven Rostedt @ 2018-07-03 22:30 UTC (permalink / raw) To: Mathieu Desnoyers, Francis Deslauriers Cc: Masami Hiramatsu, Peter Zijlstra, linux-kernel, Linus Torvalds Mathieu and Francis, Looking back, this thread never got further. Would Masami's patch work for you? -- Steve On Sat, 17 Mar 2018 10:22:11 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Sat, 17 Mar 2018 09:13:34 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT) > > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > > ----- On Mar 16, 2018, at 12:48 PM, rostedt rostedt@goodmis.org wrote: > > > > > > > On Fri, 16 Mar 2018 12:41:34 -0400 > > > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm > > > >> saying that I don't have time to fix it now, but would be happy to > > > >> accept patches if someone else does so. > > > > > > > > And looking at what I replied before for the original patch. It would > > > > probably be a good idea to blacklist directories. Like we do with > > > > function tracing. We probably should black list both kernel/tracing and > > > > kernel/events from being probed. > > > > > > > > Did this come up at plumbers? You were there too, I don't remember > > > > discussing it there. > > > > > > I don't remember this coming up last Plumbers nor KS neither, given > > > that we were focused on other topics. > > > > > > Would the general approach you envision be based on emitting all code > > > generated by compilation of all objects under kernel/tracing and > > > kernel/events into a specific "nokprobes" text section of the kernel ? > > > Perhaps we could create a specific linker scripts for those directories, > > > or do you have in mind a neater way to do this ? > > > > .kprobes.text section still exists. As I pointed in previous mail, I don't > > think we have to put all those code into that section. But if you want, > > it is acceptable to have a kconfig which push most of those ftrace related > > code into .kprobes.text section. > > Or, we can check it by ftrace_location_range() as below patch. > > Note that as a side-effect, we can not trace functions in trace_kprobe.c > anymore, and this means it is hard to me to make a testcase of kprobe events. > It was the easiest (and maintainable) way to make test cases... sigh. > > ----- > tracing: kprobes: Prohibit probing on notrace function > > From: Masami Hiramatsu <mhiramat@kernel.org> > > 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. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > --- > kernel/trace/trace_kprobe.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 5ce9b8cf7be3..7404b012e51a 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file) > return ret; > } > > +#ifdef CONFIG_KPROBES_ON_FTRACE > +static bool within_notrace_func(struct trace_kprobe *tk) > +{ > + unsigned long offset, size, addr; > + > + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk)); > + addr += trace_kprobe_offset(tk); > + > + if (!kallsyms_lookup_size_offset(addr, &size, &offset)) > + return true; /* Out of range. */ > + > + return !ftrace_location_range(addr - offset, addr - offset + size); > +} > +#else > +#define within_notrace_func(tk) (false) > +#endif > + > /* Internal register function - just handle k*probes and flags */ > static int __register_trace_kprobe(struct trace_kprobe *tk) > { > @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk) > if (trace_probe_is_registered(&tk->tp)) > return -EINVAL; > > + if (within_notrace_func(tk)) { > + pr_warn("Could not probe notrace function %s\n", > + trace_kprobe_symbol(tk)); > + return -EINVAL; > + } > + > for (i = 0; i < tk->tp.nr_args; i++) > traceprobe_update_arg(&tk->tp.args[i]); > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-07-03 22:30 ` Steven Rostedt @ 2018-07-11 19:34 ` Francis Deslauriers 2018-07-11 19:56 ` Steven Rostedt 2018-07-12 13:46 ` Masami Hiramatsu 0 siblings, 2 replies; 35+ messages in thread From: Francis Deslauriers @ 2018-07-11 19:34 UTC (permalink / raw) To: Steven Rostedt Cc: Mathieu Desnoyers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, torvalds Hi Steven, I tested it and it prevents the kernel crash I am witnessing. As for the side-effect that Masami mentioned regarding not being able to probe function inside the trace_kprobe.c file, I suggest we move the target function in its own separate compile unit so it can be compiled with the ftrace cflags. See patch below. Thanks Francis From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001 From: Francis Deslauriers <francis.deslauriers@efficios.com> Date: Wed, 11 Jul 2018 12:34:22 -0400 Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate compile unit 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 <francis.deslauriers@efficios.com> --- 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 diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile index e2538c7..e38771e 100644 --- a/kernel/trace/Makefile +++ b/kernel/trace/Makefile @@ -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 diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 952dc2a..3fe966f 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -24,6 +24,7 @@ #include <linux/error-injection.h> #include "trace_probe.h" +#include "trace_kprobe_selftest.h" #define KPROBE_EVENT_SYSTEM "kprobes" #define KRETPROBE_MAXACTIVE_MAX 4096 @@ -1560,17 +1561,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) { diff --git a/kernel/trace/trace_kprobe_selftest.c b/kernel/trace/trace_kprobe_selftest.c new file mode 100644 index 0000000..a3d2090 --- /dev/null +++ b/kernel/trace/trace_kprobe_selftest.c @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Function used during the kprobe self test. This function is in a seperate + * 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; +} diff --git a/kernel/trace/trace_kprobe_selftest.h b/kernel/trace/trace_kprobe_selftest.h new file mode 100644 index 0000000..9243d4e --- /dev/null +++ b/kernel/trace/trace_kprobe_selftest.h @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Function used during the kprobe self test. This function is in a seperate + * 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); -- 2.7.4 Le mar. 3 juill. 2018, à 18 h 31, Steven Rostedt <rostedt@goodmis.org> a écrit : > > Mathieu and Francis, > > Looking back, this thread never got further. Would Masami's patch work > for you? > > -- Steve > > > On Sat, 17 Mar 2018 10:22:11 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > On Sat, 17 Mar 2018 09:13:34 +0900 > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT) > > > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > > > > ----- On Mar 16, 2018, at 12:48 PM, rostedt rostedt@goodmis.org wrote: > > > > > > > > > On Fri, 16 Mar 2018 12:41:34 -0400 > > > > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > > > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm > > > > >> saying that I don't have time to fix it now, but would be happy to > > > > >> accept patches if someone else does so. > > > > > > > > > > And looking at what I replied before for the original patch. It would > > > > > probably be a good idea to blacklist directories. Like we do with > > > > > function tracing. We probably should black list both kernel/tracing and > > > > > kernel/events from being probed. > > > > > > > > > > Did this come up at plumbers? You were there too, I don't remember > > > > > discussing it there. > > > > > > > > I don't remember this coming up last Plumbers nor KS neither, given > > > > that we were focused on other topics. > > > > > > > > Would the general approach you envision be based on emitting all code > > > > generated by compilation of all objects under kernel/tracing and > > > > kernel/events into a specific "nokprobes" text section of the kernel ? > > > > Perhaps we could create a specific linker scripts for those directories, > > > > or do you have in mind a neater way to do this ? > > > > > > .kprobes.text section still exists. As I pointed in previous mail, I don't > > > think we have to put all those code into that section. But if you want, > > > it is acceptable to have a kconfig which push most of those ftrace related > > > code into .kprobes.text section. > > > > Or, we can check it by ftrace_location_range() as below patch. > > > > Note that as a side-effect, we can not trace functions in trace_kprobe.c > > anymore, and this means it is hard to me to make a testcase of kprobe events. > > It was the easiest (and maintainable) way to make test cases... sigh. > > > > ----- > > tracing: kprobes: Prohibit probing on notrace function > > > > From: Masami Hiramatsu <mhiramat@kernel.org> > > > > 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. > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > --- > > kernel/trace/trace_kprobe.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 5ce9b8cf7be3..7404b012e51a 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file) > > return ret; > > } > > > > +#ifdef CONFIG_KPROBES_ON_FTRACE > > +static bool within_notrace_func(struct trace_kprobe *tk) > > +{ > > + unsigned long offset, size, addr; > > + > > + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk)); > > + addr += trace_kprobe_offset(tk); > > + > > + if (!kallsyms_lookup_size_offset(addr, &size, &offset)) > > + return true; /* Out of range. */ > > + > > + return !ftrace_location_range(addr - offset, addr - offset + size); > > +} > > +#else > > +#define within_notrace_func(tk) (false) > > +#endif > > + > > /* Internal register function - just handle k*probes and flags */ > > static int __register_trace_kprobe(struct trace_kprobe *tk) > > { > > @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk) > > if (trace_probe_is_registered(&tk->tp)) > > return -EINVAL; > > > > + if (within_notrace_func(tk)) { > > + pr_warn("Could not probe notrace function %s\n", > > + trace_kprobe_symbol(tk)); > > + return -EINVAL; > > + } > > + > > for (i = 0; i < tk->tp.nr_args; i++) > > traceprobe_update_arg(&tk->tp.args[i]); > > > > > -- Francis Deslauriers Software developer EfficiOS inc. ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-07-11 19:34 ` Francis Deslauriers @ 2018-07-11 19:56 ` Steven Rostedt 2018-07-12 0:40 ` Francis Deslauriers 2018-07-12 13:46 ` Masami Hiramatsu 1 sibling, 1 reply; 35+ messages in thread From: Steven Rostedt @ 2018-07-11 19:56 UTC (permalink / raw) To: Francis Deslauriers Cc: Mathieu Desnoyers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, torvalds On Wed, 11 Jul 2018 15:34:30 -0400 Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > Hi Steven, > I tested it and it prevents the kernel crash I am witnessing. > As for the side-effect that Masami mentioned regarding not being able to probe > function inside the trace_kprobe.c file, I suggest we move the target > function in > its own separate compile unit so it can be compiled with the ftrace cflags. > See patch below. > The patch below looks fine and so does Masami's. But there's too many patches within emails (not separated out). I have no idea what to apply. I'm not going to apply anything that is not sent as a proper patch (ie. any patch within a separate thread, like the patch below). -- Steve > Thanks > Francis > > >From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001 > From: Francis Deslauriers <francis.deslauriers@efficios.com> > Date: Wed, 11 Jul 2018 12:34:22 -0400 > Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate > compile unit > > 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 <francis.deslauriers@efficios.com> > --- > 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 > > diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile > index e2538c7..e38771e 100644 > --- a/kernel/trace/Makefile > +++ b/kernel/trace/Makefile > @@ -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 > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 952dc2a..3fe966f 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -24,6 +24,7 @@ > #include <linux/error-injection.h> > > #include "trace_probe.h" > +#include "trace_kprobe_selftest.h" > > #define KPROBE_EVENT_SYSTEM "kprobes" > #define KRETPROBE_MAXACTIVE_MAX 4096 > @@ -1560,17 +1561,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) > { > diff --git a/kernel/trace/trace_kprobe_selftest.c > b/kernel/trace/trace_kprobe_selftest.c > new file mode 100644 > index 0000000..a3d2090 > --- /dev/null > +++ b/kernel/trace/trace_kprobe_selftest.c > @@ -0,0 +1,10 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Function used during the kprobe self test. This function is in a seperate > + * 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; > +} > diff --git a/kernel/trace/trace_kprobe_selftest.h > b/kernel/trace/trace_kprobe_selftest.h > new file mode 100644 > index 0000000..9243d4e > --- /dev/null > +++ b/kernel/trace/trace_kprobe_selftest.h > @@ -0,0 +1,7 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Function used during the kprobe self test. This function is in a seperate > + * 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); ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-07-11 19:56 ` Steven Rostedt @ 2018-07-12 0:40 ` Francis Deslauriers 2018-07-12 13:59 ` Masami Hiramatsu 0 siblings, 1 reply; 35+ messages in thread From: Francis Deslauriers @ 2018-07-12 0:40 UTC (permalink / raw) To: Steven Rostedt Cc: Mathieu Desnoyers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, torvalds Le mer. 11 juill. 2018, à 15 h 56, Steven Rostedt <rostedt@goodmis.org> a écrit : > > On Wed, 11 Jul 2018 15:34:30 -0400 > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > > > Hi Steven, > > I tested it and it prevents the kernel crash I am witnessing. > > As for the side-effect that Masami mentioned regarding not being able to probe > > function inside the trace_kprobe.c file, I suggest we move the target > > function in > > its own separate compile unit so it can be compiled with the ftrace cflags. > > See patch below. > > > > The patch below looks fine and so does Masami's. But there's too many > patches within emails (not separated out). I have no idea what to > apply. I'm not going to apply anything that is not sent as a proper > patch (ie. any patch within a separate thread, like the patch below). > I will put together a proper patch set with both commits. Masami, you mentioned: "So anyway we still need to mark those functions NOKPROBE_SYMBOL." in a reply. What functions were you talking about? ftrace_ops_assist_func? Aren't those functions covered by your within_notrace_func check? Thank you, Francis > -- Steve > > > > Thanks > > Francis > > > > >From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001 > > From: Francis Deslauriers <francis.deslauriers@efficios.com> > > Date: Wed, 11 Jul 2018 12:34:22 -0400 > > Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate > > compile unit > > > > 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 <francis.deslauriers@efficios.com> > > --- > > 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 > > > > diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile > > index e2538c7..e38771e 100644 > > --- a/kernel/trace/Makefile > > +++ b/kernel/trace/Makefile > > @@ -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 > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 952dc2a..3fe966f 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -24,6 +24,7 @@ > > #include <linux/error-injection.h> > > > > #include "trace_probe.h" > > +#include "trace_kprobe_selftest.h" > > > > #define KPROBE_EVENT_SYSTEM "kprobes" > > #define KRETPROBE_MAXACTIVE_MAX 4096 > > @@ -1560,17 +1561,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) > > { > > diff --git a/kernel/trace/trace_kprobe_selftest.c > > b/kernel/trace/trace_kprobe_selftest.c > > new file mode 100644 > > index 0000000..a3d2090 > > --- /dev/null > > +++ b/kernel/trace/trace_kprobe_selftest.c > > @@ -0,0 +1,10 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Function used during the kprobe self test. This function is in a seperate > > + * 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; > > +} > > diff --git a/kernel/trace/trace_kprobe_selftest.h > > b/kernel/trace/trace_kprobe_selftest.h > > new file mode 100644 > > index 0000000..9243d4e > > --- /dev/null > > +++ b/kernel/trace/trace_kprobe_selftest.h > > @@ -0,0 +1,7 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Function used during the kprobe self test. This function is in a seperate > > + * 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); > -- Francis Deslauriers Software developer EfficiOS inc. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-07-12 0:40 ` Francis Deslauriers @ 2018-07-12 13:59 ` Masami Hiramatsu 0 siblings, 0 replies; 35+ messages in thread From: Masami Hiramatsu @ 2018-07-12 13:59 UTC (permalink / raw) To: Francis Deslauriers Cc: Steven Rostedt, Mathieu Desnoyers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, torvalds On Wed, 11 Jul 2018 20:40:45 -0400 Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > Le mer. 11 juill. 2018, à 15 h 56, Steven Rostedt > <rostedt@goodmis.org> a écrit : > > > > On Wed, 11 Jul 2018 15:34:30 -0400 > > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > > > > > Hi Steven, > > > I tested it and it prevents the kernel crash I am witnessing. > > > As for the side-effect that Masami mentioned regarding not being able to probe > > > function inside the trace_kprobe.c file, I suggest we move the target > > > function in > > > its own separate compile unit so it can be compiled with the ftrace cflags. > > > See patch below. > > > > > > > The patch below looks fine and so does Masami's. But there's too many > > patches within emails (not separated out). I have no idea what to > > apply. I'm not going to apply anything that is not sent as a proper > > patch (ie. any patch within a separate thread, like the patch below). > > > I will put together a proper patch set with both commits. > > Masami, you mentioned: "So anyway we still need to mark those functions > NOKPROBE_SYMBOL." in a reply. What functions were you talking about? > ftrace_ops_assist_func? Aren't those functions covered by your > within_notrace_func check? Ah, I thought that we'd better to consider a "pure" kprobe without ftrace on notrace functions. From ftrace, we can prohibit probing on notrace funcs, but if user makes an out-of-tree kprobe module and put it on notrace funcs, that can cause a problem (if they enable some kprobe events at same time). Thank you, -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-07-11 19:34 ` Francis Deslauriers 2018-07-11 19:56 ` Steven Rostedt @ 2018-07-12 13:46 ` Masami Hiramatsu 1 sibling, 0 replies; 35+ messages in thread From: Masami Hiramatsu @ 2018-07-12 13:46 UTC (permalink / raw) To: Francis Deslauriers Cc: Steven Rostedt, Mathieu Desnoyers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, torvalds On Wed, 11 Jul 2018 15:34:30 -0400 Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > Hi Steven, > I tested it and it prevents the kernel crash I am witnessing. > As for the side-effect that Masami mentioned regarding not being able to probe > function inside the trace_kprobe.c file, I suggest we move the target > function in > its own separate compile unit so it can be compiled with the ftrace cflags. > See patch below. Ah, good catch. > > Thanks > Francis > > From d5a3645bd0046f28275d6b60207958f2751c1f47 Mon Sep 17 00:00:00 2001 > From: Francis Deslauriers <francis.deslauriers@efficios.com> > Date: Wed, 11 Jul 2018 12:34:22 -0400 > Subject: [PATCH] selftest/ftrace: Move kprobe selftest function to separate > compile unit > > 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. Looks good to me. Acked-by: Masami Hiramatsu <mhiramat@kernel.org> Thanks! > > Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com> > --- > 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 > > diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile > index e2538c7..e38771e 100644 > --- a/kernel/trace/Makefile > +++ b/kernel/trace/Makefile > @@ -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 > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 952dc2a..3fe966f 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -24,6 +24,7 @@ > #include <linux/error-injection.h> > > #include "trace_probe.h" > +#include "trace_kprobe_selftest.h" > > #define KPROBE_EVENT_SYSTEM "kprobes" > #define KRETPROBE_MAXACTIVE_MAX 4096 > @@ -1560,17 +1561,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) > { > diff --git a/kernel/trace/trace_kprobe_selftest.c > b/kernel/trace/trace_kprobe_selftest.c > new file mode 100644 > index 0000000..a3d2090 > --- /dev/null > +++ b/kernel/trace/trace_kprobe_selftest.c > @@ -0,0 +1,10 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Function used during the kprobe self test. This function is in a seperate > + * 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; > +} > diff --git a/kernel/trace/trace_kprobe_selftest.h > b/kernel/trace/trace_kprobe_selftest.h > new file mode 100644 > index 0000000..9243d4e > --- /dev/null > +++ b/kernel/trace/trace_kprobe_selftest.h > @@ -0,0 +1,7 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Function used during the kprobe self test. This function is in a seperate > + * 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); > -- > 2.7.4 > > > Le mar. 3 juill. 2018, à 18 h 31, Steven Rostedt <rostedt@goodmis.org> a écrit : > > > > Mathieu and Francis, > > > > Looking back, this thread never got further. Would Masami's patch work > > for you? > > > > -- Steve > > > > > > On Sat, 17 Mar 2018 10:22:11 +0900 > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > On Sat, 17 Mar 2018 09:13:34 +0900 > > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > On Fri, 16 Mar 2018 13:53:01 -0400 (EDT) > > > > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > > > > > > > ----- On Mar 16, 2018, at 12:48 PM, rostedt rostedt@goodmis.org wrote: > > > > > > > > > > > On Fri, 16 Mar 2018 12:41:34 -0400 > > > > > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > > > > > > >> Yes, kprobes are dangerous. I'm not saying it shouldn't be fixed, I'm > > > > > >> saying that I don't have time to fix it now, but would be happy to > > > > > >> accept patches if someone else does so. > > > > > > > > > > > > And looking at what I replied before for the original patch. It would > > > > > > probably be a good idea to blacklist directories. Like we do with > > > > > > function tracing. We probably should black list both kernel/tracing and > > > > > > kernel/events from being probed. > > > > > > > > > > > > Did this come up at plumbers? You were there too, I don't remember > > > > > > discussing it there. > > > > > > > > > > I don't remember this coming up last Plumbers nor KS neither, given > > > > > that we were focused on other topics. > > > > > > > > > > Would the general approach you envision be based on emitting all code > > > > > generated by compilation of all objects under kernel/tracing and > > > > > kernel/events into a specific "nokprobes" text section of the kernel ? > > > > > Perhaps we could create a specific linker scripts for those directories, > > > > > or do you have in mind a neater way to do this ? > > > > > > > > .kprobes.text section still exists. As I pointed in previous mail, I don't > > > > think we have to put all those code into that section. But if you want, > > > > it is acceptable to have a kconfig which push most of those ftrace related > > > > code into .kprobes.text section. > > > > > > Or, we can check it by ftrace_location_range() as below patch. > > > > > > Note that as a side-effect, we can not trace functions in trace_kprobe.c > > > anymore, and this means it is hard to me to make a testcase of kprobe events. > > > It was the easiest (and maintainable) way to make test cases... sigh. > > > > > > ----- > > > tracing: kprobes: Prohibit probing on notrace function > > > > > > From: Masami Hiramatsu <mhiramat@kernel.org> > > > > > > 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. > > > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > > --- > > > kernel/trace/trace_kprobe.c | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > > index 5ce9b8cf7be3..7404b012e51a 100644 > > > --- a/kernel/trace/trace_kprobe.c > > > +++ b/kernel/trace/trace_kprobe.c > > > @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file) > > > return ret; > > > } > > > > > > +#ifdef CONFIG_KPROBES_ON_FTRACE > > > +static bool within_notrace_func(struct trace_kprobe *tk) > > > +{ > > > + unsigned long offset, size, addr; > > > + > > > + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk)); > > > + addr += trace_kprobe_offset(tk); > > > + > > > + if (!kallsyms_lookup_size_offset(addr, &size, &offset)) > > > + return true; /* Out of range. */ > > > + > > > + return !ftrace_location_range(addr - offset, addr - offset + size); > > > +} > > > +#else > > > +#define within_notrace_func(tk) (false) > > > +#endif > > > + > > > /* Internal register function - just handle k*probes and flags */ > > > static int __register_trace_kprobe(struct trace_kprobe *tk) > > > { > > > @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk) > > > if (trace_probe_is_registered(&tk->tp)) > > > return -EINVAL; > > > > > > + if (within_notrace_func(tk)) { > > > + pr_warn("Could not probe notrace function %s\n", > > > + trace_kprobe_symbol(tk)); > > > + return -EINVAL; > > > + } > > > + > > > for (i = 0; i < tk->tp.nr_args; i++) > > > traceprobe_update_arg(&tk->tp.args[i]); > > > > > > > > > > > -- > Francis Deslauriers > Software developer > EfficiOS inc. -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist 2018-03-16 16:28 ` Mathieu Desnoyers 2018-03-16 16:41 ` Steven Rostedt @ 2018-03-17 0:08 ` Masami Hiramatsu 1 sibling, 0 replies; 35+ messages in thread From: Masami Hiramatsu @ 2018-03-17 0:08 UTC (permalink / raw) To: Mathieu Desnoyers Cc: rostedt, Francis Deslauriers, Masami Hiramatsu, Peter Zijlstra, linux-kernel, Linus Torvalds On Fri, 16 Mar 2018 12:28:59 -0400 (EDT) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > ----- On Mar 16, 2018, at 11:25 AM, rostedt rostedt@goodmis.org wrote: > > > On Fri, 16 Mar 2018 11:18:25 -0400 > > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > > > >> Hi Steven, > >> > >> I completely forgot about this issue until recently when I encountered it again. > >> Instrumenting the ftrace_ops_assist_func symbol and some other symbol > >> seems to be causing problems. > >> > >> Placing kretprobes like in the following configuration crashes my > >> kernel (4.16.0-rc5) on a Qemu/KVM virtual machine: > >> > >> config 1: > >> echo "r:event_1 __fdget" >> kprobe_events > >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events > >> > >> config 2: > >> echo "r:event_1 __fdget_pos" >> kprobe_events > >> echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events > >> > >> config 3: > >> echo 'r:event_1 arch_dup_task_struct' >> kprobe_events > >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events > >> > >> config 4: > >> echo 'r:event_1 sys_open' >> kprobe_events > >> echo 'r:event_2 ftrace_ops_assist_func' >> kprobe_events > >> > >> Here is my kernel config [1]: > >> > >> In a previous email [2], you mentioned that you would like to add the > >> ftrace-related symbols to a section to un-blacklist them all at once > >> on demand but wanted to discuss it at Linux Plumbers. Do you still > >> think that it's the right approach? > > > > We probably didn't discuss it (as there was a lot to discuss, and this > > was probably overshadowed by that). But yes, you should not probe > > ftrace called functions. That is guaranteed to crash and that crash is > > not a bug, but a feature. > > Are you really arguing that crashing the kernel from an ABI visible from > userspace (even if it's only root user) is not a bug ? You are joking right ? > Is there an EXPERIMENTAL config option that people need to select in order to > make sure those ftrace interfaces don't end up on production systems ? > > > > > The ftrace and ring buffer files should be blacklisted from being > > probed. Perhaps the entire directory. > > All code reachable from a kprobe handler should be blacklisted from > kprobes, yes. No, actually that is incorrect, since user can make a module to probe those functions from outside of ftrace via kprobes. The problematic functions are the functions called between probe-hit and set current_kprobe (iow, kprobe_ftrace_handler()). After setting current_kprobe, all probe hits are just skipped. The issue that Francis faced is actual bug. It must be blacklisted, since it is called before calling kprobe_ftrace_handler(). target_func -> fentry-code -> ftrace-code -> kprobe_ftrace_handler (safe area) <- ret <- ret (ftrace-code) <- ret (fentry-code) In above model, functions in fentry-code and ftrace-code must be blacklisted. Thanks, > > Anyway, I don't see this as much of an urgent matter, as it's one of > > those "Patient: Doc, it hurts when I do this. Doc: Don't do that" > > cases. And there's a lot of urgent issues that currently need to be > > dealt with. > > OK, short-term we'll remove everything related to ftrace functions > from our CI fuzzer coverage. Arguably, the fact that a root user can > crash the kernel through tracefs files is not that great security-wise > though. > > Considering that our current focus is to test the kprobe instrumentation > layer (and not ftrace per se), we will move our fuzzer to the LTTng ABI > instead, which should take care of removing crashes introduced by ftrace > from our fuzzing results. > > Thanks, > > Mathieu > > > > > > -- Steve > > > > > >> > >> I can easily test any patch regarding this issue. > >> > >> [1] http://paste.ubuntu.com/p/BJWvgMnW8z/ > >> [2] https://lkml.org/lkml/2017/7/14/568 > >> > >> Thank you, > >> > >> 2017-07-14 14:29 GMT-04:00 Steven Rostedt <rostedt@goodmis.org>: > >> > On Fri, 14 Jul 2017 10:58:35 -0400 > >> > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > >> > > >> >> This function is called when a kprobe is hit. Thus it should be > >> >> blacklisted to prevent kprobe to be triggered by kprobes. > >> >> > >> >> Signed-off-by: Francis Deslauriers <francis.deslauriers@efficios.com> > >> >> --- > >> >> kernel/trace/ftrace.c | 2 ++ > >> >> 1 file changed, 2 insertions(+) > >> >> > >> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > >> >> index b308be3..c473d9b 100644 > >> >> --- a/kernel/trace/ftrace.c > >> >> +++ b/kernel/trace/ftrace.c > >> >> @@ -36,6 +36,7 @@ > >> >> > >> >> #include <trace/events/sched.h> > >> >> > >> >> +#include <asm/kprobes.h> > >> >> #include <asm/sections.h> > >> >> #include <asm/setup.h> > >> >> > >> >> @@ -5739,6 +5740,7 @@ static void ftrace_ops_assist_func(unsigned long ip, > >> >> unsigned long parent_ip, > >> >> preempt_enable_notrace(); > >> >> trace_clear_recursion(bit); > >> >> } > >> >> +NOKPROBE_SYMBOL(ftrace_ops_assist_func); > >> > > >> > Continuing from what I said in the other email, this is fixing a > >> > symptom and not the problem. The real fix will be much more involved. I > >> > have a good idea on how to accomplish it too. > >> > > >> > -- Steve > >> > > >> > > >> >> > >> >> /** > >> >> * ftrace_ops_get_func - get the function a trampoline should call > >> > > >> > >> > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 0/2] tracing: kprobes: Prohibit probing on notrace functions 2017-07-14 14:58 ` [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist Francis Deslauriers 2017-07-14 18:29 ` Steven Rostedt @ 2018-07-12 17:54 ` Francis Deslauriers 2018-07-12 17:54 ` [PATCH 1/2] " Francis Deslauriers 2018-07-12 17:54 ` [PATCH 2/2] selftest/ftrace: Move kprobe selftest function to separate compile unit Francis Deslauriers 1 sibling, 2 replies; 35+ messages in thread From: Francis Deslauriers @ 2018-07-12 17:54 UTC (permalink / raw) To: rostedt, mhiramat, peterz Cc: mathieu.desnoyers, linux-kernel, Francis Deslauriers Kprobe code has a mecanism protecting against triggering kprobe during the handling of an event. If a kprobe is placed between the kprobe handling entry point and the activation of this protection, the user can cause an infinite kprobe recursion leading to a kernel crash. To avoid this, prevent kprobes from being placed in notrace functions. Also, move the kprobe selftest target function in its own compile unit so it's not marked as notrace and can thus be used in the ftrace startup tests. Francis Deslauriers (1): selftest/ftrace: Move kprobe selftest function to separate compile unit Masami Hiramatsu (1): tracing: kprobes: Prohibit probing on notrace functions kernel/trace/Makefile | 5 +++++ kernel/trace/trace_kprobe.c | 35 ++++++++++++++++++++++++----------- kernel/trace/trace_kprobe_selftest.c | 10 ++++++++++ kernel/trace/trace_kprobe_selftest.h | 7 +++++++ 4 files changed, 46 insertions(+), 11 deletions(-) create mode 100644 kernel/trace/trace_kprobe_selftest.c create mode 100644 kernel/trace/trace_kprobe_selftest.h -- 2.7.4 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/2] tracing: kprobes: Prohibit probing on notrace functions 2018-07-12 17:54 ` [PATCH 0/2] tracing: kprobes: Prohibit probing on notrace functions Francis Deslauriers @ 2018-07-12 17:54 ` Francis Deslauriers 2018-07-12 21:49 ` Steven Rostedt 2018-07-13 2:53 ` Masami Hiramatsu 2018-07-12 17:54 ` [PATCH 2/2] selftest/ftrace: Move kprobe selftest function to separate compile unit Francis Deslauriers 1 sibling, 2 replies; 35+ messages in thread From: Francis Deslauriers @ 2018-07-12 17:54 UTC (permalink / raw) To: rostedt, mhiramat, peterz; +Cc: mathieu.desnoyers, linux-kernel From: Masami Hiramatsu <mhiramat@kernel.org> 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 infinite recursive call. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Tested-by: Francis Deslauriers <francis.deslauriers@efficios.com> --- kernel/trace/trace_kprobe.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index daa8157..952dc2a 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file) return ret; } +#ifdef CONFIG_KPROBES_ON_FTRACE +static bool within_notrace_func(struct trace_kprobe *tk) +{ + unsigned long offset, size, addr; + + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk)); + addr += trace_kprobe_offset(tk); + + if (!kallsyms_lookup_size_offset(addr, &size, &offset)) + return true; /* Out of range. */ + + return !ftrace_location_range(addr - offset, addr - offset + size); +} +#else +#define within_notrace_func(tk) (false) +#endif + /* Internal register function - just handle k*probes and flags */ static int __register_trace_kprobe(struct trace_kprobe *tk) { @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk) if (trace_probe_is_registered(&tk->tp)) return -EINVAL; + if (within_notrace_func(tk)) { + pr_warn("Could not probe notrace function %s\n", + trace_kprobe_symbol(tk)); + return -EINVAL; + } + for (i = 0; i < tk->tp.nr_args; i++) traceprobe_update_arg(&tk->tp.args[i]); -- 2.7.4 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] tracing: kprobes: Prohibit probing on notrace functions 2018-07-12 17:54 ` [PATCH 1/2] " Francis Deslauriers @ 2018-07-12 21:49 ` Steven Rostedt 2018-07-13 2:53 ` Masami Hiramatsu 1 sibling, 0 replies; 35+ messages in thread From: Steven Rostedt @ 2018-07-12 21:49 UTC (permalink / raw) To: Francis Deslauriers; +Cc: mhiramat, peterz, mathieu.desnoyers, linux-kernel On Thu, 12 Jul 2018 13:54:12 -0400 Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > From: Masami Hiramatsu <mhiramat@kernel.org> > > 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 infinite recursive call. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Tested-by: Francis Deslauriers <francis.deslauriers@efficios.com> BTW, since you are sending the patch, you must also put in your Signed-off-by too, after Masami's. You can keep the Tested-by. -- Steve > --- > kernel/trace/trace_kprobe.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index daa8157..952dc2a 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -487,6 +487,23 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file) > return ret; > } > > +#ifdef CONFIG_KPROBES_ON_FTRACE > +static bool within_notrace_func(struct trace_kprobe *tk) > +{ > + unsigned long offset, size, addr; > + > + addr = kallsyms_lookup_name(trace_kprobe_symbol(tk)); > + addr += trace_kprobe_offset(tk); > + > + if (!kallsyms_lookup_size_offset(addr, &size, &offset)) > + return true; /* Out of range. */ > + > + return !ftrace_location_range(addr - offset, addr - offset + size); > +} > +#else > +#define within_notrace_func(tk) (false) > +#endif > + > /* Internal register function - just handle k*probes and flags */ > static int __register_trace_kprobe(struct trace_kprobe *tk) > { > @@ -495,6 +512,12 @@ static int __register_trace_kprobe(struct trace_kprobe *tk) > if (trace_probe_is_registered(&tk->tp)) > return -EINVAL; > > + if (within_notrace_func(tk)) { > + pr_warn("Could not probe notrace function %s\n", > + trace_kprobe_symbol(tk)); > + return -EINVAL; > + } > + > for (i = 0; i < tk->tp.nr_args; i++) > traceprobe_update_arg(&tk->tp.args[i]); > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] tracing: kprobes: Prohibit probing on notrace functions 2018-07-12 17:54 ` [PATCH 1/2] " Francis Deslauriers 2018-07-12 21:49 ` Steven Rostedt @ 2018-07-13 2:53 ` Masami Hiramatsu 2018-07-13 12:18 ` Steven Rostedt 1 sibling, 1 reply; 35+ messages in thread From: Masami Hiramatsu @ 2018-07-13 2:53 UTC (permalink / raw) To: Francis Deslauriers; +Cc: rostedt, peterz, mathieu.desnoyers, linux-kernel On Thu, 12 Jul 2018 13:54:12 -0400 Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > From: Masami Hiramatsu <mhiramat@kernel.org> > > 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 infinite recursive call. BTW, I'm considering to add an option to allow putting kprobes on notrace function - just for debugging ftrace by kprobes. That is "developer only" option so generally it should be disabled, but for debugging the ftrace, we still need it. Or should I introduce another kprobes module for debugging it? Thank you, -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] tracing: kprobes: Prohibit probing on notrace functions 2018-07-13 2:53 ` Masami Hiramatsu @ 2018-07-13 12:18 ` Steven Rostedt 2018-07-26 0:41 ` Masami Hiramatsu 0 siblings, 1 reply; 35+ messages in thread From: Steven Rostedt @ 2018-07-13 12:18 UTC (permalink / raw) To: Masami Hiramatsu Cc: Francis Deslauriers, peterz, mathieu.desnoyers, linux-kernel On Fri, 13 Jul 2018 11:53:01 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Thu, 12 Jul 2018 13:54:12 -0400 > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > > > From: Masami Hiramatsu <mhiramat@kernel.org> > > > > 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 infinite recursive call. > > BTW, I'm considering to add an option to allow putting > kprobes on notrace function - just for debugging > ftrace by kprobes. That is "developer only" option > so generally it should be disabled, but for debugging > the ftrace, we still need it. Or should I introduce > another kprobes module for debugging it? No, I think the former is better (to add an option to allow putting kprobes on notrace functions). By default we let people protect themselves. But if then provide a switch that lets you do things that might let you shoot yourself in the foot. BTW, I'm now leaving on vacation. I'll be back on the 23rd and will be looking for patches that I should be pulling in then. Thanks! -- Steve ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] tracing: kprobes: Prohibit probing on notrace functions 2018-07-13 12:18 ` Steven Rostedt @ 2018-07-26 0:41 ` Masami Hiramatsu 2018-07-26 1:13 ` Steven Rostedt 0 siblings, 1 reply; 35+ messages in thread From: Masami Hiramatsu @ 2018-07-26 0:41 UTC (permalink / raw) To: Steven Rostedt Cc: Francis Deslauriers, peterz, mathieu.desnoyers, linux-kernel On Fri, 13 Jul 2018 08:18:03 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 13 Jul 2018 11:53:01 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > On Thu, 12 Jul 2018 13:54:12 -0400 > > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > > > > > From: Masami Hiramatsu <mhiramat@kernel.org> > > > > > > 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 infinite recursive call. > > > > BTW, I'm considering to add an option to allow putting > > kprobes on notrace function - just for debugging > > ftrace by kprobes. That is "developer only" option > > so generally it should be disabled, but for debugging > > the ftrace, we still need it. Or should I introduce > > another kprobes module for debugging it? > > No, I think the former is better (to add an option to allow putting > kprobes on notrace functions). By default we let people protect > themselves. But if then provide a switch that lets you do things that > might let you shoot yourself in the foot. I'm adding CONFIG_KPROBE_EVENTS_ON_NOTRACE kconfig which allows kprobes on notrace function. I think we don't need to make it online switchable, since it is only good for ftrace developers. Thank you, > > BTW, I'm now leaving on vacation. I'll be back on the 23rd and will be > looking for patches that I should be pulling in then. > > Thanks! > > -- Steve -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] tracing: kprobes: Prohibit probing on notrace functions 2018-07-26 0:41 ` Masami Hiramatsu @ 2018-07-26 1:13 ` Steven Rostedt 0 siblings, 0 replies; 35+ messages in thread From: Steven Rostedt @ 2018-07-26 1:13 UTC (permalink / raw) To: Masami Hiramatsu Cc: Francis Deslauriers, peterz, mathieu.desnoyers, linux-kernel On Thu, 26 Jul 2018 09:41:06 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Fri, 13 Jul 2018 08:18:03 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > On Fri, 13 Jul 2018 11:53:01 +0900 > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > On Thu, 12 Jul 2018 13:54:12 -0400 > > > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > > > > > > > From: Masami Hiramatsu <mhiramat@kernel.org> > > > > > > > > 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 infinite recursive call. > > > > > > BTW, I'm considering to add an option to allow putting > > > kprobes on notrace function - just for debugging > > > ftrace by kprobes. That is "developer only" option > > > so generally it should be disabled, but for debugging > > > the ftrace, we still need it. Or should I introduce > > > another kprobes module for debugging it? > > > > No, I think the former is better (to add an option to allow putting > > kprobes on notrace functions). By default we let people protect > > themselves. But if then provide a switch that lets you do things that > > might let you shoot yourself in the foot. > > I'm adding CONFIG_KPROBE_EVENTS_ON_NOTRACE kconfig which allows > kprobes on notrace function. I think we don't need to make it > online switchable, since it is only good for ftrace developers. > Works for me. Thanks! -- Steve ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 2/2] selftest/ftrace: Move kprobe selftest function to separate compile unit 2018-07-12 17:54 ` [PATCH 0/2] tracing: kprobes: Prohibit probing on notrace functions Francis Deslauriers 2018-07-12 17:54 ` [PATCH 1/2] " Francis Deslauriers @ 2018-07-12 17:54 ` Francis Deslauriers 1 sibling, 0 replies; 35+ messages in thread From: Francis Deslauriers @ 2018-07-12 17:54 UTC (permalink / raw) To: rostedt, mhiramat, peterz Cc: mathieu.desnoyers, linux-kernel, Francis Deslauriers 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 <francis.deslauriers@efficios.com> Acked-by: Masami Hiramatsu <mhiramat@kernel.org> --- 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 diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile index e2538c7..e38771e 100644 --- a/kernel/trace/Makefile +++ b/kernel/trace/Makefile @@ -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 diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 952dc2a..d12be53 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -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" @@ -1560,17 +1561,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) { diff --git a/kernel/trace/trace_kprobe_selftest.c b/kernel/trace/trace_kprobe_selftest.c new file mode 100644 index 0000000..16548ee --- /dev/null +++ b/kernel/trace/trace_kprobe_selftest.c @@ -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; +} diff --git a/kernel/trace/trace_kprobe_selftest.h b/kernel/trace/trace_kprobe_selftest.h new file mode 100644 index 0000000..4e10ec4 --- /dev/null +++ b/kernel/trace/trace_kprobe_selftest.h @@ -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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist 2017-07-14 14:58 [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist Francis Deslauriers 2017-07-14 14:58 ` [PATCH 1/2] kprobe: fix: Add _ASM_NOKPROBE to x86 apic interrupt macro Francis Deslauriers 2017-07-14 14:58 ` [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist Francis Deslauriers @ 2017-07-14 18:27 ` Steven Rostedt 2017-07-16 15:59 ` Masami Hiramatsu 2017-07-16 14:37 ` Masami Hiramatsu 3 siblings, 1 reply; 35+ messages in thread From: Steven Rostedt @ 2017-07-14 18:27 UTC (permalink / raw) To: Francis Deslauriers; +Cc: mhiramat, peterz, mathieu.desnoyers, linux-kernel On Fri, 14 Jul 2017 10:58:33 -0400 Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > Kretprobe on ftrace_ops_assist_func and another function: > Those crashes are triggered when hooking a kretprobe on the > ftrace_ops_assist_func symbol and some other functions to make the this first > function reacheable. From my understanding, ftrace_ops_assist_func is the > function called directly when the kprobe is hit. Thus it should be marked > with NOKPROBE_SYMBOL. > Hmm, I'm wondering if I should just make an ftrace section, and black list the entire thing. Also that section could be used to not allow ftrace to use it either. I've been wanting to start letting ftrace trace the tracing code, and perf for that matter. It would be nice to be able to debug things like that. I would like to also make sections that can be enabled or disabled in groups. To group things like the tracing facility and perf and have them by default not be traced, but then set a flag that says "sure go ahead and trace them". This shouldn't be too hard to do. Hmm, I'll add this as another topic to have for the Linux Plumbers tracing track, as well as the kernel tracing topic. -- Steve ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist 2017-07-14 18:27 ` [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist Steven Rostedt @ 2017-07-16 15:59 ` Masami Hiramatsu 0 siblings, 0 replies; 35+ messages in thread From: Masami Hiramatsu @ 2017-07-16 15:59 UTC (permalink / raw) To: Steven Rostedt Cc: Francis Deslauriers, mhiramat, peterz, mathieu.desnoyers, linux-kernel On Fri, 14 Jul 2017 14:27:56 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 14 Jul 2017 10:58:33 -0400 > Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > > > > Kretprobe on ftrace_ops_assist_func and another function: > > Those crashes are triggered when hooking a kretprobe on the > > ftrace_ops_assist_func symbol and some other functions to make the this first > > function reacheable. From my understanding, ftrace_ops_assist_func is the > > function called directly when the kprobe is hit. Thus it should be marked > > with NOKPROBE_SYMBOL. > > > > Hmm, I'm wondering if I should just make an ftrace section, and black > list the entire thing. Also that section could be used to not allow > ftrace to use it either. I've been wanting to start letting ftrace > trace the tracing code, and perf for that matter. It would be nice to > be able to debug things like that. Tracer usually has 2 parts, one is off-line setting part (kicked by user) and another is core online tracing part (which kicked from anywhere). Former can be traced but latter is not. Yeah, I did same thing when I introduced NOKPROBE_SYMBOL() macro. And I also think that is good for kgdb. > I would like to also make sections that can be enabled or disabled in > groups. To group things like the tracing facility and perf and have > them by default not be traced, but then set a flag that says "sure go > ahead and trace them". This shouldn't be too hard to do. We have to notice that is a trigger which allows to shoot yourself in the foot :) Thanks, > > Hmm, I'll add this as another topic to have for the Linux Plumbers > tracing track, as well as the kernel tracing topic. > > -- Steve -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist 2017-07-14 14:58 [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist Francis Deslauriers ` (2 preceding siblings ...) 2017-07-14 18:27 ` [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist Steven Rostedt @ 2017-07-16 14:37 ` Masami Hiramatsu 2017-07-16 15:46 ` Masami Hiramatsu 3 siblings, 1 reply; 35+ messages in thread From: Masami Hiramatsu @ 2017-07-16 14:37 UTC (permalink / raw) To: Francis Deslauriers; +Cc: rostedt, peterz, mathieu.desnoyers, linux-kernel Hi Francis, At first, thank you for reporting the report! On Fri, 14 Jul 2017 10:58:33 -0400 Francis Deslauriers <francis.deslauriers@efficios.com> wrote: > Hi all, > > While fuzzing the Perf kprobe and kretprobe interfaces, I found some inputs > that trigger crashes of a 4.12 kernel(6f7da290413ba713f0cdd9ff1a2a9bb129ef4f6c) > on a x86-64 VM. I know that K(ret)probes can crash the kernel in multiple ways > but should Perf be allowed to do it? No, it must not be, it should be fixed. > To do this analysis, I used the symbols reported by /proc/kallsyms in > conjonction with the Perf debugfs interface. Using this technique, I was able > to find two instrumentation configurations that could crash the kernel. I am > suggesting changes that fixed both issues for me by blacklisting the symbols in > question. > > Kprobe on apic_timer_interrupt: > I believe that this is caused by the fact that kprobe adds a INT3 in a apic > interrupt routine. > How to reproduce: > echo 'p:event1 apic_timer_interrupt ' > kprobe_events > <Generate kernel activity. e.g. launch bash> > Crash log:[1] OK, I got this was reproduced even on qemu. (and you also seems to use qemu) > This can be fixed by blacklisting the apicinterrupt3 symbols directly in the > assembly macro. See patch[1/2]. I am not sure that blacklisting all > apicinterrupt symbols is the right solution. Wait, would you find the root cause of this crash? Your log [1] doesn't show why this happens. It just shows there was an infinit call/fault loop and kernel stack overflow. We should carefully analyze how it happened before just blacklisting it. (BTW, ahead to keep notice, but this problem can be solved by disabling optprobe via /proc/sys/debug/kprobes-optimization.) [ 161.618249] ? trace_hardirqs_off_caller+0x7/0xa0 [ 161.618965] ? trace_hardirqs_off_thunk+0x1a/0x1c [ 161.619677] ? native_iret+0x7/0x7 [ 161.620282] ? error_entry+0x72/0xd0 [ 161.620901] ? error_entry+0x72/0xd0 [ 161.621517] ? async_page_fault+0x12/0x30 [ 161.622207] ? native_iret+0x7/0x7 [ 161.622819] ? error_entry+0x72/0xd0 [ 161.623435] ? trace_hardirqs_off_caller+0x7/0xa0 [ 161.624176] ? trace_hardirqs_off_thunk+0x1a/0x1c [ 161.624893] ? native_iret+0x7/0x7 [ 161.625496] ? error_entry+0x72/0xd0 [ 161.626115] ? async_page_fault+0x12/0x30 [ 161.626771] ? optimized_callback+0x17/0xf0 [ 161.627443] ? 0xffffffffa000202f $ eu-addr2line -e vmlinux optimized_callback+0x17 /home/mhiramat/ksrc/linux/include/linux/kprobes.h:384 ---- 382 static inline struct kprobe_ctlblk *get_kprobe_ctlblk(void) 383 { 384 return this_cpu_ptr(&kprobe_ctlblk); 385 } And the disasm is; ffffffff8103d716: 53 push %rbx ffffffff8103d717: 65 4c 03 25 11 ca fc add %gs:0x7efcca11(%rip),%r12 # a130 <this_cpu_off> ffffffff8103d71e: 7e ---- OK, it seems this_cpu_ptr() caused pagefault. And it seems very odd stack, since we have a few unique addresses on there. (1) async_page_fault+0x12 ---- ffffffff815f6480 <async_page_fault>: [...] ffffffff815f6489: 48 83 c4 88 add $0xffffffffffffff88,%rsp ffffffff815f648d: e8 2e 01 00 00 callq ffffffff815f65c0 <error_entry> ffffffff815f6492: 48 89 e7 mov %rsp,%rdi ---- (2) error_entry+0x72 ---- ffffffff815f65c0 <error_entry>: [...] ffffffff815f662d: e8 88 ab a0 ff callq ffffffff810011ba <trace_hardirqs_off_thunk> ffffffff815f6632: c3 retq ---- (3) trace_hardirqs_off_thunk+0x1a ---- ffffffff810011ba <trace_hardirqs_off_thunk>: [...] ffffffff810011cf: e8 3c 2f 0a 00 callq ffffffff810a4110 <trace_hardirqs_off_caller> ffffffff810011d4: eb 18 jmp ffffffff810011ee <lockdep_sys_exit_thunk+0x18> ---- (4) trace_hardirqs_off_caller+0x7 ---- ffffffff810a4110: 44 8b 05 b9 11 a1 00 mov 0xa111b9(%rip),%r8d # ffffffff81ab52d0 <debug_locks> ffffffff810a4117: 65 48 8b 14 25 00 c4 mov %gs:0xc400,%rdx ffffffff810a411e: 00 00 ---- (5) native_iret+0x7 -> this seems native_irq_return_iret ---- ffffffff815f5d00 <native_iret>: ffffffff815f5d00: f6 44 24 20 04 testb $0x4,0x20(%rsp) ffffffff815f5d05: 75 02 jne ffffffff815f5d09 <native_irq_return_ldt> ffffffff815f5d07 <native_irq_return_iret>: ffffffff815f5d07: 48 cf iretq ---- So, the story what the stack said, - optimized_callback() calls get_kprobe_ctlblk(), and this_cpu_ptr(&kprobe_ctlblk) caused a page fault (in apic_timer_interrupt, does it cause any problem?) - and following call-chain occured async_page_fault -> error_entry -> trace_hardirqs_off_thunk -> trace_hardirqs_off_caller - "mov %gs:0xc400,%rdx" caused async_page_fault() again. Since trace_hardirqs_off_thunk() stores general registers on the stack, there are some noises. [ 114.429637] FS: 00000000021e7880(0000) GS:ffff88001fd40000(0000) knlGS:0000000000000000 So, the problem seems that cpu can not access to per-cpu pages. > Kretprobe on ftrace_ops_assist_func and another function: For this problem, Steve gave us an good idea so I agree with him. Thank you, > Those crashes are triggered when hooking a kretprobe on the > ftrace_ops_assist_func symbol and some other functions to make the this first > function reacheable. From my understanding, ftrace_ops_assist_func is the > function called directly when the kprobe is hit. Thus it should be marked > with NOKPROBE_SYMBOL. > > Here are some configurations that can easily reproduce this bug. Those other > functions are called during the fork of a process so they are easy to control. > Enable the following kprobes and launch a process to trigger a fork to see the > kernel crash. > > Conf #1 > echo 'r:event1 ftrace_ops_assist_func' > kprobe_events > echo 'r:event2 clear_all_latency_tracing' > kprobe_events > Crash log:[2] > > Conf #2 > echo 'r:event1 ftrace_ops_assist_func' > kprobe_events > echo 'r:event2 acct_clear_integrals' > kprobe_events > Crash log:[3] > > Conf #3 > echo 'r:event1 ftrace_ops_assist_func' > kprobe_events > echo 'r:event2 arch_dup_task_struct' > kprobe_events > Crash log:[4] > > The ftrace_ops_assist_func should be included in the kprobe blacklist using > NOKPROBE_SYMBOL. See patch [2/2]. > > Since those were found using fuzzing, it's not an exhaustive analysis. > Here is the .config I am using[5]. > > Thanks, > > Francis Deslauriers > EfficiOS inc. > > > [1]: https://pastebin.com/Mpp9Yzqb > [2]: https://pastebin.com/CtsfzUwG > [3]: https://pastebin.com/txxuJXrz > [4]: https://pastebin.com/8qrJvzD3 > [5]: https://pastebin.com/x5q0sgyK > > Francis Deslauriers (2): > kprobe: fix: Add _ASM_NOKPROBE to x86 apic interrupt macro > kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist > > arch/x86/entry/entry_64.S | 1 + > kernel/trace/ftrace.c | 2 ++ > 2 files changed, 3 insertions(+) > > -- > 2.7.4 > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist 2017-07-16 14:37 ` Masami Hiramatsu @ 2017-07-16 15:46 ` Masami Hiramatsu 2017-07-17 18:46 ` Francis Deslauriers 0 siblings, 1 reply; 35+ messages in thread From: Masami Hiramatsu @ 2017-07-16 15:46 UTC (permalink / raw) To: Masami Hiramatsu Cc: Francis Deslauriers, rostedt, peterz, mathieu.desnoyers, linux-kernel On Sun, 16 Jul 2017 23:37:44 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > So, the story what the stack said, > > - optimized_callback() calls get_kprobe_ctlblk(), and this_cpu_ptr(&kprobe_ctlblk) caused a page fault (in apic_timer_interrupt, does it cause any problem?) > - and following call-chain occured > async_page_fault -> error_entry -> trace_hardirqs_off_thunk -> > trace_hardirqs_off_caller > - "mov %gs:0xc400,%rdx" caused async_page_fault() again. > > Since trace_hardirqs_off_thunk() stores general registers on > the stack, there are some noises. > > [ 114.429637] FS: 00000000021e7880(0000) GS:ffff88001fd40000(0000) knlGS:0000000000000000 > > So, the problem seems that cpu can not access to per-cpu pages. OK, I got the root cause of this issue. Since at the irqentry code, segment registers are not prepared for kernel yet (e.g. interrupted in user-mode), so we must not optimize it. I found we had already checked that by checking __entry_text_start/end, but that is not enough, we need irqentry_text check too. Here I made another patch, please try it. ----- kprobes/x86: Do not jump-optimize kprobes on irq entry code From: Masami Hiramatsu <mhiramat@kernel.org> Since the segment registers are not prepared for kernel in the irq-entry code, if a kprobe on such code is jump-optimized, accessing per-cpu variables may cause kernel panic. However, if the kprobe is not optimized, it kicks int3 exception and set segment registers correctly. This checks probe-address and if it is in irq-entry code, it prohibits optimizing such kprobes. This means we can continuously probing such interrupt handlers by kprobes but it is not optimized anymore. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com> --- arch/x86/entry/entry_64.S | 2 +- arch/x86/include/asm/unwind.h | 1 + arch/x86/kernel/kprobes/opt.c | 4 ++-- arch/x86/kernel/unwind_frame.c | 4 ++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index a9a8027..95bca8b 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -675,7 +675,7 @@ apicinterrupt3 \num trace(\sym) smp_trace(\sym) #endif /* Make sure APIC interrupt handlers end up in the irqentry section: */ -#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) || defined(CONFIG_KPROBES) # define PUSH_SECTION_IRQENTRY .pushsection .irqentry.text, "ax" # define POP_SECTION_IRQENTRY .popsection #else diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h index e667649..a9896fb9 100644 --- a/arch/x86/include/asm/unwind.h +++ b/arch/x86/include/asm/unwind.h @@ -28,6 +28,7 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task, bool unwind_next_frame(struct unwind_state *state); unsigned long unwind_get_return_address(struct unwind_state *state); +bool in_entry_code(unsigned long ip); static inline bool unwind_done(struct unwind_state *state) { diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c index 69ea0bc..a51c144 100644 --- a/arch/x86/kernel/kprobes/opt.c +++ b/arch/x86/kernel/kprobes/opt.c @@ -39,6 +39,7 @@ #include <asm/insn.h> #include <asm/debugreg.h> #include <asm/set_memory.h> +#include <asm/unwind.h> #include "common.h" @@ -253,8 +254,7 @@ static int can_optimize(unsigned long paddr) * Do not optimize in the entry code due to the unstable * stack handling. */ - if ((paddr >= (unsigned long)__entry_text_start) && - (paddr < (unsigned long)__entry_text_end)) + if (in_entry_code(paddr)) return 0; /* Check there is enough space for a relative jump. */ diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c index b9389d7..95123ce 100644 --- a/arch/x86/kernel/unwind_frame.c +++ b/arch/x86/kernel/unwind_frame.c @@ -84,14 +84,14 @@ static size_t regs_size(struct pt_regs *regs) return sizeof(*regs); } -static bool in_entry_code(unsigned long ip) +bool in_entry_code(unsigned long ip) { char *addr = (char *)ip; if (addr >= __entry_text_start && addr < __entry_text_end) return true; -#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) || defined(CONFIG_KPROBES) if (addr >= __irqentry_text_start && addr < __irqentry_text_end) return true; #endif -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist 2017-07-16 15:46 ` Masami Hiramatsu @ 2017-07-17 18:46 ` Francis Deslauriers 0 siblings, 0 replies; 35+ messages in thread From: Francis Deslauriers @ 2017-07-17 18:46 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: rostedt, peterz, Mathieu Desnoyers, linux-kernel 2017-07-16 11:46 GMT-04:00 Masami Hiramatsu <mhiramat@kernel.org>: > > On Sun, 16 Jul 2017 23:37:44 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > So, the story what the stack said, > > > > - optimized_callback() calls get_kprobe_ctlblk(), and this_cpu_ptr(&kprobe_ctlblk) caused a page fault (in apic_timer_interrupt, does it cause any problem?) > > - and following call-chain occured > > async_page_fault -> error_entry -> trace_hardirqs_off_thunk -> > > trace_hardirqs_off_caller > > - "mov %gs:0xc400,%rdx" caused async_page_fault() again. > > > > Since trace_hardirqs_off_thunk() stores general registers on > > the stack, there are some noises. > > > > [ 114.429637] FS: 00000000021e7880(0000) GS:ffff88001fd40000(0000) knlGS:0000000000000000 > > > > So, the problem seems that cpu can not access to per-cpu pages. > > OK, I got the root cause of this issue. Since at the irqentry code, > segment registers are not prepared for kernel yet (e.g. interrupted > in user-mode), so we must not optimize it. > I found we had already checked that by checking __entry_text_start/end, > but that is not enough, we need irqentry_text check too. > > Here I made another patch, please try it. Hi, This patch fixes the apic_timer_interrupt crash I was seeing. Thank you! Tested-by: Francis Deslauriers <francis.deslauriers@efficios.com> > > ----- > kprobes/x86: Do not jump-optimize kprobes on irq entry code > > From: Masami Hiramatsu <mhiramat@kernel.org> > > Since the segment registers are not prepared for kernel > in the irq-entry code, if a kprobe on such code is > jump-optimized, accessing per-cpu variables may cause > kernel panic. > However, if the kprobe is not optimized, it kicks int3 > exception and set segment registers correctly. > > This checks probe-address and if it is in irq-entry code, > it prohibits optimizing such kprobes. This means we can > continuously probing such interrupt handlers by kprobes > but it is not optimized anymore. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com> > --- > arch/x86/entry/entry_64.S | 2 +- > arch/x86/include/asm/unwind.h | 1 + > arch/x86/kernel/kprobes/opt.c | 4 ++-- > arch/x86/kernel/unwind_frame.c | 4 ++-- > 4 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S > index a9a8027..95bca8b 100644 > --- a/arch/x86/entry/entry_64.S > +++ b/arch/x86/entry/entry_64.S > @@ -675,7 +675,7 @@ apicinterrupt3 \num trace(\sym) smp_trace(\sym) > #endif > > /* Make sure APIC interrupt handlers end up in the irqentry section: */ > -#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) > +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) || defined(CONFIG_KPROBES) > # define PUSH_SECTION_IRQENTRY .pushsection .irqentry.text, "ax" > # define POP_SECTION_IRQENTRY .popsection > #else > diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h > index e667649..a9896fb9 100644 > --- a/arch/x86/include/asm/unwind.h > +++ b/arch/x86/include/asm/unwind.h > @@ -28,6 +28,7 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task, > bool unwind_next_frame(struct unwind_state *state); > > unsigned long unwind_get_return_address(struct unwind_state *state); > +bool in_entry_code(unsigned long ip); > > static inline bool unwind_done(struct unwind_state *state) > { > diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c > index 69ea0bc..a51c144 100644 > --- a/arch/x86/kernel/kprobes/opt.c > +++ b/arch/x86/kernel/kprobes/opt.c > @@ -39,6 +39,7 @@ > #include <asm/insn.h> > #include <asm/debugreg.h> > #include <asm/set_memory.h> > +#include <asm/unwind.h> > > #include "common.h" > > @@ -253,8 +254,7 @@ static int can_optimize(unsigned long paddr) > * Do not optimize in the entry code due to the unstable > * stack handling. > */ > - if ((paddr >= (unsigned long)__entry_text_start) && > - (paddr < (unsigned long)__entry_text_end)) > + if (in_entry_code(paddr)) > return 0; > > /* Check there is enough space for a relative jump. */ > diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c > index b9389d7..95123ce 100644 > --- a/arch/x86/kernel/unwind_frame.c > +++ b/arch/x86/kernel/unwind_frame.c > @@ -84,14 +84,14 @@ static size_t regs_size(struct pt_regs *regs) > return sizeof(*regs); > } > > -static bool in_entry_code(unsigned long ip) > +bool in_entry_code(unsigned long ip) > { > char *addr = (char *)ip; > > if (addr >= __entry_text_start && addr < __entry_text_end) > return true; > > -#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) > +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) || defined(CONFIG_KPROBES) > if (addr >= __irqentry_text_start && addr < __irqentry_text_end) > return true; > #endif > -- > Masami Hiramatsu <mhiramat@kernel.org> -- Francis Deslauriers Software developer EfficiOS inc. ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2018-07-26 1:14 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-14 14:58 [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist Francis Deslauriers 2017-07-14 14:58 ` [PATCH 1/2] kprobe: fix: Add _ASM_NOKPROBE to x86 apic interrupt macro Francis Deslauriers 2017-07-14 14:58 ` [PATCH 2/2] kprobe: fix: Add ftrace_ops_assist_func to kprobe blacklist Francis Deslauriers 2017-07-14 18:29 ` Steven Rostedt 2018-03-16 15:18 ` Francis Deslauriers 2018-03-16 15:25 ` Steven Rostedt 2018-03-16 16:28 ` Mathieu Desnoyers 2018-03-16 16:41 ` Steven Rostedt 2018-03-16 16:48 ` Steven Rostedt 2018-03-16 17:53 ` Mathieu Desnoyers 2018-03-16 19:02 ` Steven Rostedt 2018-03-17 0:13 ` Masami Hiramatsu 2018-03-17 1:22 ` Masami Hiramatsu 2018-03-17 3:01 ` Steven Rostedt 2018-03-17 7:57 ` Masami Hiramatsu 2018-07-03 22:30 ` Steven Rostedt 2018-07-11 19:34 ` Francis Deslauriers 2018-07-11 19:56 ` Steven Rostedt 2018-07-12 0:40 ` Francis Deslauriers 2018-07-12 13:59 ` Masami Hiramatsu 2018-07-12 13:46 ` Masami Hiramatsu 2018-03-17 0:08 ` Masami Hiramatsu 2018-07-12 17:54 ` [PATCH 0/2] tracing: kprobes: Prohibit probing on notrace functions Francis Deslauriers 2018-07-12 17:54 ` [PATCH 1/2] " Francis Deslauriers 2018-07-12 21:49 ` Steven Rostedt 2018-07-13 2:53 ` Masami Hiramatsu 2018-07-13 12:18 ` Steven Rostedt 2018-07-26 0:41 ` Masami Hiramatsu 2018-07-26 1:13 ` Steven Rostedt 2018-07-12 17:54 ` [PATCH 2/2] selftest/ftrace: Move kprobe selftest function to separate compile unit Francis Deslauriers 2017-07-14 18:27 ` [PATCH 0/2] kprobe: Fix: add symbols to kprobe blacklist Steven Rostedt 2017-07-16 15:59 ` Masami Hiramatsu 2017-07-16 14:37 ` Masami Hiramatsu 2017-07-16 15:46 ` Masami Hiramatsu 2017-07-17 18:46 ` Francis Deslauriers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).