Re: [PATCH] PowerPC: Replace kretprobe with rethook

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Thu, 16 May 2024 09:46:46 -0400
Abhishek Dubey <adubey@linux.ibm.com> wrote:

> This is an adaptation of commit f3a112c0c40d ("x86,rethook,kprobes:
> Replace kretprobe with rethook on x86") to Power.
> 
> 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")
> 

Hi Abhishek,

Thanks for applying rethook, it looks good. I have comments below.

> diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
> index e6a958a5da27..6de912cf198c 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,13 @@ 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

This still needs to check CONFIG_RETHOOK.

> +
>  		/*
>  		 * Mark stacktraces with kretprobed functions on them
>  		 * as unreliable.
>  		 */
> -		if (ip == (unsigned long)__kretprobe_trampoline)
> +		if (ip == (unsigned long)arch_rethook_trampoline)
>  			return -EINVAL;
> -#endif
>  
>  		if (!consume_entry(cookie, ip))
>  			return -EINVAL;
> diff --git a/include/linux/rethook.h b/include/linux/rethook.h
> index ba60962805f6..9f2fb6abdc60 100644
> --- a/include/linux/rethook.h
> +++ b/include/linux/rethook.h
> @@ -65,7 +65,6 @@ void rethook_recycle(struct rethook_node *node);
>  void rethook_hook(struct rethook_node *node, struct pt_regs *regs, bool mcount);
>  unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame,
>  				    struct llist_node **cur);
> -

nit: removed unrelated line.

>  /* Arch dependent code must implement arch_* and trampoline code */
>  void arch_rethook_prepare(struct rethook_node *node, struct pt_regs *regs, bool mcount);
>  void arch_rethook_trampoline(void);
> -- 
> 2.44.0
> 

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>