Re: [RFC PATCH v2 3/6] fprobe: rethook: Use fprobe_regs in fprobe exit handler and rethook

From: Florent Revest
Date: Wed Aug 09 2023 - 06:31:04 EST

Next message: Florent Revest: "Re: [RFC PATCH v2 4/6] tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS"
Previous message: Joao Martins: "Re: [PATCH 0/2] KVM: SVM: Set pCPU during IRTE update if vCPU is running"
Next in thread: Google: "Re: [RFC PATCH v2 3/6] fprobe: rethook: Use fprobe_regs in fprobe exit handler and rethook"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

The title says "Use fprobe_regs", I think you meant ftrace_regs (we
have enough problems with two regs structs already! :o) ) same comment
for patch 1

On Mon, Aug 7, 2023 at 8:49 AM Masami Hiramatsu (Google)
<mhiramat@xxxxxxxxxx> wrote:
>
> From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
>
> Change the fprobe exit handler and rethook to use ftrace_regs data structure
> instead of pt_regs. This also introduce HAVE_FTRACE_REGS_COMPATIBLE_WITH_PT_REGS

The macro name in the patch description doesn't match the one in the
patch (HAVE_PT_REGS_COMPAT_FTRACE_REGS)

> which means the ftrace_regs is equal to the pt_regs so that those are
> compatible. Only if it is enabled, kretprobe will use rethook since kretprobe
> requires pt_regs for backward compatibility.
>
> This means the archs which currently implement rethook for kretprobes needs to
> set that flag and it must ensure struct ftrace_regs is same as pt_regs.

nit: I'm a bit confused when you say that these structures are "the
same". Let's take x86 as an example, ftrace_regs is "the same" as
pt_regs in the sense that they use the same space in memory and have
the same fields alignment but my understanding of ftrace_regs is that
it can either be a full pt_regs or a sparse pt_regs. If the trampoline
doesn't go through ftrace_regs_caller_op_ptr, registers like CS will
be missing and ftrace_get_regs will return NULL, I don't call it being
"the same". I guess what you mean here is that a pt_regs can be casted
back into a ftrace_regs. Maybe HAVE_PT_REGS_TO_FTRACE_REGS_CAST would
be a clearer name ?

