Hello,
This is v2 series of fixing kretprobe incorrect stacking order patches.
In this version, I fixed a lack of kprobes.h including and added new
patch for kretprobe trampoline recursion issue. (and add Cc:stable)
(1) kprobe incorrct stacking order problem
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 ).
See the investigation details in
https://lkml.kernel.org/r/154686789378.15479.2886543882215785247.stgit@devbox
When we put a kretprobe on ftrace_ops_assist_func() and put another
kretprobe on probed-function, 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.
(2) kretprobe trampoline recursion problem
This was found by Andrea in the previous thread
https://lkml.kernel.org/r/20190107183444.GA5966@xps-13
----
echo "r:event_1 __fdget" >> kprobe_events
echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
echo 1 > events/kprobes/enable
[DEADLOCK]
----
Because kretprobe trampoline_handler uses spinlock for protecting
hash table, if we probe the spinlock itself, it causes deadlock.
Thank you Andrea and Steve for discovering this root cause!!
This bug has been introduced with the asm-coded trampoline
code, since previously it used another kprobe for hooking
the function return placeholder (which only has a nop) and
trampoline handler was called from that kprobe.
To fix this bug, I introduced a dummy kprobe and set it in
current_kprobe as we did in old days.
Thank you,
---
Masami Hiramatsu (3):
x86/kprobes: Verify stack frame on kretprobe
kprobes: Mark ftrace mcount handler functions nokprobe
x86/kprobes: Fix to avoid kretprobe recursion
arch/x86/kernel/kprobes/core.c | 48 ++++++++++++++++++++++++++++++++++++++--
include/linux/kprobes.h | 1 +
kernel/trace/ftrace.c | 6 ++++-
3 files changed, 52 insertions(+), 3 deletions(-)
--
Masami Hiramatsu (Linaro) <[email protected]>
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]>
Tested-by: Andrea Righi <[email protected]>
Cc: [email protected]
---
- Changes in v2:
Fix to include kprobes.h (Thanks Andrea!)
---
kernel/trace/ftrace.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f0ff24173a0b..b0774388d52b 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -34,6 +34,7 @@
#include <linux/list.h>
#include <linux/hash.h>
#include <linux/rcupdate.h>
+#include <linux/kprobes.h>
#include <trace/events/sched.h>
@@ -6250,7 +6251,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 +6311,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 +6344,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
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]>
Tested-by: Andrea Righi <[email protected]>
Cc: [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 = ®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];
};
Fix to avoid kretprobe recursion loop by setting a dummy
kprobes to current_kprobe per-cpu variable.
This bug has been introduced with the asm-coded trampoline
code, since previously it used another kprobe for hooking
the function return placeholder (which only has a nop) and
trampoline handler was called from that kprobe.
This revives the old lost kprobe again.
With this fix, we don't see deadlock anymore.
# echo "r:event_1 __fdget" >> kprobe_events
# echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
# echo 1 > events/kprobes/enable
And you can see that all inner-called kretprobe are skipped.
# cat kprobe_profile
event_1 235 0
event_2 19375 19612
The 1st column is recorded count and the 2nd is missed count.
Above shows (event_1 rec) + (event_2 rec) ~= (event_2 missed)
Signed-off-by: Masami Hiramatsu <[email protected]>
Reported-by: Andrea Righi <[email protected]>
Fixes: c9becf58d935 ("[PATCH] kretprobe: kretprobe-booster")
Cc: [email protected]
---
arch/x86/kernel/kprobes/core.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 69b6400d1ce2..f4b954ff5b89 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -749,11 +749,16 @@ asm(
NOKPROBE_SYMBOL(kretprobe_trampoline);
STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
+static struct kprobe kretprobe_kprobe = {
+ .addr = (void *)kretprobe_trampoline,
+};
+
/*
* Called from kretprobe_trampoline
*/
static __used void *trampoline_handler(struct pt_regs *regs)
{
+ struct kprobe_ctlblk *kcb;
struct kretprobe_instance *ri = NULL;
struct hlist_head *head, empty_rp;
struct hlist_node *tmp;
@@ -763,6 +768,17 @@ static __used void *trampoline_handler(struct pt_regs *regs)
void *frame_pointer;
bool skipped = false;
+ preempt_disable();
+
+ /*
+ * Set a dummy kprobe for avoiding kretprobe recursion.
+ * Since kretprobe never run in kprobe handler, kprobe must not
+ * be running at this point.
+ */
+ kcb = get_kprobe_ctlblk();
+ __this_cpu_write(current_kprobe, &kretprobe_kprobe);
+ kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+
INIT_HLIST_HEAD(&empty_rp);
kretprobe_hash_lock(current, &head, &flags);
/* fixup registers */
@@ -838,10 +854,9 @@ static __used void *trampoline_handler(struct pt_regs *regs)
orig_ret_address = (unsigned long)ri->ret_addr;
if (ri->rp && ri->rp->handler) {
__this_cpu_write(current_kprobe, &ri->rp->kp);
- get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
ri->ret_addr = correct_ret_addr;
ri->rp->handler(ri, regs);
- __this_cpu_write(current_kprobe, NULL);
+ __this_cpu_write(current_kprobe, &kretprobe_kprobe);
}
recycle_rp_inst(ri, &empty_rp);
@@ -857,6 +872,9 @@ static __used void *trampoline_handler(struct pt_regs *regs)
kretprobe_hash_unlock(current, &flags);
+ __this_cpu_write(current_kprobe, NULL);
+ preempt_enable();
+
hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
hlist_del(&ri->hlist);
kfree(ri);
On Tue, Jan 08, 2019 at 01:43:55PM +0900, Masami Hiramatsu wrote:
> Hello,
>
> This is v2 series of fixing kretprobe incorrect stacking order patches.
> In this version, I fixed a lack of kprobes.h including and added new
> patch for kretprobe trampoline recursion issue. (and add Cc:stable)
>
> (1) kprobe incorrct stacking order problem
>
> 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 ).
>
> See the investigation details in
> https://lkml.kernel.org/r/154686789378.15479.2886543882215785247.stgit@devbox
>
> When we put a kretprobe on ftrace_ops_assist_func() and put another
> kretprobe on probed-function, 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.
>
> (2) kretprobe trampoline recursion problem
>
> This was found by Andrea in the previous thread
> https://lkml.kernel.org/r/20190107183444.GA5966@xps-13
>
> ----
> echo "r:event_1 __fdget" >> kprobe_events
> echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
> echo 1 > events/kprobes/enable
> [DEADLOCK]
> ----
>
> Because kretprobe trampoline_handler uses spinlock for protecting
> hash table, if we probe the spinlock itself, it causes deadlock.
> Thank you Andrea and Steve for discovering this root cause!!
>
> This bug has been introduced with the asm-coded trampoline
> code, since previously it used another kprobe for hooking
> the function return placeholder (which only has a nop) and
> trampoline handler was called from that kprobe.
>
> To fix this bug, I introduced a dummy kprobe and set it in
> current_kprobe as we did in old days.
>
> Thank you,
It looks all good to me, with this patch set I couldn't break the
kernel in any way.
Tested-by: Andrea Righi <[email protected]>
Thanks,
-Andrea
On Tue, 8 Jan 2019 11:31:01 +0100
Andrea Righi <[email protected]> wrote:
> On Tue, Jan 08, 2019 at 01:43:55PM +0900, Masami Hiramatsu wrote:
> > Hello,
> >
> > This is v2 series of fixing kretprobe incorrect stacking order patches.
> > In this version, I fixed a lack of kprobes.h including and added new
> > patch for kretprobe trampoline recursion issue. (and add Cc:stable)
> >
> > (1) kprobe incorrct stacking order problem
> >
> > 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 ).
> >
> > See the investigation details in
> > https://lkml.kernel.org/r/154686789378.15479.2886543882215785247.stgit@devbox
> >
> > When we put a kretprobe on ftrace_ops_assist_func() and put another
> > kretprobe on probed-function, 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.
> >
> > (2) kretprobe trampoline recursion problem
> >
> > This was found by Andrea in the previous thread
> > https://lkml.kernel.org/r/20190107183444.GA5966@xps-13
> >
> > ----
> > echo "r:event_1 __fdget" >> kprobe_events
> > echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
> > echo 1 > events/kprobes/enable
> > [DEADLOCK]
> > ----
> >
> > Because kretprobe trampoline_handler uses spinlock for protecting
> > hash table, if we probe the spinlock itself, it causes deadlock.
> > Thank you Andrea and Steve for discovering this root cause!!
> >
> > This bug has been introduced with the asm-coded trampoline
> > code, since previously it used another kprobe for hooking
> > the function return placeholder (which only has a nop) and
> > trampoline handler was called from that kprobe.
> >
> > To fix this bug, I introduced a dummy kprobe and set it in
> > current_kprobe as we did in old days.
> >
> > Thank you,
>
> It looks all good to me, with this patch set I couldn't break the
> kernel in any way.
>
> Tested-by: Andrea Righi <[email protected]>
Thank you, Andrea!
Ingo, could you pick this series?
--
Masami Hiramatsu <[email protected]>
On Tue, 8 Jan 2019 13:44:54 +0900
Masami Hiramatsu <[email protected]> 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]>
> Tested-by: Andrea Righi <[email protected]>
> Cc: [email protected]
Acked-by: Steven Rostedt (VMware) <[email protected]>
-- Steve
> ---
> - Changes in v2:
> Fix to include kprobes.h (Thanks Andrea!)
> ---
> kernel/trace/ftrace.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f0ff24173a0b..b0774388d52b 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -34,6 +34,7 @@
> #include <linux/list.h>
> #include <linux/hash.h>
> #include <linux/rcupdate.h>
> +#include <linux/kprobes.h>
>
> #include <trace/events/sched.h>
>
> @@ -6250,7 +6251,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 +6311,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 +6344,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
On Tue, 8 Jan 2019 13:45:22 +0900
Masami Hiramatsu <[email protected]> wrote:
> Fix to avoid kretprobe recursion loop by setting a dummy
> kprobes to current_kprobe per-cpu variable.
>
> This bug has been introduced with the asm-coded trampoline
> code, since previously it used another kprobe for hooking
> the function return placeholder (which only has a nop) and
> trampoline handler was called from that kprobe.
>
> This revives the old lost kprobe again.
>
> With this fix, we don't see deadlock anymore.
>
> # echo "r:event_1 __fdget" >> kprobe_events
> # echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
> # echo 1 > events/kprobes/enable
>
> And you can see that all inner-called kretprobe are skipped.
>
> # cat kprobe_profile
> event_1 235 0
> event_2 19375 19612
>
> The 1st column is recorded count and the 2nd is missed count.
> Above shows (event_1 rec) + (event_2 rec) ~= (event_2 missed)
I don't quite understand the above. Is the miss count because we missed
event_2 events for both event_1 and event_2?
trace raw_spin_lock()
handler calls raw_spin_lock()
trace raw_spin_lock() [ skip ]
I'm also guessing that the 2 extra (19612 - (235 + 19375) = 2) are
possibly due to the displaying being racy?
>
> Signed-off-by: Masami Hiramatsu <[email protected]>
> Reported-by: Andrea Righi <[email protected]>
> Fixes: c9becf58d935 ("[PATCH] kretprobe: kretprobe-booster")
> Cc: [email protected]
> ---
> arch/x86/kernel/kprobes/core.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 69b6400d1ce2..f4b954ff5b89 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -749,11 +749,16 @@ asm(
> NOKPROBE_SYMBOL(kretprobe_trampoline);
> STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
>
> +static struct kprobe kretprobe_kprobe = {
> + .addr = (void *)kretprobe_trampoline,
> +};
> +
> /*
> * Called from kretprobe_trampoline
> */
> static __used void *trampoline_handler(struct pt_regs *regs)
> {
> + struct kprobe_ctlblk *kcb;
> struct kretprobe_instance *ri = NULL;
> struct hlist_head *head, empty_rp;
> struct hlist_node *tmp;
> @@ -763,6 +768,17 @@ static __used void *trampoline_handler(struct pt_regs *regs)
> void *frame_pointer;
> bool skipped = false;
>
> + preempt_disable();
> +
> + /*
> + * Set a dummy kprobe for avoiding kretprobe recursion.
> + * Since kretprobe never run in kprobe handler, kprobe must not
> + * be running at this point.
> + */
> + kcb = get_kprobe_ctlblk();
> + __this_cpu_write(current_kprobe, &kretprobe_kprobe);
If an interrupt comes in here, is this still safe, if the interrupt
handler has a kretprobe too?
-- Steve
> + kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> +
> INIT_HLIST_HEAD(&empty_rp);
> kretprobe_hash_lock(current, &head, &flags);
> /* fixup registers */
> @@ -838,10 +854,9 @@ static __used void *trampoline_handler(struct pt_regs *regs)
> orig_ret_address = (unsigned long)ri->ret_addr;
> if (ri->rp && ri->rp->handler) {
> __this_cpu_write(current_kprobe, &ri->rp->kp);
> - get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
> ri->ret_addr = correct_ret_addr;
> ri->rp->handler(ri, regs);
> - __this_cpu_write(current_kprobe, NULL);
> + __this_cpu_write(current_kprobe, &kretprobe_kprobe);
> }
>
> recycle_rp_inst(ri, &empty_rp);
> @@ -857,6 +872,9 @@ static __used void *trampoline_handler(struct pt_regs *regs)
>
> kretprobe_hash_unlock(current, &flags);
>
> + __this_cpu_write(current_kprobe, NULL);
> + preempt_enable();
> +
> hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
> hlist_del(&ri->hlist);
> kfree(ri);
On Wed, 9 Jan 2019 11:08:16 -0500
Steven Rostedt <[email protected]> wrote:
> On Tue, 8 Jan 2019 13:45:22 +0900
> Masami Hiramatsu <[email protected]> wrote:
>
> > Fix to avoid kretprobe recursion loop by setting a dummy
> > kprobes to current_kprobe per-cpu variable.
> >
> > This bug has been introduced with the asm-coded trampoline
> > code, since previously it used another kprobe for hooking
> > the function return placeholder (which only has a nop) and
> > trampoline handler was called from that kprobe.
> >
> > This revives the old lost kprobe again.
> >
> > With this fix, we don't see deadlock anymore.
> >
> > # echo "r:event_1 __fdget" >> kprobe_events
> > # echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
> > # echo 1 > events/kprobes/enable
> >
> > And you can see that all inner-called kretprobe are skipped.
> >
> > # cat kprobe_profile
> > event_1 235 0
> > event_2 19375 19612
> >
> > The 1st column is recorded count and the 2nd is missed count.
> > Above shows (event_1 rec) + (event_2 rec) ~= (event_2 missed)
>
> I don't quite understand the above. Is the miss count because we missed
> event_2 events for both event_1 and event_2?
>
> trace raw_spin_lock()
> handler calls raw_spin_lock()
> trace raw_spin_lock() [ skip ]
Yes, both events(kretprobe) eventually hits event_2 in trampoline_handler()'s
spin_lock_irqsave().
>
> I'm also guessing that the 2 extra (19612 - (235 + 19375) = 2) are
> possibly due to the displaying being racy?
Yes, it can be racy.
Thank you,
>
> >
> > Signed-off-by: Masami Hiramatsu <[email protected]>
> > Reported-by: Andrea Righi <[email protected]>
> > Fixes: c9becf58d935 ("[PATCH] kretprobe: kretprobe-booster")
> > Cc: [email protected]
> > ---
> > arch/x86/kernel/kprobes/core.c | 22 ++++++++++++++++++++--
> > 1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index 69b6400d1ce2..f4b954ff5b89 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -749,11 +749,16 @@ asm(
> > NOKPROBE_SYMBOL(kretprobe_trampoline);
> > STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
> >
> > +static struct kprobe kretprobe_kprobe = {
> > + .addr = (void *)kretprobe_trampoline,
> > +};
> > +
> > /*
> > * Called from kretprobe_trampoline
> > */
> > static __used void *trampoline_handler(struct pt_regs *regs)
> > {
> > + struct kprobe_ctlblk *kcb;
> > struct kretprobe_instance *ri = NULL;
> > struct hlist_head *head, empty_rp;
> > struct hlist_node *tmp;
> > @@ -763,6 +768,17 @@ static __used void *trampoline_handler(struct pt_regs *regs)
> > void *frame_pointer;
> > bool skipped = false;
> >
> > + preempt_disable();
> > +
> > + /*
> > + * Set a dummy kprobe for avoiding kretprobe recursion.
> > + * Since kretprobe never run in kprobe handler, kprobe must not
> > + * be running at this point.
> > + */
> > + kcb = get_kprobe_ctlblk();
> > + __this_cpu_write(current_kprobe, &kretprobe_kprobe);
>
> If an interrupt comes in here, is this still safe, if the interrupt
> handler has a kretprobe too?
>
> -- Steve
>
> > + kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > +
> > INIT_HLIST_HEAD(&empty_rp);
> > kretprobe_hash_lock(current, &head, &flags);
> > /* fixup registers */
> > @@ -838,10 +854,9 @@ static __used void *trampoline_handler(struct pt_regs *regs)
> > orig_ret_address = (unsigned long)ri->ret_addr;
> > if (ri->rp && ri->rp->handler) {
> > __this_cpu_write(current_kprobe, &ri->rp->kp);
> > - get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
> > ri->ret_addr = correct_ret_addr;
> > ri->rp->handler(ri, regs);
> > - __this_cpu_write(current_kprobe, NULL);
> > + __this_cpu_write(current_kprobe, &kretprobe_kprobe);
> > }
> >
> > recycle_rp_inst(ri, &empty_rp);
> > @@ -857,6 +872,9 @@ static __used void *trampoline_handler(struct pt_regs *regs)
> >
> > kretprobe_hash_unlock(current, &flags);
> >
> > + __this_cpu_write(current_kprobe, NULL);
> > + preempt_enable();
> > +
> > hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
> > hlist_del(&ri->hlist);
> > kfree(ri);
>
--
Masami Hiramatsu <[email protected]>