[v2] 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 <adubey@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

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 <adubey@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
>

On Mon, 17 Jun 2024 18:28:07 +0530
Naveen N Rao <naveen@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 <adubey@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
> >

On Tue, Jun 18, 2024 at 06:43:06AM GMT, Masami Hiramatsu wrote:
> On Mon, 17 Jun 2024 18:28:07 +0530
> Naveen N Rao <naveen@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 <adubey@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
...
> > > +
> > > +	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?

Yes, you are right. That would be a good addition. I suppose we could 
add something in show_stack() to show the actual function name rather 
than the rethook trampoline. It can be a separate patch though.


Thanks,
Naveen