LinuxLists.cc - [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[permalink] [raw]

Subject: Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

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 <[email protected]>
> Reported-by: Francis Deslauriers <[email protected]>
> ---
> 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

2019-01-07 16:31:06

[permalink] [raw]

Subject: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

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 <[email protected]>
Reported-by: Francis Deslauriers <[email protected]>
---
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

2019-01-07 17:28:08

[permalink] [raw]

Subject: [PATCH 1/2] x86/kprobes: Verify stack frame on kretprobe

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 <[email protected]>
---
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 = &regs->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 = &regs->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];
};

2019-01-07 18:08:36

[permalink] [raw]

Subject: Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

2019-01-07 18:09:30

[permalink] [raw]

Subject: Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

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

Thanks!
-Andrea

2019-01-07 18:37:40

[permalink] [raw]

Subject: Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

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

2019-01-07 19:30:39

[permalink] [raw]

Subject: Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

On Mon, 7 Jan 2019 19:34:44 +0100
Andrea Righi <[email protected]> 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

2019-01-07 19:55:38

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

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 <[email protected]>
:::::: CC: Steven Rostedt <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (8.21 kB)
.config.gz (32.17 kB)
Download all attachments

2019-01-07 19:55:49

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

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 <[email protected]>
:::::: CC: Steven Rostedt <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (8.26 kB)
.config.gz (30.12 kB)
Download all attachments

2019-01-07 20:02:42

[permalink] [raw]

Subject: Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

On Mon, 7 Jan 2019 20:52:09 +0100
Andrea Righi <[email protected]> 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

2019-01-07 20:12:56

[permalink] [raw]

Subject: Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

On Mon, Jan 07, 2019 at 02:27:49PM -0500, Steven Rostedt wrote:
> On Mon, 7 Jan 2019 19:34:44 +0100
> Andrea Righi <[email protected]> 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

2019-01-07 21:21:47

[permalink] [raw]

Subject: Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

On Mon, Jan 07, 2019 at 02:59:18PM -0500, Steven Rostedt wrote:
> On Mon, 7 Jan 2019 20:52:09 +0100
> Andrea Righi <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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

2019-01-07 21:31:09

[permalink] [raw]

Subject: Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

On Mon, 7 Jan 2019 22:19:04 +0100
Andrea Righi <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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);
>

2019-01-07 21:36:33

[permalink] [raw]

Subject: Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

On Mon, Jan 07, 2019 at 04:28:33PM -0500, Steven Rostedt wrote:
> On Mon, 7 Jan 2019 22:19:04 +0100
> Andrea Righi <[email protected]> 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

2019-01-08 02:43:27

[permalink] [raw]

Subject: Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

On Mon, 7 Jan 2019 15:55:36 +0100
Andrea Righi <[email protected]> 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 <[email protected]>
> > Reported-by: Francis Deslauriers <[email protected]>
> > ---
> > 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 $B!F(B=$B!G(B, $B!F(B,$B!G(B, $B!F(B;$B!G(B, $B!F(Basm$B!G(B or $B!F(B__attribute__$B!G(B before $B!F(Bvoid$B!G(B
> 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 <[email protected]>

2019-01-08 02:44:14

[permalink] [raw]

Subject: Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

On Mon, 7 Jan 2019 12:29:51 -0500
Steven Rostedt <[email protected]> wrote:

> On Mon, 7 Jan 2019 15:55:36 +0100
> Andrea Righi <[email protected]> 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 <[email protected]>
> > > Reported-by: Francis Deslauriers <[email protected]>
> > > ---
> > > 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 $B!F(B=$B!G(B, $B!F(B,$B!G(B, $B!F(B;$B!G(B, $B!F(Basm$B!G(B or $B!F(B__attribute__$B!G(B before $B!F(Bvoid$B!G(B
> > 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 <[email protected]>

2019-01-08 02:58:31