* [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem @ 2019-01-07 13:31 Masami Hiramatsu 2019-01-07 13:32 ` [PATCH 1/2] x86/kprobes: Verify stack frame on kretprobe Masami Hiramatsu ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Masami Hiramatsu @ 2019-01-07 13:31 UTC (permalink / raw) To: Ingo Molnar Cc: Masami Hiramatsu, peterz, Mathieu Desnoyers, linux-kernel, Andrea Righi, Steven Rostedt Hello, On recent talk with Andrea, I started more precise investigation on the kernel panic with kretprobes on notrace functions, which Francis had been reported last year ( https://lkml.org/lkml/2017/7/14/466 ). At first, I tried to reproduce the issue. I picked up __fdget and ftrace_ops_assist_func as probed functions. With CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, I could reproduce the kernel panic as below. ===== /sys/kernel/debug/tracing # echo "r:event_1 __fdget" >> kprobe_events /sys/kernel/debug/tracing # echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events /sys/kernel/debug/tracing # echo 1 > events/kprobes/enable [ 70.491856] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 [ 70.493203] PGD 800000001c62e067 P4D 800000001c62e067 PUD 1b5bf067 PMD 0 [ 70.494247] Oops: 0010 [#1] PREEMPT SMP PTI [ 70.494918] CPU: 6 PID: 1210 Comm: sh Not tainted 4.20.0-rc3+ #58 [ 70.495931] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.1-0-g8891697-prebuilt.qemu-project.org 04/01/2014 [ 70.497906] RIP: 0010:0x10 [ 70.498465] Code: Bad RIP value. [ 70.499077] RSP: 0018:ffffb1d4c0347e78 EFLAGS: 00010246 [ 70.499959] RAX: 00000000fffffff7 RBX: 0000000000000000 RCX: 0000000000000000 [ 70.501383] RDX: ffff88f19f9c4f80 RSI: ffffffffb7d75e12 RDI: ffffffffb7d0ede7 [ 70.502501] RBP: 00007ffc7061af20 R08: 0000000080000002 R09: ffff88f19f9c4f80 [ 70.503698] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000005401 [ 70.504810] R13: 0000000000000000 R14: ffffb1d4c0347f58 R15: 0000000000000000 [ 70.506028] FS: 0000000000922880(0000) GS:ffff88f19d380000(0000) knlGS:0000000000000000 [ 70.507354] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 70.508271] CR2: ffffffffffffffe6 CR3: 000000001f916000 CR4: 00000000000006e0 [ 70.509419] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 70.510803] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 70.511748] Call Trace: [ 70.512225] ? ksys_ioctl+0x70/0x70 [ 70.512884] ? __fdget+0x5/0x10 [ 70.513454] ? __fdget+0x5/0x10 [ 70.513980] ? copy_oldmem_page_encrypted+0x20/0x20 [ 70.514815] ? __x64_sys_ioctl+0x16/0x20 [ 70.515596] ? do_syscall_64+0x50/0x100 [ 70.516229] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 70.517143] Modules linked in: [ 70.517806] CR2: 0000000000000010 [ 70.518527] ---[ end trace ece844ac05189f10 ]--- [ 70.519417] RIP: 0010:0x10 [ 70.520026] Code: Bad RIP value. [ 70.520800] RSP: 0018:ffffb1d4c0347e78 EFLAGS: 00010246 [ 70.521948] RAX: 00000000fffffff7 RBX: 0000000000000000 RCX: 0000000000000000 [ 70.523315] RDX: ffff88f19f9c4f80 RSI: ffffffffb7d75e12 RDI: ffffffffb7d0ede7 [ 70.524515] RBP: 00007ffc7061af20 R08: 0000000080000002 R09: ffff88f19f9c4f80 [ 70.525702] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000005401 [ 70.526715] R13: 0000000000000000 R14: ffffb1d4c0347f58 R15: 0000000000000000 [ 70.527673] FS: 0000000000922880(0000) GS:ffff88f19d380000(0000) knlGS:0000000000000000 [ 70.528896] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 70.529851] CR2: ffffffffffffffe6 CR3: 000000001f916000 CR4: 00000000000006e0 [ 70.530922] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 70.531907] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Killed ===== This seems the kernel is trying to execute incorrect address. Next, I focused on the combination of probes. From Francis's report, this issue caused by the combination of kretprobes, not kprobes. I ensured that was true. r __fdget & r ftrace_ops_assist_func => NG p __fdget & p ftrace_ops_assist_func => OK p __fdget & r ftrace_ops_assist_func => OK r __fdget & p ftrace_ops_assist_func => OK r: kretprobe, p: kprobe This gave me a hint of what happened. I can explain the cause of this issue as below; Correct processing of kretprobe on probed-function. <caller> -><probed-function> ->fentry ->ftrace_ops_assist_func() ->kprobe_ftrace_handler() ...->pre_handler_kretprobe() push the return address (caller) of probed-function to top of the kretprobe list and replace it with kretprobe_trampoline. <-(ftrace_ops_assist_func()) <-(fentry) <-(probed-function) [kretprobe_trampoline] ->tampoline_handler() pop the return address (caller) from top of the kretprobe list <-(trampoline_handler()) <caller> When we put a kretprobe on ftrace_ops_assist_func(), below happens <caller> -><probed-function> ->fentry ->ftrace_ops_assist_func() ->int3 ->kprobe_int3_handler() ...->pre_handler_kretprobe() push the return address (*fentry*) of ftrace_ops_assist_func() to top of the kretprobe list and replace it with kretprobe_trampoline. <-kprobe_int3_handler() <-(int3) ->kprobe_ftrace_handler() ...->pre_handler_kretprobe() push the return address (caller) of probed-function to top of the kretprobe list and replace it with kretprobe_trampoline. <-(kprobe_ftrace_handler()) <-(ftrace_ops_assist_func()) [kretprobe_trampoline] ->tampoline_handler() pop the return address (caller) from top of the kretprobe list <-(trampoline_handler()) <caller> [run caller with incorrect stack information] <-(<caller>) !!KERNEL PANIC!! Therefore, this kernel panic happens only when we put 2 k*ret*probes on ftrace_ops_assist_func() and other functions. If we put kprobes, it doesn't cause any issue, since it doesn't change the return address. To fix (or just avoid) this issue, we can introduce a frame pointer verification to skip wrong order entries. And I also would like to blacklist those functions because those are part of ftrace-based kprobe handling routine. BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE I'm trying to find out other notrace functions which can cause kernel crash by probing. Mostly done on x86, so I'll post it after this series. Thank you, --- Masami Hiramatsu (2): x86/kprobes: Verify stack frame on kretprobe kprobes: Mark ftrace mcount handler functions nokprobe arch/x86/kernel/kprobes/core.c | 26 ++++++++++++++++++++++++++ include/linux/kprobes.h | 1 + kernel/trace/ftrace.c | 5 ++++- 3 files changed, 31 insertions(+), 1 deletion(-) -- Masami Hiramatsu (Linaro) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] x86/kprobes: Verify stack frame on kretprobe 2019-01-07 13:31 [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem Masami Hiramatsu @ 2019-01-07 13:32 ` Masami Hiramatsu 2019-01-07 13:32 ` [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe Masami Hiramatsu ` (2 subsequent siblings) 3 siblings, 0 replies; 18+ messages in thread From: Masami Hiramatsu @ 2019-01-07 13:32 UTC (permalink / raw) To: Ingo Molnar Cc: Masami Hiramatsu, peterz, Mathieu Desnoyers, linux-kernel, Andrea Righi, Steven Rostedt Verify the stack frame pointer on kretprobe trampoline handler, If the stack frame pointer does not match, it skips the wrong entry and tries to find correct one. This can happen if user puts the kretprobe on the function which can be used in the path of ftrace user-function call. Such functions should not be probed, so this adds a warning message that reports which function should be blacklisted. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> --- arch/x86/kernel/kprobes/core.c | 26 ++++++++++++++++++++++++++ include/linux/kprobes.h | 1 + 2 files changed, 27 insertions(+) diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index 4ba75afba527..69b6400d1ce2 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -569,6 +569,7 @@ void arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs) unsigned long *sara = stack_addr(regs); ri->ret_addr = (kprobe_opcode_t *) *sara; + ri->fp = sara; /* Replace the return addr with trampoline addr */ *sara = (unsigned long) &kretprobe_trampoline; @@ -759,15 +760,21 @@ static __used void *trampoline_handler(struct pt_regs *regs) unsigned long flags, orig_ret_address = 0; unsigned long trampoline_address = (unsigned long)&kretprobe_trampoline; kprobe_opcode_t *correct_ret_addr = NULL; + void *frame_pointer; + bool skipped = false; INIT_HLIST_HEAD(&empty_rp); kretprobe_hash_lock(current, &head, &flags); /* fixup registers */ #ifdef CONFIG_X86_64 regs->cs = __KERNEL_CS; + /* On x86-64, we use pt_regs->sp for return address holder. */ + frame_pointer = ®s->sp; #else regs->cs = __KERNEL_CS | get_kernel_rpl(); regs->gs = 0; + /* On x86-32, we use pt_regs->flags for return address holder. */ + frame_pointer = ®s->flags; #endif regs->ip = trampoline_address; regs->orig_ax = ~0UL; @@ -789,8 +796,25 @@ static __used void *trampoline_handler(struct pt_regs *regs) if (ri->task != current) /* another task is sharing our hash bucket */ continue; + /* + * Return probes must be pushed on this hash list correct + * order (same as return order) so that it can be poped + * correctly. However, if we find it is pushed it incorrect + * order, this means we find a function which should not be + * probed, because the wrong order entry is pushed on the + * path of processing other kretprobe itself. + */ + if (ri->fp != frame_pointer) { + if (!skipped) + pr_warn("kretprobe is stacked incorrectly. Trying to fixup.\n"); + skipped = true; + continue; + } orig_ret_address = (unsigned long)ri->ret_addr; + if (skipped) + pr_warn("%ps must be blacklisted because of incorrect kretprobe order\n", + ri->rp->kp.addr); if (orig_ret_address != trampoline_address) /* @@ -808,6 +832,8 @@ static __used void *trampoline_handler(struct pt_regs *regs) if (ri->task != current) /* another task is sharing our hash bucket */ continue; + if (ri->fp != frame_pointer) + continue; orig_ret_address = (unsigned long)ri->ret_addr; if (ri->rp && ri->rp->handler) { diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h index e07e91daaacc..72ff78c33033 100644 --- a/include/linux/kprobes.h +++ b/include/linux/kprobes.h @@ -173,6 +173,7 @@ struct kretprobe_instance { struct kretprobe *rp; kprobe_opcode_t *ret_addr; struct task_struct *task; + void *fp; char data[0]; }; ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe 2019-01-07 13:31 [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem Masami Hiramatsu 2019-01-07 13:32 ` [PATCH 1/2] x86/kprobes: Verify stack frame on kretprobe Masami Hiramatsu @ 2019-01-07 13:32 ` Masami Hiramatsu 2019-01-07 14:55 ` Andrea Righi ` (2 more replies) 2019-01-07 17:28 ` [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem Andrea Righi 2019-01-07 18:34 ` Andrea Righi 3 siblings, 3 replies; 18+ messages in thread From: Masami Hiramatsu @ 2019-01-07 13:32 UTC (permalink / raw) To: Ingo Molnar Cc: Masami Hiramatsu, peterz, Mathieu Desnoyers, linux-kernel, Andrea Righi, Steven Rostedt Mark ftrace mcount handler functions nokprobe since probing on these functions with kretprobe pushes return address incorrectly on kretprobe shadow stack. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com> --- kernel/trace/ftrace.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index f0ff24173a0b..ad4babad4a03 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr) tr->ops->func = ftrace_stub; } -static inline void +static nokprobe_inline void __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, struct ftrace_ops *ignored, struct pt_regs *regs) { @@ -6310,11 +6310,13 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, { __ftrace_ops_list_func(ip, parent_ip, NULL, regs); } +NOKPROBE_SYMBOL(ftrace_ops_list_func); #else static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip) { __ftrace_ops_list_func(ip, parent_ip, NULL, NULL); } +NOKPROBE_SYMBOL(ftrace_ops_no_ops); #endif /* @@ -6341,6 +6343,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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe 2019-01-07 13:32 ` [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe Masami Hiramatsu @ 2019-01-07 14:55 ` Andrea Righi 2019-01-07 17:29 ` Steven Rostedt 2019-01-08 2:40 ` Masami Hiramatsu 2019-01-07 17:23 ` kbuild test robot 2019-01-07 17:38 ` kbuild test robot 2 siblings, 2 replies; 18+ messages in thread From: Andrea Righi @ 2019-01-07 14:55 UTC (permalink / raw) To: Masami Hiramatsu Cc: Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel, Steven Rostedt On Mon, Jan 07, 2019 at 10:32:32PM +0900, Masami Hiramatsu wrote: > Mark ftrace mcount handler functions nokprobe since > probing on these functions with kretprobe pushes > return address incorrectly on kretprobe shadow stack. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com> > --- > kernel/trace/ftrace.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index f0ff24173a0b..ad4babad4a03 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr) > tr->ops->func = ftrace_stub; > } > > -static inline void > +static nokprobe_inline void I think we need to #include <linux/kprobes.h>, otherwise: CC kernel/trace/ftrace.o kernel/trace/ftrace.c:6219:24: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’ static nokprobe_inline void ^~~~ kernel/trace/ftrace.c | 1 + 1 file changed, 1 insertion(+) diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 3a58ad280d83..0333241034d5 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -32,6 +32,7 @@ #include <linux/sort.h> #include <linux/list.h> #include <linux/hash.h> +#include <linux/kprobes.h> #include <linux/rcupdate.h> #include <trace/events/sched.h> Thanks, -Andrea > __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, > struct ftrace_ops *ignored, struct pt_regs *regs) > { > @@ -6310,11 +6310,13 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, > { > __ftrace_ops_list_func(ip, parent_ip, NULL, regs); > } > +NOKPROBE_SYMBOL(ftrace_ops_list_func); > #else > static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip) > { > __ftrace_ops_list_func(ip, parent_ip, NULL, NULL); > } > +NOKPROBE_SYMBOL(ftrace_ops_no_ops); > #endif > > /* > @@ -6341,6 +6343,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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe 2019-01-07 14:55 ` Andrea Righi @ 2019-01-07 17:29 ` Steven Rostedt 2019-01-08 2:41 ` Masami Hiramatsu 2019-01-08 2:40 ` Masami Hiramatsu 1 sibling, 1 reply; 18+ messages in thread From: Steven Rostedt @ 2019-01-07 17:29 UTC (permalink / raw) To: Andrea Righi Cc: Masami Hiramatsu, Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel On Mon, 7 Jan 2019 15:55:36 +0100 Andrea Righi <righi.andrea@gmail.com> wrote: > On Mon, Jan 07, 2019 at 10:32:32PM +0900, Masami Hiramatsu wrote: > > Mark ftrace mcount handler functions nokprobe since > > probing on these functions with kretprobe pushes > > return address incorrectly on kretprobe shadow stack. > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com> > > --- > > kernel/trace/ftrace.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index f0ff24173a0b..ad4babad4a03 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr) > > tr->ops->func = ftrace_stub; > > } > > > > -static inline void > > +static nokprobe_inline void > > I think we need to #include <linux/kprobes.h>, otherwise: > > CC kernel/trace/ftrace.o > kernel/trace/ftrace.c:6219:24: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’ > static nokprobe_inline void > ^~~~ > > kernel/trace/ftrace.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 3a58ad280d83..0333241034d5 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -32,6 +32,7 @@ > #include <linux/sort.h> > #include <linux/list.h> > #include <linux/hash.h> > +#include <linux/kprobes.h> > #include <linux/rcupdate.h> > > #include <trace/events/sched.h> And zero day just caught it too. Masami, want to fold this into your patch and send out a v2? -- Steve ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe 2019-01-07 17:29 ` Steven Rostedt @ 2019-01-08 2:41 ` Masami Hiramatsu 0 siblings, 0 replies; 18+ messages in thread From: Masami Hiramatsu @ 2019-01-08 2:41 UTC (permalink / raw) To: Steven Rostedt Cc: Andrea Righi, Masami Hiramatsu, Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel On Mon, 7 Jan 2019 12:29:51 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 7 Jan 2019 15:55:36 +0100 > Andrea Righi <righi.andrea@gmail.com> wrote: > > > On Mon, Jan 07, 2019 at 10:32:32PM +0900, Masami Hiramatsu wrote: > > > Mark ftrace mcount handler functions nokprobe since > > > probing on these functions with kretprobe pushes > > > return address incorrectly on kretprobe shadow stack. > > > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > > Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com> > > > --- > > > kernel/trace/ftrace.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > > index f0ff24173a0b..ad4babad4a03 100644 > > > --- a/kernel/trace/ftrace.c > > > +++ b/kernel/trace/ftrace.c > > > @@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr) > > > tr->ops->func = ftrace_stub; > > > } > > > > > > -static inline void > > > +static nokprobe_inline void > > > > I think we need to #include <linux/kprobes.h>, otherwise: > > > > CC kernel/trace/ftrace.o > > kernel/trace/ftrace.c:6219:24: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’ > > static nokprobe_inline void > > ^~~~ > > > > kernel/trace/ftrace.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index 3a58ad280d83..0333241034d5 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -32,6 +32,7 @@ > > #include <linux/sort.h> > > #include <linux/list.h> > > #include <linux/hash.h> > > +#include <linux/kprobes.h> > > #include <linux/rcupdate.h> > > > > #include <trace/events/sched.h> > > And zero day just caught it too. > > Masami, want to fold this into your patch and send out a v2? Yes, I'll do that. Thank you, > > -- Steve -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe 2019-01-07 14:55 ` Andrea Righi 2019-01-07 17:29 ` Steven Rostedt @ 2019-01-08 2:40 ` Masami Hiramatsu 1 sibling, 0 replies; 18+ messages in thread From: Masami Hiramatsu @ 2019-01-08 2:40 UTC (permalink / raw) To: Andrea Righi Cc: Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel, Steven Rostedt On Mon, 7 Jan 2019 15:55:36 +0100 Andrea Righi <righi.andrea@gmail.com> wrote: > On Mon, Jan 07, 2019 at 10:32:32PM +0900, Masami Hiramatsu wrote: > > Mark ftrace mcount handler functions nokprobe since > > probing on these functions with kretprobe pushes > > return address incorrectly on kretprobe shadow stack. > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com> > > --- > > kernel/trace/ftrace.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index f0ff24173a0b..ad4babad4a03 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr) > > tr->ops->func = ftrace_stub; > > } > > > > -static inline void > > +static nokprobe_inline void > > I think we need to #include <linux/kprobes.h>, otherwise: > > CC kernel/trace/ftrace.o > kernel/trace/ftrace.c:6219:24: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’ > static nokprobe_inline void > ^~~~ > > kernel/trace/ftrace.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 3a58ad280d83..0333241034d5 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -32,6 +32,7 @@ > #include <linux/sort.h> > #include <linux/list.h> > #include <linux/hash.h> > +#include <linux/kprobes.h> > #include <linux/rcupdate.h> > > #include <trace/events/sched.h> Oops, I missed it while reordering other patches... Thank you, > > Thanks, > -Andrea > > > __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, > > struct ftrace_ops *ignored, struct pt_regs *regs) > > { > > @@ -6310,11 +6310,13 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, > > { > > __ftrace_ops_list_func(ip, parent_ip, NULL, regs); > > } > > +NOKPROBE_SYMBOL(ftrace_ops_list_func); > > #else > > static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip) > > { > > __ftrace_ops_list_func(ip, parent_ip, NULL, NULL); > > } > > +NOKPROBE_SYMBOL(ftrace_ops_no_ops); > > #endif > > > > /* > > @@ -6341,6 +6343,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 -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe 2019-01-07 13:32 ` [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe Masami Hiramatsu 2019-01-07 14:55 ` Andrea Righi @ 2019-01-07 17:23 ` kbuild test robot 2019-01-07 17:38 ` kbuild test robot 2 siblings, 0 replies; 18+ messages in thread From: kbuild test robot @ 2019-01-07 17:23 UTC (permalink / raw) To: Masami Hiramatsu Cc: kbuild-all, Ingo Molnar, Masami Hiramatsu, peterz, Mathieu Desnoyers, linux-kernel, Andrea Righi, Steven Rostedt [-- Attachment #1: Type: text/plain, Size: 8336 bytes --] Hi Masami, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.0-rc1 next-20190107] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Masami-Hiramatsu/kprobes-Fix-kretprobe-incorrect-stacking-order-problem/20190108-003448 config: i386-randconfig-x075-201901 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All error/warnings (new ones prefixed by >>): >> kernel//trace/ftrace.c:6219:24: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'void' static nokprobe_inline void ^~~~ kernel//trace/ftrace.c: In function 'ftrace_ops_list_func': >> kernel//trace/ftrace.c:6277:2: error: implicit declaration of function '__ftrace_ops_list_func'; did you mean 'ftrace_ops_list_func'? [-Werror=implicit-function-declaration] __ftrace_ops_list_func(ip, parent_ip, NULL, regs); ^~~~~~~~~~~~~~~~~~~~~~ ftrace_ops_list_func kernel//trace/ftrace.c: At top level: >> kernel//trace/ftrace.c:6279:1: warning: data definition has no type or storage class NOKPROBE_SYMBOL(ftrace_ops_list_func); ^~~~~~~~~~~~~~~ >> kernel//trace/ftrace.c:6279:1: error: type defaults to 'int' in declaration of 'NOKPROBE_SYMBOL' [-Werror=implicit-int] >> kernel//trace/ftrace.c:6279:1: warning: parameter names (without types) in function declaration kernel//trace/ftrace.c:6312:1: warning: data definition has no type or storage class NOKPROBE_SYMBOL(ftrace_ops_assist_func); ^~~~~~~~~~~~~~~ kernel//trace/ftrace.c:6312:1: error: type defaults to 'int' in declaration of 'NOKPROBE_SYMBOL' [-Werror=implicit-int] kernel//trace/ftrace.c:6312:1: warning: parameter names (without types) in function declaration cc1: some warnings being treated as errors vim +6277 kernel//trace/ftrace.c 4104d326 Steven Rostedt (Red Hat 2014-01-10 6218) 0b5a8971 Masami Hiramatsu 2019-01-07 @6219 static nokprobe_inline void 2f5f6ad9 Steven Rostedt 2011-08-08 6220 __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, a1e2e31d Steven Rostedt 2011-08-09 6221 struct ftrace_ops *ignored, struct pt_regs *regs) b848914c Steven Rostedt 2011-05-04 6222 { cdbe61bf Steven Rostedt 2011-05-05 6223 struct ftrace_ops *op; edc15caf Steven Rostedt 2012-11-02 6224 int bit; b848914c Steven Rostedt 2011-05-04 6225 edc15caf Steven Rostedt 2012-11-02 6226 bit = trace_test_and_set_recursion(TRACE_LIST_START, TRACE_LIST_MAX); edc15caf Steven Rostedt 2012-11-02 6227 if (bit < 0) b1cff0ad Steven Rostedt 2011-05-25 6228 return; b1cff0ad Steven Rostedt 2011-05-25 6229 cdbe61bf Steven Rostedt 2011-05-05 6230 /* cdbe61bf Steven Rostedt 2011-05-05 6231 * Some of the ops may be dynamically allocated, 74401729 Paul E. McKenney 2018-11-06 6232 * they must be freed after a synchronize_rcu(). cdbe61bf Steven Rostedt 2011-05-05 6233 */ cdbe61bf Steven Rostedt 2011-05-05 6234 preempt_disable_notrace(); ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6235) 0a016409 Steven Rostedt 2012-11-02 6236 do_for_each_ftrace_op(op, ftrace_ops_list) { ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6237) /* ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6238) * Check the following for each ops before calling their func: ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6239) * if RCU flag is set, then rcu_is_watching() must be true ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6240) * if PER_CPU is set, then ftrace_function_local_disable() ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6241) * must be false ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6242) * Otherwise test if the ip matches the ops filter ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6243) * ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6244) * If any of the above fails then the op->func() is not executed. ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6245) */ ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6246) if ((!(op->flags & FTRACE_OPS_FL_RCU) || rcu_is_watching()) && ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6247) ftrace_ops_test(op, ip, regs)) { 1d48d596 Steven Rostedt (Red Hat 2014-06-25 6248) if (FTRACE_WARN_ON(!op->func)) { 1d48d596 Steven Rostedt (Red Hat 2014-06-25 6249) pr_warn("op=%p %pS\n", op, op); 4104d326 Steven Rostedt (Red Hat 2014-01-10 6250) goto out; 4104d326 Steven Rostedt (Red Hat 2014-01-10 6251) } a1e2e31d Steven Rostedt 2011-08-09 6252 op->func(ip, parent_ip, op, regs); 4104d326 Steven Rostedt (Red Hat 2014-01-10 6253) } 0a016409 Steven Rostedt 2012-11-02 6254 } while_for_each_ftrace_op(op); 4104d326 Steven Rostedt (Red Hat 2014-01-10 6255) out: cdbe61bf Steven Rostedt 2011-05-05 6256 preempt_enable_notrace(); edc15caf Steven Rostedt 2012-11-02 6257 trace_clear_recursion(bit); b848914c Steven Rostedt 2011-05-04 6258 } b848914c Steven Rostedt 2011-05-04 6259 2f5f6ad9 Steven Rostedt 2011-08-08 6260 /* 2f5f6ad9 Steven Rostedt 2011-08-08 6261 * Some archs only support passing ip and parent_ip. Even though 2f5f6ad9 Steven Rostedt 2011-08-08 6262 * the list function ignores the op parameter, we do not want any 2f5f6ad9 Steven Rostedt 2011-08-08 6263 * C side effects, where a function is called without the caller 2f5f6ad9 Steven Rostedt 2011-08-08 6264 * sending a third parameter. a1e2e31d Steven Rostedt 2011-08-09 6265 * Archs are to support both the regs and ftrace_ops at the same time. a1e2e31d Steven Rostedt 2011-08-09 6266 * If they support ftrace_ops, it is assumed they support regs. a1e2e31d Steven Rostedt 2011-08-09 6267 * If call backs want to use regs, they must either check for regs 06aeaaea Masami Hiramatsu 2012-09-28 6268 * being NULL, or CONFIG_DYNAMIC_FTRACE_WITH_REGS. 06aeaaea Masami Hiramatsu 2012-09-28 6269 * Note, CONFIG_DYNAMIC_FTRACE_WITH_REGS expects a full regs to be saved. a1e2e31d Steven Rostedt 2011-08-09 6270 * An architecture can pass partial regs with ftrace_ops and still b8ec330a Li Bin 2015-11-30 6271 * set the ARCH_SUPPORTS_FTRACE_OPS. 2f5f6ad9 Steven Rostedt 2011-08-08 6272 */ 2f5f6ad9 Steven Rostedt 2011-08-08 6273 #if ARCH_SUPPORTS_FTRACE_OPS 2f5f6ad9 Steven Rostedt 2011-08-08 6274 static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, a1e2e31d Steven Rostedt 2011-08-09 6275 struct ftrace_ops *op, struct pt_regs *regs) 2f5f6ad9 Steven Rostedt 2011-08-08 6276 { a1e2e31d Steven Rostedt 2011-08-09 @6277 __ftrace_ops_list_func(ip, parent_ip, NULL, regs); 2f5f6ad9 Steven Rostedt 2011-08-08 6278 } 0b5a8971 Masami Hiramatsu 2019-01-07 @6279 NOKPROBE_SYMBOL(ftrace_ops_list_func); 2f5f6ad9 Steven Rostedt 2011-08-08 6280 #else 2f5f6ad9 Steven Rostedt 2011-08-08 6281 static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip) 2f5f6ad9 Steven Rostedt 2011-08-08 6282 { a1e2e31d Steven Rostedt 2011-08-09 6283 __ftrace_ops_list_func(ip, parent_ip, NULL, NULL); 2f5f6ad9 Steven Rostedt 2011-08-08 6284 } 0b5a8971 Masami Hiramatsu 2019-01-07 6285 NOKPROBE_SYMBOL(ftrace_ops_no_ops); 2f5f6ad9 Steven Rostedt 2011-08-08 6286 #endif 2f5f6ad9 Steven Rostedt 2011-08-08 6287 :::::: The code at line 6277 was first introduced by commit :::::: a1e2e31d175a1349274eba3465d17616c6725f8c ftrace: Return pt_regs to function trace callback :::::: TO: Steven Rostedt <srostedt@redhat.com> :::::: CC: Steven Rostedt <rostedt@goodmis.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 30841 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe 2019-01-07 13:32 ` [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe Masami Hiramatsu 2019-01-07 14:55 ` Andrea Righi 2019-01-07 17:23 ` kbuild test robot @ 2019-01-07 17:38 ` kbuild test robot 2 siblings, 0 replies; 18+ messages in thread From: kbuild test robot @ 2019-01-07 17:38 UTC (permalink / raw) To: Masami Hiramatsu Cc: kbuild-all, Ingo Molnar, Masami Hiramatsu, peterz, Mathieu Desnoyers, linux-kernel, Andrea Righi, Steven Rostedt [-- Attachment #1: Type: text/plain, Size: 8285 bytes --] Hi Masami, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.0-rc1 next-20190107] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Masami-Hiramatsu/kprobes-Fix-kretprobe-incorrect-stacking-order-problem/20190108-003448 config: i386-randconfig-s3-201901 (attached as .config) compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): kernel/trace/ftrace.c:6219:24: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'void' static nokprobe_inline void ^~~~ kernel/trace/ftrace.c: In function 'ftrace_ops_list_func': >> kernel/trace/ftrace.c:6277:2: error: implicit declaration of function '__ftrace_ops_list_func' [-Werror=implicit-function-declaration] __ftrace_ops_list_func(ip, parent_ip, NULL, regs); ^~~~~~~~~~~~~~~~~~~~~~ kernel/trace/ftrace.c: At top level: kernel/trace/ftrace.c:6279:1: warning: data definition has no type or storage class NOKPROBE_SYMBOL(ftrace_ops_list_func); ^~~~~~~~~~~~~~~ kernel/trace/ftrace.c:6279:1: error: type defaults to 'int' in declaration of 'NOKPROBE_SYMBOL' [-Werror=implicit-int] kernel/trace/ftrace.c:6279:1: warning: parameter names (without types) in function declaration kernel/trace/ftrace.c:6312:1: warning: data definition has no type or storage class NOKPROBE_SYMBOL(ftrace_ops_assist_func); ^~~~~~~~~~~~~~~ kernel/trace/ftrace.c:6312:1: error: type defaults to 'int' in declaration of 'NOKPROBE_SYMBOL' [-Werror=implicit-int] kernel/trace/ftrace.c:6312:1: warning: parameter names (without types) in function declaration cc1: some warnings being treated as errors vim +/__ftrace_ops_list_func +6277 kernel/trace/ftrace.c 4104d326 Steven Rostedt (Red Hat 2014-01-10 6218) 0b5a8971 Masami Hiramatsu 2019-01-07 @6219 static nokprobe_inline void 2f5f6ad9 Steven Rostedt 2011-08-08 6220 __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, a1e2e31d Steven Rostedt 2011-08-09 6221 struct ftrace_ops *ignored, struct pt_regs *regs) b848914c Steven Rostedt 2011-05-04 6222 { cdbe61bf Steven Rostedt 2011-05-05 6223 struct ftrace_ops *op; edc15caf Steven Rostedt 2012-11-02 6224 int bit; b848914c Steven Rostedt 2011-05-04 6225 edc15caf Steven Rostedt 2012-11-02 6226 bit = trace_test_and_set_recursion(TRACE_LIST_START, TRACE_LIST_MAX); edc15caf Steven Rostedt 2012-11-02 6227 if (bit < 0) b1cff0ad Steven Rostedt 2011-05-25 6228 return; b1cff0ad Steven Rostedt 2011-05-25 6229 cdbe61bf Steven Rostedt 2011-05-05 6230 /* cdbe61bf Steven Rostedt 2011-05-05 6231 * Some of the ops may be dynamically allocated, 74401729 Paul E. McKenney 2018-11-06 6232 * they must be freed after a synchronize_rcu(). cdbe61bf Steven Rostedt 2011-05-05 6233 */ cdbe61bf Steven Rostedt 2011-05-05 6234 preempt_disable_notrace(); ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6235) 0a016409 Steven Rostedt 2012-11-02 6236 do_for_each_ftrace_op(op, ftrace_ops_list) { ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6237) /* ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6238) * Check the following for each ops before calling their func: ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6239) * if RCU flag is set, then rcu_is_watching() must be true ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6240) * if PER_CPU is set, then ftrace_function_local_disable() ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6241) * must be false ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6242) * Otherwise test if the ip matches the ops filter ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6243) * ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6244) * If any of the above fails then the op->func() is not executed. ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6245) */ ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6246) if ((!(op->flags & FTRACE_OPS_FL_RCU) || rcu_is_watching()) && ba27f2bc Steven Rostedt (Red Hat 2015-11-30 6247) ftrace_ops_test(op, ip, regs)) { 1d48d596 Steven Rostedt (Red Hat 2014-06-25 6248) if (FTRACE_WARN_ON(!op->func)) { 1d48d596 Steven Rostedt (Red Hat 2014-06-25 6249) pr_warn("op=%p %pS\n", op, op); 4104d326 Steven Rostedt (Red Hat 2014-01-10 6250) goto out; 4104d326 Steven Rostedt (Red Hat 2014-01-10 6251) } a1e2e31d Steven Rostedt 2011-08-09 6252 op->func(ip, parent_ip, op, regs); 4104d326 Steven Rostedt (Red Hat 2014-01-10 6253) } 0a016409 Steven Rostedt 2012-11-02 6254 } while_for_each_ftrace_op(op); 4104d326 Steven Rostedt (Red Hat 2014-01-10 6255) out: cdbe61bf Steven Rostedt 2011-05-05 6256 preempt_enable_notrace(); edc15caf Steven Rostedt 2012-11-02 6257 trace_clear_recursion(bit); b848914c Steven Rostedt 2011-05-04 6258 } b848914c Steven Rostedt 2011-05-04 6259 2f5f6ad9 Steven Rostedt 2011-08-08 6260 /* 2f5f6ad9 Steven Rostedt 2011-08-08 6261 * Some archs only support passing ip and parent_ip. Even though 2f5f6ad9 Steven Rostedt 2011-08-08 6262 * the list function ignores the op parameter, we do not want any 2f5f6ad9 Steven Rostedt 2011-08-08 6263 * C side effects, where a function is called without the caller 2f5f6ad9 Steven Rostedt 2011-08-08 6264 * sending a third parameter. a1e2e31d Steven Rostedt 2011-08-09 6265 * Archs are to support both the regs and ftrace_ops at the same time. a1e2e31d Steven Rostedt 2011-08-09 6266 * If they support ftrace_ops, it is assumed they support regs. a1e2e31d Steven Rostedt 2011-08-09 6267 * If call backs want to use regs, they must either check for regs 06aeaaea Masami Hiramatsu 2012-09-28 6268 * being NULL, or CONFIG_DYNAMIC_FTRACE_WITH_REGS. 06aeaaea Masami Hiramatsu 2012-09-28 6269 * Note, CONFIG_DYNAMIC_FTRACE_WITH_REGS expects a full regs to be saved. a1e2e31d Steven Rostedt 2011-08-09 6270 * An architecture can pass partial regs with ftrace_ops and still b8ec330a Li Bin 2015-11-30 6271 * set the ARCH_SUPPORTS_FTRACE_OPS. 2f5f6ad9 Steven Rostedt 2011-08-08 6272 */ 2f5f6ad9 Steven Rostedt 2011-08-08 6273 #if ARCH_SUPPORTS_FTRACE_OPS 2f5f6ad9 Steven Rostedt 2011-08-08 6274 static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, a1e2e31d Steven Rostedt 2011-08-09 6275 struct ftrace_ops *op, struct pt_regs *regs) 2f5f6ad9 Steven Rostedt 2011-08-08 6276 { a1e2e31d Steven Rostedt 2011-08-09 @6277 __ftrace_ops_list_func(ip, parent_ip, NULL, regs); 2f5f6ad9 Steven Rostedt 2011-08-08 6278 } 0b5a8971 Masami Hiramatsu 2019-01-07 6279 NOKPROBE_SYMBOL(ftrace_ops_list_func); 2f5f6ad9 Steven Rostedt 2011-08-08 6280 #else 2f5f6ad9 Steven Rostedt 2011-08-08 6281 static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip) 2f5f6ad9 Steven Rostedt 2011-08-08 6282 { a1e2e31d Steven Rostedt 2011-08-09 6283 __ftrace_ops_list_func(ip, parent_ip, NULL, NULL); 2f5f6ad9 Steven Rostedt 2011-08-08 6284 } 0b5a8971 Masami Hiramatsu 2019-01-07 6285 NOKPROBE_SYMBOL(ftrace_ops_no_ops); 2f5f6ad9 Steven Rostedt 2011-08-08 6286 #endif 2f5f6ad9 Steven Rostedt 2011-08-08 6287 :::::: The code at line 6277 was first introduced by commit :::::: a1e2e31d175a1349274eba3465d17616c6725f8c ftrace: Return pt_regs to function trace callback :::::: TO: Steven Rostedt <srostedt@redhat.com> :::::: CC: Steven Rostedt <rostedt@goodmis.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 32939 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem 2019-01-07 13:31 [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem Masami Hiramatsu 2019-01-07 13:32 ` [PATCH 1/2] x86/kprobes: Verify stack frame on kretprobe Masami Hiramatsu 2019-01-07 13:32 ` [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe Masami Hiramatsu @ 2019-01-07 17:28 ` Andrea Righi 2019-01-07 18:34 ` Andrea Righi 3 siblings, 0 replies; 18+ messages in thread From: Andrea Righi @ 2019-01-07 17:28 UTC (permalink / raw) To: Masami Hiramatsu Cc: Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel, Steven Rostedt On Mon, Jan 07, 2019 at 10:31:34PM +0900, Masami Hiramatsu wrote: > Hello, > > On recent talk with Andrea, I started more precise investigation on > the kernel panic with kretprobes on notrace functions, which Francis > had been reported last year ( https://lkml.org/lkml/2017/7/14/466 ). > > At first, I tried to reproduce the issue. I picked up __fdget and > ftrace_ops_assist_func as probed functions. > With CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, I could reproduce the kernel > panic as below. > > ===== > /sys/kernel/debug/tracing # echo "r:event_1 __fdget" >> kprobe_events > /sys/kernel/debug/tracing # echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events > /sys/kernel/debug/tracing # echo 1 > events/kprobes/enable > [ 70.491856] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > [ 70.493203] PGD 800000001c62e067 P4D 800000001c62e067 PUD 1b5bf067 PMD 0 > [ 70.494247] Oops: 0010 [#1] PREEMPT SMP PTI > [ 70.494918] CPU: 6 PID: 1210 Comm: sh Not tainted 4.20.0-rc3+ #58 > [ 70.495931] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.1-0-g8891697-prebuilt.qemu-project.org 04/01/2014 > [ 70.497906] RIP: 0010:0x10 > [ 70.498465] Code: Bad RIP value. > [ 70.499077] RSP: 0018:ffffb1d4c0347e78 EFLAGS: 00010246 > [ 70.499959] RAX: 00000000fffffff7 RBX: 0000000000000000 RCX: 0000000000000000 > [ 70.501383] RDX: ffff88f19f9c4f80 RSI: ffffffffb7d75e12 RDI: ffffffffb7d0ede7 > [ 70.502501] RBP: 00007ffc7061af20 R08: 0000000080000002 R09: ffff88f19f9c4f80 > [ 70.503698] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000005401 > [ 70.504810] R13: 0000000000000000 R14: ffffb1d4c0347f58 R15: 0000000000000000 > [ 70.506028] FS: 0000000000922880(0000) GS:ffff88f19d380000(0000) knlGS:0000000000000000 > [ 70.507354] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 70.508271] CR2: ffffffffffffffe6 CR3: 000000001f916000 CR4: 00000000000006e0 > [ 70.509419] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 70.510803] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > [ 70.511748] Call Trace: > [ 70.512225] ? ksys_ioctl+0x70/0x70 > [ 70.512884] ? __fdget+0x5/0x10 > [ 70.513454] ? __fdget+0x5/0x10 > [ 70.513980] ? copy_oldmem_page_encrypted+0x20/0x20 > [ 70.514815] ? __x64_sys_ioctl+0x16/0x20 > [ 70.515596] ? do_syscall_64+0x50/0x100 > [ 70.516229] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe > [ 70.517143] Modules linked in: > [ 70.517806] CR2: 0000000000000010 > [ 70.518527] ---[ end trace ece844ac05189f10 ]--- > [ 70.519417] RIP: 0010:0x10 > [ 70.520026] Code: Bad RIP value. > [ 70.520800] RSP: 0018:ffffb1d4c0347e78 EFLAGS: 00010246 > [ 70.521948] RAX: 00000000fffffff7 RBX: 0000000000000000 RCX: 0000000000000000 > [ 70.523315] RDX: ffff88f19f9c4f80 RSI: ffffffffb7d75e12 RDI: ffffffffb7d0ede7 > [ 70.524515] RBP: 00007ffc7061af20 R08: 0000000080000002 R09: ffff88f19f9c4f80 > [ 70.525702] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000005401 > [ 70.526715] R13: 0000000000000000 R14: ffffb1d4c0347f58 R15: 0000000000000000 > [ 70.527673] FS: 0000000000922880(0000) GS:ffff88f19d380000(0000) knlGS:0000000000000000 > [ 70.528896] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 70.529851] CR2: ffffffffffffffe6 CR3: 000000001f916000 CR4: 00000000000006e0 > [ 70.530922] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 70.531907] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Killed > ===== > > This seems the kernel is trying to execute incorrect address. > > Next, I focused on the combination of probes. From Francis's report, > this issue caused by the combination of kretprobes, not kprobes. > I ensured that was true. > > r __fdget & r ftrace_ops_assist_func => NG > p __fdget & p ftrace_ops_assist_func => OK > p __fdget & r ftrace_ops_assist_func => OK > r __fdget & p ftrace_ops_assist_func => OK > > r: kretprobe, p: kprobe > > This gave me a hint of what happened. I can explain the cause of this > issue as below; > > Correct processing of kretprobe on probed-function. > > <caller> > -><probed-function> > ->fentry > ->ftrace_ops_assist_func() > ->kprobe_ftrace_handler() > ...->pre_handler_kretprobe() > push the return address (caller) of probed-function to top of the > kretprobe list and replace it with kretprobe_trampoline. > <-(ftrace_ops_assist_func()) > <-(fentry) > <-(probed-function) > [kretprobe_trampoline] > ->tampoline_handler() > pop the return address (caller) from top of the kretprobe list > <-(trampoline_handler()) > <caller> > > When we put a kretprobe on ftrace_ops_assist_func(), below happens > > <caller> > -><probed-function> > ->fentry > ->ftrace_ops_assist_func() > ->int3 > ->kprobe_int3_handler() > ...->pre_handler_kretprobe() > push the return address (*fentry*) of ftrace_ops_assist_func() to > top of the kretprobe list and replace it with kretprobe_trampoline. > <-kprobe_int3_handler() > <-(int3) > ->kprobe_ftrace_handler() > ...->pre_handler_kretprobe() > push the return address (caller) of probed-function to top of the > kretprobe list and replace it with kretprobe_trampoline. > <-(kprobe_ftrace_handler()) > <-(ftrace_ops_assist_func()) > [kretprobe_trampoline] > ->tampoline_handler() > pop the return address (caller) from top of the kretprobe list > <-(trampoline_handler()) > <caller> > [run caller with incorrect stack information] > <-(<caller>) > !!KERNEL PANIC!! > > Therefore, this kernel panic happens only when we put 2 k*ret*probes on > ftrace_ops_assist_func() and other functions. If we put kprobes, it > doesn't cause any issue, since it doesn't change the return address. > > To fix (or just avoid) this issue, we can introduce a frame pointer > verification to skip wrong order entries. And I also would like to > blacklist those functions because those are part of ftrace-based > kprobe handling routine. > > BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE > I'm trying to find out other notrace functions which can cause > kernel crash by probing. Mostly done on x86, so I'll post it > after this series. > > Thank you, Apart than the missing include <linux/kprobes.h> in PATCH 2/2 everything else looks good to me. Tested-by: Andrea Righi <righi.andrea@gmail.com> Thanks! -Andrea ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem 2019-01-07 13:31 [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem Masami Hiramatsu ` (2 preceding siblings ...) 2019-01-07 17:28 ` [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem Andrea Righi @ 2019-01-07 18:34 ` Andrea Righi 2019-01-07 19:27 ` Steven Rostedt 3 siblings, 1 reply; 18+ messages in thread From: Andrea Righi @ 2019-01-07 18:34 UTC (permalink / raw) To: Masami Hiramatsu Cc: Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel, Steven Rostedt On Mon, Jan 07, 2019 at 10:31:34PM +0900, Masami Hiramatsu wrote: ... > BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE > I'm trying to find out other notrace functions which can cause > kernel crash by probing. Mostly done on x86, so I'll post it > after this series. Not sure if you found it already, but it looks like some of the _raw_spin_lock/unlock* functions (when they're not inlined) are causing the same problem (or something similar), I can deadlock the system by doing this for example: echo "r:event_1 __fdget" >> kprobe_events echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events echo 1 > events/kprobes/enable [DEADLOCK] Sending the following just in case... Thanks, kernel/locking/spinlock.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c index 936f3d14dd6b..d93e88019239 100644 --- a/kernel/locking/spinlock.c +++ b/kernel/locking/spinlock.c @@ -19,6 +19,7 @@ #include <linux/preempt.h> #include <linux/spinlock.h> #include <linux/interrupt.h> +#include <linux/kprobes.h> #include <linux/debug_locks.h> #include <linux/export.h> @@ -128,6 +129,7 @@ int __lockfunc _raw_spin_trylock(raw_spinlock_t *lock) return __raw_spin_trylock(lock); } EXPORT_SYMBOL(_raw_spin_trylock); +NOKPROBE_SYMBOL(_raw_spin_trylock); #endif #ifndef CONFIG_INLINE_SPIN_TRYLOCK_BH @@ -136,6 +138,7 @@ int __lockfunc _raw_spin_trylock_bh(raw_spinlock_t *lock) return __raw_spin_trylock_bh(lock); } EXPORT_SYMBOL(_raw_spin_trylock_bh); +NOKPROBE_SYMBOL(_raw_spin_trylock_bh); #endif #ifndef CONFIG_INLINE_SPIN_LOCK @@ -144,6 +147,7 @@ void __lockfunc _raw_spin_lock(raw_spinlock_t *lock) __raw_spin_lock(lock); } EXPORT_SYMBOL(_raw_spin_lock); +NOKPROBE_SYMBOL(_raw_spin_lock); #endif #ifndef CONFIG_INLINE_SPIN_LOCK_IRQSAVE @@ -152,6 +156,7 @@ unsigned long __lockfunc _raw_spin_lock_irqsave(raw_spinlock_t *lock) return __raw_spin_lock_irqsave(lock); } EXPORT_SYMBOL(_raw_spin_lock_irqsave); +NOKPROBE_SYMBOL(_raw_spin_lock_irqsave); #endif #ifndef CONFIG_INLINE_SPIN_LOCK_IRQ @@ -160,6 +165,7 @@ void __lockfunc _raw_spin_lock_irq(raw_spinlock_t *lock) __raw_spin_lock_irq(lock); } EXPORT_SYMBOL(_raw_spin_lock_irq); +NOKPROBE_SYMBOL(_raw_spin_lock_irq); #endif #ifndef CONFIG_INLINE_SPIN_LOCK_BH @@ -168,6 +174,7 @@ void __lockfunc _raw_spin_lock_bh(raw_spinlock_t *lock) __raw_spin_lock_bh(lock); } EXPORT_SYMBOL(_raw_spin_lock_bh); +NOKPROBE_SYMBOL(_raw_spin_lock_bh); #endif #ifdef CONFIG_UNINLINE_SPIN_UNLOCK @@ -176,6 +183,7 @@ void __lockfunc _raw_spin_unlock(raw_spinlock_t *lock) __raw_spin_unlock(lock); } EXPORT_SYMBOL(_raw_spin_unlock); +NOKPROBE_SYMBOL(_raw_spin_unlock); #endif #ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQRESTORE @@ -184,6 +192,7 @@ void __lockfunc _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long __raw_spin_unlock_irqrestore(lock, flags); } EXPORT_SYMBOL(_raw_spin_unlock_irqrestore); +NOKPROBE_SYMBOL(_raw_spin_unlock_irqrestore); #endif #ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQ @@ -192,6 +201,7 @@ void __lockfunc _raw_spin_unlock_irq(raw_spinlock_t *lock) __raw_spin_unlock_irq(lock); } EXPORT_SYMBOL(_raw_spin_unlock_irq); +NOKPROBE_SYMBOL(_raw_spin_unlock_irq); #endif #ifndef CONFIG_INLINE_SPIN_UNLOCK_BH @@ -200,6 +210,7 @@ void __lockfunc _raw_spin_unlock_bh(raw_spinlock_t *lock) __raw_spin_unlock_bh(lock); } EXPORT_SYMBOL(_raw_spin_unlock_bh); +NOKPROBE_SYMBOL(_raw_spin_unlock_bh); #endif #ifndef CONFIG_INLINE_READ_TRYLOCK Signed-off-by: Andrea Righi <righi.andrea@gmail.com> ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem 2019-01-07 18:34 ` Andrea Righi @ 2019-01-07 19:27 ` Steven Rostedt 2019-01-07 19:52 ` Andrea Righi 0 siblings, 1 reply; 18+ messages in thread From: Steven Rostedt @ 2019-01-07 19:27 UTC (permalink / raw) To: Andrea Righi Cc: Masami Hiramatsu, Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel On Mon, 7 Jan 2019 19:34:44 +0100 Andrea Righi <righi.andrea@gmail.com> wrote: > On Mon, Jan 07, 2019 at 10:31:34PM +0900, Masami Hiramatsu wrote: > ... > > BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE > > I'm trying to find out other notrace functions which can cause > > kernel crash by probing. Mostly done on x86, so I'll post it > > after this series. > > Not sure if you found it already, but it looks like some of the > _raw_spin_lock/unlock* functions (when they're not inlined) are causing > the same problem (or something similar), I can deadlock the system by > doing this for example: > > echo "r:event_1 __fdget" >> kprobe_events > echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events > echo 1 > events/kprobes/enable > [DEADLOCK] > > Sending the following just in case... > Ug, kretprobe calls spinlocks in the callback? I wonder if we can remove them. I'm guessing this is a different issue than the one that this patch fixes. This sounds like we are calling kretprobe from kretprobe? -- Steve ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem 2019-01-07 19:27 ` Steven Rostedt @ 2019-01-07 19:52 ` Andrea Righi 2019-01-07 19:59 ` Steven Rostedt 0 siblings, 1 reply; 18+ messages in thread From: Andrea Righi @ 2019-01-07 19:52 UTC (permalink / raw) To: Steven Rostedt Cc: Masami Hiramatsu, Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel On Mon, Jan 07, 2019 at 02:27:49PM -0500, Steven Rostedt wrote: > On Mon, 7 Jan 2019 19:34:44 +0100 > Andrea Righi <righi.andrea@gmail.com> wrote: > > > On Mon, Jan 07, 2019 at 10:31:34PM +0900, Masami Hiramatsu wrote: > > ... > > > BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE > > > I'm trying to find out other notrace functions which can cause > > > kernel crash by probing. Mostly done on x86, so I'll post it > > > after this series. > > > > Not sure if you found it already, but it looks like some of the > > _raw_spin_lock/unlock* functions (when they're not inlined) are causing > > the same problem (or something similar), I can deadlock the system by > > doing this for example: > > > > echo "r:event_1 __fdget" >> kprobe_events > > echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events > > echo 1 > events/kprobes/enable > > [DEADLOCK] > > > > Sending the following just in case... > > > > Ug, kretprobe calls spinlocks in the callback? I wonder if we can > remove them. > > I'm guessing this is a different issue than the one that this patch > fixes. This sounds like we are calling kretprobe from kretprobe? > > -- Steve kretprobe_trampoline() -> trampoline_handler() -> kretprobe_hash_lock() -> raw_spin_lock_irqsave() If we put a kretprobe to raw_spin_lock_irqsave() it looks like kretprobe is going to call kretprobe... -Andrea ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem 2019-01-07 19:52 ` Andrea Righi @ 2019-01-07 19:59 ` Steven Rostedt 2019-01-07 21:19 ` Andrea Righi 0 siblings, 1 reply; 18+ messages in thread From: Steven Rostedt @ 2019-01-07 19:59 UTC (permalink / raw) To: Andrea Righi Cc: Masami Hiramatsu, Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel On Mon, 7 Jan 2019 20:52:09 +0100 Andrea Righi <righi.andrea@gmail.com> wrote: > > Ug, kretprobe calls spinlocks in the callback? I wonder if we can > > remove them. > > > > I'm guessing this is a different issue than the one that this patch > > fixes. This sounds like we are calling kretprobe from kretprobe? > > > > -- Steve > > kretprobe_trampoline() > -> trampoline_handler() > -> kretprobe_hash_lock() > -> raw_spin_lock_irqsave() > > If we put a kretprobe to raw_spin_lock_irqsave() it looks like > kretprobe is going to call kretprobe... Right, but we should be able to add some recursion protection to stop that. I have similar protection in the ftrace code. -- Steve ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem 2019-01-07 19:59 ` Steven Rostedt @ 2019-01-07 21:19 ` Andrea Righi 2019-01-07 21:28 ` Steven Rostedt 0 siblings, 1 reply; 18+ messages in thread From: Andrea Righi @ 2019-01-07 21:19 UTC (permalink / raw) To: Steven Rostedt Cc: Masami Hiramatsu, Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel On Mon, Jan 07, 2019 at 02:59:18PM -0500, Steven Rostedt wrote: > On Mon, 7 Jan 2019 20:52:09 +0100 > Andrea Righi <righi.andrea@gmail.com> wrote: > > > > Ug, kretprobe calls spinlocks in the callback? I wonder if we can > > > remove them. > > > > > > I'm guessing this is a different issue than the one that this patch > > > fixes. This sounds like we are calling kretprobe from kretprobe? > > > > > > -- Steve > > > > kretprobe_trampoline() > > -> trampoline_handler() > > -> kretprobe_hash_lock() > > -> raw_spin_lock_irqsave() > > > > If we put a kretprobe to raw_spin_lock_irqsave() it looks like > > kretprobe is going to call kretprobe... > > Right, but we should be able to add some recursion protection to stop > that. I have similar protection in the ftrace code. If we assume that __raw_spin_lock/unlock*() are always inlined a possible way to prevent this recursion could be to use directly those functions to do locking from the kretprobe trampoline. But I'm not sure if that's a safe assumption... if not I'll see if I can find a better solution. Thanks, From: Andrea Righi <righi.andrea@gmail.com> Subject: [PATCH] kprobes: prevent recursion deadlock with kretprobe and spinlocks kretprobe_trampoline() is using a spinlock to protect the hash of kretprobes. Adding a kretprobe to the spinlock functions may cause a recursion deadlock where kretprobe is calling itself: kretprobe_trampoline() -> trampoline_handler() -> kretprobe_hash_lock() -> raw_spin_lock_irqsave() -> _raw_spin_lock_irqsave() kretprobe_trampoline from _raw_spin_lock_irqsave => DEADLOCK kretprobe_trampoline() -> trampoline_handler() -> recycle_rp_inst() -> raw_spin_lock() -> _raw_spin_lock() kretprobe_trampoline from _raw_spin_lock => DEADLOCK Use the corresponding inlined spinlock functions to prevent this recursion. Signed-off-by: Andrea Righi <righi.andrea@gmail.com> --- kernel/kprobes.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel/kprobes.c b/kernel/kprobes.c index f4ddfdd2d07e..b89bef5e3d80 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -1154,9 +1154,9 @@ void recycle_rp_inst(struct kretprobe_instance *ri, hlist_del(&ri->hlist); INIT_HLIST_NODE(&ri->hlist); if (likely(rp)) { - raw_spin_lock(&rp->lock); + __raw_spin_lock(&rp->lock); hlist_add_head(&ri->hlist, &rp->free_instances); - raw_spin_unlock(&rp->lock); + __raw_spin_unlock(&rp->lock); } else /* Unregistering */ hlist_add_head(&ri->hlist, head); @@ -1172,7 +1172,7 @@ __acquires(hlist_lock) *head = &kretprobe_inst_table[hash]; hlist_lock = kretprobe_table_lock_ptr(hash); - raw_spin_lock_irqsave(hlist_lock, *flags); + *flags = __raw_spin_lock_irqsave(hlist_lock); } NOKPROBE_SYMBOL(kretprobe_hash_lock); @@ -1193,7 +1193,7 @@ __releases(hlist_lock) raw_spinlock_t *hlist_lock; hlist_lock = kretprobe_table_lock_ptr(hash); - raw_spin_unlock_irqrestore(hlist_lock, *flags); + __raw_spin_unlock_irqrestore(hlist_lock, *flags); } NOKPROBE_SYMBOL(kretprobe_hash_unlock); -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem 2019-01-07 21:19 ` Andrea Righi @ 2019-01-07 21:28 ` Steven Rostedt 2019-01-07 21:34 ` Andrea Righi 0 siblings, 1 reply; 18+ messages in thread From: Steven Rostedt @ 2019-01-07 21:28 UTC (permalink / raw) To: Andrea Righi Cc: Masami Hiramatsu, Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel On Mon, 7 Jan 2019 22:19:04 +0100 Andrea Righi <righi.andrea@gmail.com> wrote: > > > If we put a kretprobe to raw_spin_lock_irqsave() it looks like > > > kretprobe is going to call kretprobe... > > > > Right, but we should be able to add some recursion protection to stop > > that. I have similar protection in the ftrace code. > > If we assume that __raw_spin_lock/unlock*() are always inlined a I wouldn't assume that. > possible way to prevent this recursion could be to use directly those > functions to do locking from the kretprobe trampoline. > > But I'm not sure if that's a safe assumption... if not I'll see if I can > find a better solution. All you need to do is have a per_cpu variable, where you just do: preempt_disable_notrace(); if (this_cpu_read(kprobe_recursion)) goto out; this_cpu_inc(kprobe_recursion); [...] this_cpu_dec(kprobe_recursion); out: preempt_enable_notrace(); And then just ignore any kprobes that trigger while you are processing the current kprobe. Something like that. If you want (or if it already happens) replace preempt_disable() with local_irq_save(). -- Steve > > Thanks, > > From: Andrea Righi <righi.andrea@gmail.com> > Subject: [PATCH] kprobes: prevent recursion deadlock with kretprobe and > spinlocks > > kretprobe_trampoline() is using a spinlock to protect the hash of > kretprobes. Adding a kretprobe to the spinlock functions may cause > a recursion deadlock where kretprobe is calling itself: > > kretprobe_trampoline() > -> trampoline_handler() > -> kretprobe_hash_lock() > -> raw_spin_lock_irqsave() > -> _raw_spin_lock_irqsave() > kretprobe_trampoline from _raw_spin_lock_irqsave => DEADLOCK > > kretprobe_trampoline() > -> trampoline_handler() > -> recycle_rp_inst() > -> raw_spin_lock() > -> _raw_spin_lock() > kretprobe_trampoline from _raw_spin_lock => DEADLOCK > > Use the corresponding inlined spinlock functions to prevent this > recursion. > > Signed-off-by: Andrea Righi <righi.andrea@gmail.com> > --- > kernel/kprobes.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index f4ddfdd2d07e..b89bef5e3d80 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -1154,9 +1154,9 @@ void recycle_rp_inst(struct kretprobe_instance *ri, > hlist_del(&ri->hlist); > INIT_HLIST_NODE(&ri->hlist); > if (likely(rp)) { > - raw_spin_lock(&rp->lock); > + __raw_spin_lock(&rp->lock); > hlist_add_head(&ri->hlist, &rp->free_instances); > - raw_spin_unlock(&rp->lock); > + __raw_spin_unlock(&rp->lock); > } else > /* Unregistering */ > hlist_add_head(&ri->hlist, head); > @@ -1172,7 +1172,7 @@ __acquires(hlist_lock) > > *head = &kretprobe_inst_table[hash]; > hlist_lock = kretprobe_table_lock_ptr(hash); > - raw_spin_lock_irqsave(hlist_lock, *flags); > + *flags = __raw_spin_lock_irqsave(hlist_lock); > } > NOKPROBE_SYMBOL(kretprobe_hash_lock); > > @@ -1193,7 +1193,7 @@ __releases(hlist_lock) > raw_spinlock_t *hlist_lock; > > hlist_lock = kretprobe_table_lock_ptr(hash); > - raw_spin_unlock_irqrestore(hlist_lock, *flags); > + __raw_spin_unlock_irqrestore(hlist_lock, *flags); > } > NOKPROBE_SYMBOL(kretprobe_hash_unlock); > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem 2019-01-07 21:28 ` Steven Rostedt @ 2019-01-07 21:34 ` Andrea Righi 2019-01-08 2:56 ` Masami Hiramatsu 0 siblings, 1 reply; 18+ messages in thread From: Andrea Righi @ 2019-01-07 21:34 UTC (permalink / raw) To: Steven Rostedt Cc: Masami Hiramatsu, Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel On Mon, Jan 07, 2019 at 04:28:33PM -0500, Steven Rostedt wrote: > On Mon, 7 Jan 2019 22:19:04 +0100 > Andrea Righi <righi.andrea@gmail.com> wrote: > > > > > If we put a kretprobe to raw_spin_lock_irqsave() it looks like > > > > kretprobe is going to call kretprobe... > > > > > > Right, but we should be able to add some recursion protection to stop > > > that. I have similar protection in the ftrace code. > > > > If we assume that __raw_spin_lock/unlock*() are always inlined a > > I wouldn't assume that. > > > possible way to prevent this recursion could be to use directly those > > functions to do locking from the kretprobe trampoline. > > > > But I'm not sure if that's a safe assumption... if not I'll see if I can > > find a better solution. > > All you need to do is have a per_cpu variable, where you just do: > > preempt_disable_notrace(); > if (this_cpu_read(kprobe_recursion)) > goto out; > this_cpu_inc(kprobe_recursion); > [...] > this_cpu_dec(kprobe_recursion); > out: > preempt_enable_notrace(); > > And then just ignore any kprobes that trigger while you are processing > the current kprobe. > > Something like that. If you want (or if it already happens) replace > preempt_disable() with local_irq_save(). Oh.. definitely much better. I'll work on that and send a new patch. Thanks for the suggestion! -Andrea ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem 2019-01-07 21:34 ` Andrea Righi @ 2019-01-08 2:56 ` Masami Hiramatsu 0 siblings, 0 replies; 18+ messages in thread From: Masami Hiramatsu @ 2019-01-08 2:56 UTC (permalink / raw) To: Andrea Righi Cc: Steven Rostedt, Masami Hiramatsu, Ingo Molnar, peterz, Mathieu Desnoyers, linux-kernel Hi Andrea and Steve, On Mon, 7 Jan 2019 22:34:39 +0100 Andrea Righi <righi.andrea@gmail.com> wrote: > On Mon, Jan 07, 2019 at 04:28:33PM -0500, Steven Rostedt wrote: > > On Mon, 7 Jan 2019 22:19:04 +0100 > > Andrea Righi <righi.andrea@gmail.com> wrote: > > > > > > > If we put a kretprobe to raw_spin_lock_irqsave() it looks like > > > > > kretprobe is going to call kretprobe... > > > > > > > > Right, but we should be able to add some recursion protection to stop > > > > that. I have similar protection in the ftrace code. > > > > > > If we assume that __raw_spin_lock/unlock*() are always inlined a > > > > I wouldn't assume that. > > > > > possible way to prevent this recursion could be to use directly those > > > functions to do locking from the kretprobe trampoline. > > > > > > But I'm not sure if that's a safe assumption... if not I'll see if I can > > > find a better solution. > > > > All you need to do is have a per_cpu variable, where you just do: > > > > preempt_disable_notrace(); > > if (this_cpu_read(kprobe_recursion)) > > goto out; > > this_cpu_inc(kprobe_recursion); > > [...] > > this_cpu_dec(kprobe_recursion); > > out: > > preempt_enable_notrace(); > > > > And then just ignore any kprobes that trigger while you are processing > > the current kprobe. > > > > Something like that. If you want (or if it already happens) replace > > preempt_disable() with local_irq_save(). > > Oh.. definitely much better. I'll work on that and send a new patch. > Thanks for the suggestion! Thank you for pointing it out, Since we already have current_kprobe per_cpu, it can be done by setting up a dummy kprobe on it. I'll add that in v2 series. Actually, this bug has been introduced a long time ago by me... when I introduced asm-coded kretprobe-trampoline. Before that, kretprobe trampoline handler uses a kprobe to hook it, so the 2nd kretprobe must be skipped automatically. Thank you, -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-01-08 2:56 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-07 13:31 [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem Masami Hiramatsu 2019-01-07 13:32 ` [PATCH 1/2] x86/kprobes: Verify stack frame on kretprobe Masami Hiramatsu 2019-01-07 13:32 ` [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe Masami Hiramatsu 2019-01-07 14:55 ` Andrea Righi 2019-01-07 17:29 ` Steven Rostedt 2019-01-08 2:41 ` Masami Hiramatsu 2019-01-08 2:40 ` Masami Hiramatsu 2019-01-07 17:23 ` kbuild test robot 2019-01-07 17:38 ` kbuild test robot 2019-01-07 17:28 ` [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem Andrea Righi 2019-01-07 18:34 ` Andrea Righi 2019-01-07 19:27 ` Steven Rostedt 2019-01-07 19:52 ` Andrea Righi 2019-01-07 19:59 ` Steven Rostedt 2019-01-07 21:19 ` Andrea Righi 2019-01-07 21:28 ` Steven Rostedt 2019-01-07 21:34 ` Andrea Righi 2019-01-08 2:56 ` Masami Hiramatsu
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).