Re: [PATCH v3] PowerPC: Replace kretprobe with rethook
On Tue, 09 Jul 2024 12:28:29 +0530 Naveen N Rao wrote: > Masami Hiramatsu wrote: > > On Thu, 27 Jun 2024 09:21:01 -0400 > > Abhishek Dubey wrote: > > > >> +/* rethook initializer */ > >> +int __init arch_init_kprobes(void) > >> +{ > >> + return register_kprobe(&trampoline_p); > >> +} > > > > No, please don't use arch_init_kprobes() for initializing rethook, since > > rethook is used from fprobe too (at this moment). > > > > If you want to make it relays on kprobes, you have to make a dependency > > in powerpc's kconfig, e.g. > > > > select HAVE_RETHOOK if KPROBES > > > > But I don't recommend it. > > Given that kretprobe has always worked this way on powerpc, I think this > is a fair tradeoff. We get to enable fprobes on powerpc only if kprobes > is also enabled. > > Longer term, it would certainly be nice to get rid of that probe, and to > expand the trampoline to directly invoke the rethook callback. OK. In longer term, rethook will be only for kretprobe, and kretprobe will be replaced by fprobe[1]. So please comment it and add that [1] https://lore.kernel.org/all/172000134410.63468.1374887213469474.stgit@devnote2/ Thank you, > > > Thanks, > Naveen -- Masami Hiramatsu (Google)
Re: [PATCH v3] PowerPC: Replace kretprobe with rethook
Masami Hiramatsu wrote: On Thu, 27 Jun 2024 09:21:01 -0400 Abhishek Dubey wrote: +/* rethook initializer */ +int __init arch_init_kprobes(void) +{ + return register_kprobe(&trampoline_p); +} No, please don't use arch_init_kprobes() for initializing rethook, since rethook is used from fprobe too (at this moment). If you want to make it relays on kprobes, you have to make a dependency in powerpc's kconfig, e.g. select HAVE_RETHOOK if KPROBES But I don't recommend it. Given that kretprobe has always worked this way on powerpc, I think this is a fair tradeoff. We get to enable fprobes on powerpc only if kprobes is also enabled. Longer term, it would certainly be nice to get rid of that probe, and to expand the trampoline to directly invoke the rethook callback. Thanks, Naveen
Re: [PATCH v3] PowerPC: Replace kretprobe with rethook
On Thu, 27 Jun 2024 09:21:01 -0400 Abhishek Dubey wrote: > +/* rethook initializer */ > +int __init arch_init_kprobes(void) > +{ > + return register_kprobe(&trampoline_p); > +} No, please don't use arch_init_kprobes() for initializing rethook, since rethook is used from fprobe too (at this moment). If you want to make it relays on kprobes, you have to make a dependency in powerpc's kconfig, e.g. select HAVE_RETHOOK if KPROBES But I don't recommend it. Thank you, -- Masami Hiramatsu (Google)
Re: [PATCH v3] PowerPC: Replace kretprobe with rethook
Hi Abhishek, This patch looks good to me, except for one main change in trampoline_rethook_handler(). Other than that, I have a few more minor comments. Sorry, I should have caught that last time. But, the other change needs a new submission, so good to get these addressed as well. For the subject, we generally use 'powerpc' (all lowercase): powerpc: Replace kretprobe with rethook On Thu, Jun 27, 2024 at 09:21:01AM GMT, Abhishek Dubey wrote: > This is an adaptation of commit f3a112c0c40d ("x86,rethook,kprobes: > Replace kretprobe with rethook on x86") to PowerPC. ^^^ Here too. > > Replaces the kretprobe code with rethook on Power. With this patch, > kretprobe on Power uses the rethook instead of kretprobe specific > trampoline code. In general, it is good to describe what is being done in the patch. So, rather than 'Replaces', you can start that with 'Replace kretprobe code with rethook on powerpc.' The second sentence also looks redundant. It is good if you can describe what the patch does where possible. For instance: Rethook follows the existing kretprobe implementation, but separates it from kprobes so that it can be used by fprobe (ftrace-based function entry/exit probes). As such, this patch also enables fprobe to work on powerpc. The only other change compared to the existing kretprobe implementation is doing the return address fixup in arch_rethook_fixup_return(). > > Reference to other archs: > commit b57c2f124098 ("riscv: add riscv rethook implementation") > commit 7b0a096436c2 ("LoongArch: Replace kretprobe with rethook") > > Signed-off-by: Abhishek Dubey > --- > > Changes in v3: > * Fixing return address moved to arch_rethook_fixup_return() > * Addressed other minor comments > * show_stack mods to show correct fn name to go in separate patch > --- > 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| 77 > arch/powerpc/kernel/stacktrace.c | 6 ++- > 6 files changed, 85 insertions(+), 67 deletions(-) > create mode 100644 arch/powerpc/kernel/rethook.c > > 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_addr
[PATCH v3] PowerPC: Replace kretprobe with rethook
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 --- Changes in v3: * Fixing return address moved to arch_rethook_fixup_return() * Addressed other minor comments * show_stack mods to show correct fn name to go in separate patch --- 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| 77 arch/powerpc/kernel/stacktrace.c | 6 ++- 6 files changed, 85 insertions(+), 67 deletions(-) create mode 100644 arch/powerpc/kernel/rethook.c 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_tram