> If not, it must be either disabling kretprobe or implementing kretprobe
> trampoline separately from rethook trampoline.
>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> ---
> arch/Kconfig | 1 +
> arch/loongarch/Kconfig | 1 +
> arch/s390/Kconfig | 1 +
> arch/x86/Kconfig | 1 +
> arch/x86/kernel/rethook.c | 9 ++++++---
> include/linux/fprobe.h | 2 +-
> include/linux/rethook.h | 11 ++++++-----
> kernel/kprobes.c | 9 +++++++--
> kernel/trace/Kconfig | 7 +++++++
> kernel/trace/bpf_trace.c | 6 +++++-
> kernel/trace/fprobe.c | 6 +++---
> kernel/trace/rethook.c | 16 ++++++++--------
> kernel/trace/trace_fprobe.c | 6 +++++-
> lib/test_fprobe.c | 6 +++---
> samples/fprobe/fprobe_example.c | 2 +-
> 15 files changed, 56 insertions(+), 28 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index aff2746c8af2..e321bdb8b22b 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -201,6 +201,7 @@ config KRETPROBE_ON_RETHOOK
> def_bool y
> depends on HAVE_RETHOOK
> depends on KRETPROBES
> + depends on HAVE_PT_REGS_COMPAT_FTRACE_REGS || !HAVE_DYNAMIC_FTRACE_WITH_ARGS
> select RETHOOK
>
> config USER_RETURN_NOTIFIER
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index e55511af4c77..93a4336b0a94 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -102,6 +102,7 @@ config LOONGARCH
> select HAVE_DMA_CONTIGUOUS
> select HAVE_DYNAMIC_FTRACE
> select HAVE_DYNAMIC_FTRACE_WITH_ARGS
> + select HAVE_PT_REGS_COMPAT_FTRACE_REGS
> select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> select HAVE_DYNAMIC_FTRACE_WITH_REGS
> select HAVE_EBPF_JIT
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 5b39918b7042..299ba17d3316 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -165,6 +165,7 @@ config S390
> select HAVE_DMA_CONTIGUOUS
> select HAVE_DYNAMIC_FTRACE
> select HAVE_DYNAMIC_FTRACE_WITH_ARGS
> + select HAVE_PT_REGS_COMPAT_FTRACE_REGS
> select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> select HAVE_DYNAMIC_FTRACE_WITH_REGS
> select HAVE_EBPF_JIT if HAVE_MARCH_Z196_FEATURES
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 7422db409770..df1b7a2791e8 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -207,6 +207,7 @@ config X86
> select HAVE_DYNAMIC_FTRACE
> select HAVE_DYNAMIC_FTRACE_WITH_REGS
> select HAVE_DYNAMIC_FTRACE_WITH_ARGS if X86_64
> + select HAVE_PT_REGS_COMPAT_FTRACE_REGS if X86_64
> select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> select HAVE_SAMPLE_FTRACE_DIRECT if X86_64
> select HAVE_SAMPLE_FTRACE_DIRECT_MULTI if X86_64
> diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
> index 8a1c0111ae79..79a52bfde562 100644
> --- a/arch/x86/kernel/rethook.c
> +++ b/arch/x86/kernel/rethook.c
> @@ -83,7 +83,8 @@ __used __visible void arch_rethook_trampoline_callback(struct pt_regs *regs)
> * arch_rethook_fixup_return() which called from this
> * rethook_trampoline_handler().
> */
> - rethook_trampoline_handler(regs, (unsigned long)frame_pointer);
> + rethook_trampoline_handler((struct ftrace_regs *)regs,
> + (unsigned long)frame_pointer);
>
> /*
> * Copy FLAGS to 'pt_regs::ss' so that arch_rethook_trapmoline()
> @@ -104,9 +105,10 @@ NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
> STACK_FRAME_NON_STANDARD_FP(arch_rethook_trampoline);
>
> /* This is called from rethook_trampoline_handler(). */
> -void arch_rethook_fixup_return(struct pt_regs *regs,
> +void arch_rethook_fixup_return(struct ftrace_regs *fregs,
> unsigned long correct_ret_addr)
> {
> + struct pt_regs *regs = ftrace_get_regs(fregs);
> unsigned long *frame_pointer = (void *)(regs + 1);
>
> /* Replace fake return address with real one. */
> @@ -114,8 +116,9 @@ void arch_rethook_fixup_return(struct pt_regs *regs,
> }
> NOKPROBE_SYMBOL(arch_rethook_fixup_return);
>
> -void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
> +void arch_rethook_prepare(struct rethook_node *rh, struct ftrace_regs *fregs, bool mcount)
> {
> + struct pt_regs *regs = ftrace_get_regs(fregs);
> unsigned long *stack = (unsigned long *)regs->sp;
>
> rh->ret_addr = stack[0];
> diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> index 36c0595f7b93..b9c0c216dedb 100644
> --- a/include/linux/fprobe.h
> +++ b/include/linux/fprobe.h
> @@ -38,7 +38,7 @@ struct fprobe {
> unsigned long ret_ip, struct ftrace_regs *regs,
> void *entry_data);
> void (*exit_handler)(struct fprobe *fp, unsigned long entry_ip,
> - unsigned long ret_ip, struct pt_regs *regs,
> + unsigned long ret_ip, struct ftrace_regs *regs,
> void *entry_data);
> };
>
> diff --git a/include/linux/rethook.h b/include/linux/rethook.h
> index 26b6f3c81a76..138d64c8b67b 100644
> --- a/include/linux/rethook.h
> +++ b/include/linux/rethook.h
> @@ -7,6 +7,7 @@
>
> #include <linux/compiler.h>
> #include <linux/freelist.h>
> +#include <linux/ftrace.h>
> #include <linux/kallsyms.h>
> #include <linux/llist.h>
> #include <linux/rcupdate.h>
> @@ -14,7 +15,7 @@
>
> struct rethook_node;
>
> -typedef void (*rethook_handler_t) (struct rethook_node *, void *, unsigned long, struct pt_regs *);
> +typedef void (*rethook_handler_t) (struct rethook_node *, void *, unsigned long, struct ftrace_regs *);
>
> /**
> * struct rethook - The rethook management data structure.
> @@ -64,12 +65,12 @@ void rethook_free(struct rethook *rh);
> void rethook_add_node(struct rethook *rh, struct rethook_node *node);
> struct rethook_node *rethook_try_get(struct rethook *rh);
> void rethook_recycle(struct rethook_node *node);
> -void rethook_hook(struct rethook_node *node, struct pt_regs *regs, bool mcount);
> +void rethook_hook(struct rethook_node *node, struct ftrace_regs *regs, bool mcount);
> unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame,
> struct llist_node **cur);
>
> /* 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_prepare(struct rethook_node *node, struct ftrace_regs *regs, bool mcount);
> void arch_rethook_trampoline(void);
>
> /**
> @@ -84,11 +85,11 @@ static inline bool is_rethook_trampoline(unsigned long addr)
> }
>
> /* If the architecture needs to fixup the return address, implement it. */
> -void arch_rethook_fixup_return(struct pt_regs *regs,
> +void arch_rethook_fixup_return(struct ftrace_regs *regs,
> unsigned long correct_ret_addr);
>
> /* Generic trampoline handler, arch code must prepare asm stub */
> -unsigned long rethook_trampoline_handler(struct pt_regs *regs,
> +unsigned long rethook_trampoline_handler(struct ftrace_regs *regs,
> unsigned long frame);
>
> #ifdef CONFIG_RETHOOK
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 1fc6095d502d..ccbe41c961c3 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2120,7 +2120,7 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> if (rp->entry_handler && rp->entry_handler(ri, regs))
> rethook_recycle(rhn);
> else
> - rethook_hook(rhn, regs, kprobe_ftrace(p));
> + rethook_hook(rhn, (struct ftrace_regs *)regs, kprobe_ftrace(p));

I think there are two things that can be meant with "rethook uses ftrace_regs":

- rethook callbacks receive a ftrace_regs (that's what you do further down)
- rethook can hook to a traced function using a ftrace_regs (that's
what you use in fprobe now)

But I think the second proposition shouldn't imply that rethook_hook
can _only_ hook to ftrace_regs. For the kprobe use case, I think there
should also be a rethook_hook_pt_regs() that operates on a pt_regs. We
could have a default implementation of rethook_hook that calls into
the other (or vice versa) on HAVE_FTRACE_REGS_COMPATIBLE_WITH_PT_REGS
but I think it's good to separate these two APIs

Next message: Florent Revest: "Re: [RFC PATCH v2 4/6] tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS"
Previous message: Joao Martins: "Re: [PATCH 0/2] KVM: SVM: Set pCPU during IRTE update if vCPU is running"
Next in thread: Google: "Re: [RFC PATCH v2 3/6] fprobe: rethook: Use fprobe_regs in fprobe exit handler and rethook"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]