On Tue, 8 Jan 2019 13:45:22 +0900
Masami Hiramatsu <mhiramat@xxxxxxxxxx> 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 <mhiramat@xxxxxxxxxx>
> Reported-by: Andrea Righi <righi.andrea@xxxxxxxxx>
> Fixes: c9becf58d935 ("[PATCH] kretprobe: kretprobe-booster")
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> 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);
