Hi Abhishek,
On Mon, Jun 10, 2024 at 11:45:09AM GMT, Abhishek Dubey wrote:
> This is an adaptation of commit f3a112c0c40d ("x86,rethook,kprobes:
> Replace kretprobe with rethook on x86") to PowerPC.
>
> Replaces the kretprobe code with rethook on Power. With this patch,
> kretprobe on Power uses the rethook instead of kretprobe specific
> trampoline code.
>
> Reference to other archs:
> commit b57c2f124098 ("riscv: add riscv rethook implementation")
> commit 7b0a096436c2 ("LoongArch: Replace kretprobe with rethook")
>
> Signed-off-by: Abhishek Dubey <adu...@linux.ibm.com>
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/kernel/Makefile | 1 +
> arch/powerpc/kernel/kprobes.c | 65 +----------------------------
> arch/powerpc/kernel/optprobes.c | 2 +-
> arch/powerpc/kernel/rethook.c | 71 ++++++++++++++++++++++++++++++++
> arch/powerpc/kernel/stacktrace.c | 10 +++--
> 6 files changed, 81 insertions(+), 69 deletions(-)
> create mode 100644 arch/powerpc/kernel/rethook.c
Thanks for implementing this - it is looking good, but please find a few
small suggestions below.
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index c88c6d46a5bc..fa0b1ab3f935 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -270,6 +270,7 @@ config PPC
> select HAVE_PERF_EVENTS_NMI if PPC64
> select HAVE_PERF_REGS
> select HAVE_PERF_USER_STACK_DUMP
> + select HAVE_RETHOOK
> select HAVE_REGS_AND_STACK_ACCESS_API
> select HAVE_RELIABLE_STACKTRACE
> select HAVE_RSEQ
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 8585d03c02d3..7dd1b523b17f 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -140,6 +140,7 @@ obj-$(CONFIG_KPROBES) += kprobes.o
> obj-$(CONFIG_OPTPROBES) += optprobes.o optprobes_head.o
> obj-$(CONFIG_KPROBES_ON_FTRACE) += kprobes-ftrace.o
> obj-$(CONFIG_UPROBES) += uprobes.o
> +obj-$(CONFIG_RETHOOK) += rethook.o
> obj-$(CONFIG_PPC_UDBG_16550) += legacy_serial.o udbg_16550.o
> obj-$(CONFIG_SWIOTLB) += dma-swiotlb.o
> obj-$(CONFIG_ARCH_HAS_DMA_SET_MASK) += dma-mask.o
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 14c5ddec3056..f8aa91bc3b17 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -228,16 +228,6 @@ static nokprobe_inline void set_current_kprobe(struct
> kprobe *p, struct pt_regs
> kcb->kprobe_saved_msr = regs->msr;
> }
>
> -void arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs
> *regs)
> -{
> - ri->ret_addr = (kprobe_opcode_t *)regs->link;
> - ri->fp = NULL;
> -
> - /* Replace the return addr with trampoline addr */
> - regs->link = (unsigned long)__kretprobe_trampoline;
> -}
> -NOKPROBE_SYMBOL(arch_prepare_kretprobe);
> -
> static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
> {
> int ret;
> @@ -394,49 +384,6 @@ int kprobe_handler(struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(kprobe_handler);
>
> -/*
> - * Function return probe trampoline:
> - * - init_kprobes() establishes a probepoint here
> - * - When the probed function returns, this probe
> - * causes the handlers to fire
> - */
> -asm(".global __kretprobe_trampoline\n"
> - ".type __kretprobe_trampoline, @function\n"
> - "__kretprobe_trampoline:\n"
> - "nop\n"
> - "blr\n"
> - ".size __kretprobe_trampoline, .-__kretprobe_trampoline\n");
> -
> -/*
> - * Called when the probe at kretprobe trampoline is hit
> - */
> -static int trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
> -{
> - unsigned long orig_ret_address;
> -
> - orig_ret_address = __kretprobe_trampoline_handler(regs, NULL);
> - /*
> - * We get here through one of two paths:
> - * 1. by taking a trap -> kprobe_handler() -> here
> - * 2. by optprobe branch -> optimized_callback() -> opt_pre_handler()
> -> here
> - *
> - * When going back through (1), we need regs->nip to be setup properly
> - * as it is used to determine the return address from the trap.
> - * For (2), since nip is not honoured with optprobes, we instead setup
> - * the link register properly so that the subsequent 'blr' in
> - * __kretprobe_trampoline jumps back to the right instruction.
> - *
> - * For nip, we should set the address to the previous instruction since
> - * we end up emulating it in kprobe_handler(), which increments the nip
> - * again.
> - */
> - regs_set_return_ip(regs, orig_ret_address - 4);
> - regs->link = orig_ret_address;
> -
> - return 0;
> -}
> -NOKPROBE_SYMBOL(trampoline_probe_handler);
> -
> /*
> * Called after single-stepping. p->addr is the address of the
> * instruction whose first byte has been replaced by the "breakpoint"
> @@ -539,19 +486,9 @@ int kprobe_fault_handler(struct pt_regs *regs, int
> trapnr)
> }
> NOKPROBE_SYMBOL(kprobe_fault_handler);
>
> -static struct kprobe trampoline_p = {
> - .addr = (kprobe_opcode_t *) &__kretprobe_trampoline,
> - .pre_handler = trampoline_probe_handler
> -};
> -
> -int __init arch_init_kprobes(void)
> -{
> - return register_kprobe(&trampoline_p);
> -}
> -
> int arch_trampoline_kprobe(struct kprobe *p)
> {
> - if (p->addr == (kprobe_opcode_t *)&__kretprobe_trampoline)
> + if (p->addr == (kprobe_opcode_t *)&arch_rethook_trampoline)
> return 1;
>
> return 0;
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index 004fae2044a3..c0b351d61058 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -56,7 +56,7 @@ static unsigned long can_optimize(struct kprobe *p)
> * has a 'nop' instruction, which can be emulated.
> * So further checks can be skipped.
> */
> - if (p->addr == (kprobe_opcode_t *)&__kretprobe_trampoline)
> + if (p->addr == (kprobe_opcode_t *)&arch_rethook_trampoline)
> return addr + sizeof(kprobe_opcode_t);
>
> /*
> diff --git a/arch/powerpc/kernel/rethook.c b/arch/powerpc/kernel/rethook.c
> new file mode 100644
> index 000000000000..7386f602c9ab
> --- /dev/null
> +++ b/arch/powerpc/kernel/rethook.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PowerPC implementation of rethook. This depends on kprobes.
> + */
> +
> +#include <linux/kprobes.h>
> +#include <linux/rethook.h>
> +
> +/*
> + * Function return trampoline:
> + * - init_kprobes() establishes a probepoint here
> + * - When the probed function returns, this probe
> + * causes the handlers to fire
> + */
> +asm(".global arch_rethook_trampoline\n"
> + ".type arch_rethook_trampoline, @function\n"
> + "arch_rethook_trampoline:\n"
> + "nop\n"
> + "blr\n"
> + ".size arch_rethook_trampoline, .-arch_rethook_trampoline\n");
> +
> +/*
> + * Called when the probe at kretprobe trampoline is hit
> + */
> +static int trampoline_rethook_handler(struct kprobe *p, struct pt_regs *regs)
> +{
> + unsigned long orig_ret_address;
> +
> + orig_ret_address = rethook_trampoline_handler(regs, 0);
> + /*
> + * We get here through one of two paths:
> + * 1. by taking a trap -> kprobe_handler() -> here
> + * 2. by optprobe branch -> optimized_callback() -> opt_pre_handler()
> -> here
> + *
> + * When going back through (1), we need regs->nip to be setup properly
> + * as it is used to determine the return address from the trap.
> + * For (2), since nip is not honoured with optprobes, we instead setup
> + * the link register properly so that the subsequent 'blr' in
> + * __kretprobe_trampoline jumps back to the right instruction.
> + *
> + * For nip, we should set the address to the previous instruction since
> + * we end up emulating it in kprobe_handler(), which increments the nip
> + * again.
> + */
> + regs_set_return_ip(regs, orig_ret_address - 4);
> + regs->link = orig_ret_address;
I think we can move these into arch_rethook_fixup_return(), so
trampoline_rethook_handler() can simply invoke
rethook_trampoline_handler().
> +
> + return 0;
> +}
> +NOKPROBE_SYMBOL(trampoline_rethook_handler);
> +
> +void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs,
> bool mcount)
> +{
> + rh->ret_addr = regs->link;
> + rh->frame = 0;
There is additional code to validate our assumption with a frame pointer
set, so I think we should set this to regs->gpr[1].
> +
> + /* Replace the return addr with trampoline addr */
> + regs->link = (unsigned long)arch_rethook_trampoline;
> +}
> +NOKPROBE_SYMBOL(arch_rethook_prepare);
> +
> +static struct kprobe trampoline_p = {
> + .addr = (kprobe_opcode_t *) &arch_rethook_trampoline,
> + .pre_handler = trampoline_rethook_handler
> +};
> +
> +/* rethook initializer */
> +int arch_init_kprobes(void)
Needs __init annotation.
> +{
> + return register_kprobe(&trampoline_p);
> +}
> diff --git a/arch/powerpc/kernel/stacktrace.c
> b/arch/powerpc/kernel/stacktrace.c
> index e6a958a5da27..a31648b45956 100644
> --- a/arch/powerpc/kernel/stacktrace.c
> +++ b/arch/powerpc/kernel/stacktrace.c
> @@ -21,6 +21,7 @@
> #include <asm/processor.h>
> #include <linux/ftrace.h>
> #include <asm/kprobes.h>
> +#include <linux/rethook.h>
>
> #include <asm/paca.h>
>
> @@ -133,14 +134,15 @@ int __no_sanitize_address
> arch_stack_walk_reliable(stack_trace_consume_fn consum
> * arch-dependent code, they are generic.
> */
> ip = ftrace_graph_ret_addr(task, &graph_idx, ip, stack);
> -#ifdef CONFIG_KPROBES
> +
> /*
> * Mark stacktraces with kretprobed functions on them
> * as unreliable.
> */
> - if (ip == (unsigned long)__kretprobe_trampoline)
> - return -EINVAL;
> -#endif
> + #ifdef CONFIG_RETHOOK
The #ifdef usually starts at column 0, and there is no need to indent
the below code.
- Naveen
> + if (ip == (unsigned long)arch_rethook_trampoline)
> + return -EINVAL;
> + #endif
>
> if (!consume_entry(cookie, ip))
> return -EINVAL;
> --
> 2.44.0
>
