On Fri, 12 Mar 2021 15:42:28 +0900
Masami Hiramatsu wrote:
> Recover the return address on the stack which changed by the
> kretprobe. Note that this does not recover the address on the
> !current stack trace if CONFIG_ARCH_STACKWALK=n because old
> stack trace interface doesn't lock the stack in the generic
> stack_trace_save*() functions.
I found that v2 didn't work correctly with FP unwinder,
because this changes the unlink timing.
With frame pointer, the unwinder skips the first (on-going)
kretprobe_trampoline because kretprobe_trampoline doesn't
setup the frame pointer (push bp; mov sp, bp).
If there are 2 or more kretprobes on the stack, when the last
kretprobe_trampoline is running, the unwinder finds the 2nd
kretprobe_trampoline on the unwinding call stack at first.
However, while the user kretprobe handler is running, the last
real return address is still linked to the current->kretprobe_instances.
Thus, this will decode the 2nd kretprobe_trampoline with the
last real return address.
If the kretprobe_trampoline sets up the frame pointer at the entry
this can be avoided. However, that helps only x86.
Refering kretprobe_instance.fp (which should point the address of
replaced stack entry) to find correct return address seems better
solution, but this is implemented on the arch on which "call"
instruction stores the return address on the stack. E.g. arm and
some other RISCs stores the return address to the link register,
which has no "address".
So I would like to drop this arch-independent recovery routine.
Instead, it should be fixed per-arch basis.
Thank you,
>
> So with this patch, ftrace correctly shows the stacktrace
> as below;
>
> # echo r vfs_read > kprobe_events
> # echo stacktrace > events/kprobes/r_vfs_read_0/trigger
> # echo 1 > events/kprobes/r_vfs_read_0/enable
> # echo 1 > options/sym-offset
> # less trace
> ...
>
> sh-132 [007] ...122.524917:
> => kretprobe_dispatcher+0x7d/0xc0
> => __kretprobe_trampoline_handler+0xdb/0x1b0
> => trampoline_handler+0x48/0x60
> => kretprobe_trampoline+0x2a/0x50
> => ksys_read+0x70/0xf0
> => __x64_sys_read+0x1a/0x20
> => do_syscall_64+0x38/0x50
> => entry_SYSCALL_64_after_hwframe+0x44/0xae
> => 0
>
> The trampoline_handler+0x48 is actual call site address,
> not modified by kretprobe.
>
> Reported-by: Daniel Xu
> Signed-off-by: Masami Hiramatsu
> ---
> Changes in v2:
> - Add is_kretprobe_trampoline() for checking address outside of
> kretprobe_find_ret_addr()
> - Remove unneeded addr from kretprobe_find_ret_addr()
> - Rename fixup_kretprobe_tramp_addr() to fixup_kretprobe_trampoline()
> ---
> include/linux/kprobes.h | 22 ++
> kernel/kprobes.c| 73
> ++-
> kernel/stacktrace.c | 22 ++
> 3 files changed, 91 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 65dadd4238a2..674b5adad281 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -215,6 +215,14 @@ static nokprobe_inline void
> *kretprobe_trampoline_addr(void)
> return dereference_function_descriptor(kretprobe_trampoline);
> }
>
> +static nokprobe_inline bool is_kretprobe_trampoline(unsigned long addr)
> +{
> + return (void *)addr == kretprobe_trampoline_addr();
> +}
> +
> +unsigned long kretprobe_find_ret_addr(struct task_struct *tsk,
> + struct llist_node **cur);
> +
> /* If the trampoline handler called from a kprobe, use this version */
> unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
>void *frame_pointer);
> @@ -514,6 +522,20 @@ static inline bool is_kprobe_optinsn_slot(unsigned long
> addr)
> }
> #endif
>
> +#if !defined(CONFIG_KRETPROBES)
> +static nokprobe_inline bool is_kretprobe_trampoline(unsigned long addr)
> +{
> + return false;
> +}
> +
> +static nokprobe_inline
> +unsigned long kretprobe_find_ret_addr(struct task_struct *tsk,
> + struct llist_node **cur)
> +{
> + return 0;
> +}
> +#endif
> +
> /* Returns true if kprobes handled the fault */
> static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs,
> unsigned int trap)
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 75c0a58c19c2..2550521ff64d 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1858,45 +1858,51 @@ static struct notifier_block kprobe_exceptions_nb = {
>
> #ifdef CONFIG_KRETPROBES
>
> -unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
> - void *frame_pointer)
> +/* This assumes the tsk is current or the task which is not running. */
> +unsigned long kretprobe_find_ret_addr(struct task_struct *tsk,
> + struct llist_node **cur)
> {
> - kprobe_opcode_t *correct_re
