On Mon, 17 Jun 2024 18:28:07 +0530
Naveen N Rao <nav...@kernel.org> wrote:
> 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].
Additonal note: If this sets regs->gpr[1], pass it to
rethook_trampoline_handler()
too, so that it can find correct frame.
BTW, it seems powerpc does not use kretprobe/rethook shadow stack for
stack unwinding yet, is that right?
Thanks!
>
> > +
> > + /* 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
> >
--
Masami Hiramatsu (Google) <mhira...@kernel.org>
