Message-ID: <YlXNPEZ18RPfjsd6@FVFF77S0Q05N.cambridge.arm.com>
Date:   Tue, 12 Apr 2022 20:04:28 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Shubham Bansal <illusionist.neo@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
        kernel-team@...com, Jiri Olsa <jolsa@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        "Naveen N . Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, Will Deacon <will@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Catalin Marinas <catalin.marinas@....com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH bpf v2 4/4] arm64: rethook: Replace kretprobe trampoline
 with rethook

On Fri, Apr 08, 2022 at 09:51:24AM +0900, Masami Hiramatsu wrote:
> Replace the kretprobe's trampoline code with the rethook on arm64.
> The rethook on arm64 is almost renamed from kretprobe trampoline
> code. The mechanism is completely same.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> ---
>  Changes in v2:
>   - Fix a compile warning about no prototype.
> ---
>  arch/arm64/Kconfig                            |    1 
>  arch/arm64/include/asm/kprobes.h              |    2 -
>  arch/arm64/include/asm/stacktrace.h           |    2 -
>  arch/arm64/kernel/Makefile                    |    1 
>  arch/arm64/kernel/probes/Makefile             |    1 
>  arch/arm64/kernel/probes/kprobes.c            |   15 ----
>  arch/arm64/kernel/probes/kprobes_trampoline.S |   86 -------------------------
>  arch/arm64/kernel/rethook.c                   |   28 ++++++++
>  arch/arm64/kernel/rethook_trampoline.S        |   87 +++++++++++++++++++++++++
>  arch/arm64/kernel/stacktrace.c                |    9 +--
>  10 files changed, 123 insertions(+), 109 deletions(-)
>  delete mode 100644 arch/arm64/kernel/probes/kprobes_trampoline.S
>  create mode 100644 arch/arm64/kernel/rethook.c
>  create mode 100644 arch/arm64/kernel/rethook_trampoline.S
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 57c4c995965f..7d2945930283 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -204,6 +204,7 @@ config ARM64
>  	select HAVE_SYSCALL_TRACEPOINTS
>  	select HAVE_KPROBES
>  	select HAVE_KRETPROBES
> +	select HAVE_RETHOOK
>  	select HAVE_GENERIC_VDSO
>  	select IOMMU_DMA if IOMMU_SUPPORT
>  	select IRQ_DOMAIN
> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
> index 05cd82eeca13..4ac558058377 100644
> --- a/arch/arm64/include/asm/kprobes.h
> +++ b/arch/arm64/include/asm/kprobes.h
> @@ -39,8 +39,6 @@ void arch_remove_kprobe(struct kprobe *);
>  int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
>  int kprobe_exceptions_notify(struct notifier_block *self,
>  			     unsigned long val, void *data);
> -void __kretprobe_trampoline(void);
> -void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
>  
>  #endif /* CONFIG_KPROBES */
>  #endif /* _ARM_KPROBES_H */
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index e77cdef9ca29..f781874f1609 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -58,7 +58,7 @@ struct stackframe {
>  	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
>  	unsigned long prev_fp;
>  	enum stack_type prev_type;
> -#ifdef CONFIG_KRETPROBES
> +#if defined(CONFIG_RETHOOK)
>  	struct llist_node *kr_cur;
>  #endif
>  };
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 986837d7ec82..62e033b1b095 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -60,6 +60,7 @@ obj-$(CONFIG_ACPI_NUMA)			+= acpi_numa.o
>  obj-$(CONFIG_ARM64_ACPI_PARKING_PROTOCOL)	+= acpi_parking_protocol.o
>  obj-$(CONFIG_PARAVIRT)			+= paravirt.o
>  obj-$(CONFIG_RANDOMIZE_BASE)		+= kaslr.o
> +obj-$(CONFIG_RETHOOK)			+= rethook.o rethook_trampoline.o
>  obj-$(CONFIG_HIBERNATION)		+= hibernate.o hibernate-asm.o
>  obj-$(CONFIG_ELF_CORE)			+= elfcore.o
>  obj-$(CONFIG_KEXEC_CORE)		+= machine_kexec.o relocate_kernel.o	\
> diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile
> index 8e4be92e25b1..1fa58cda64ff 100644
> --- a/arch/arm64/kernel/probes/Makefile
> +++ b/arch/arm64/kernel/probes/Makefile
> @@ -1,6 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_KPROBES)		+= kprobes.o decode-insn.o	\
> -				   kprobes_trampoline.o		\
>  				   simulate-insn.o
>  obj-$(CONFIG_UPROBES)		+= uprobes.o decode-insn.o	\
>  				   simulate-insn.o
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index d9dfa82c1f18..4a3cc266e77e 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -399,21 +399,6 @@ int __init arch_populate_kprobe_blacklist(void)
>  	return ret;
>  }
>  
> -void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
> -{
> -	return (void *)kretprobe_trampoline_handler(regs, (void *)regs->regs[29]);
> -}
> -
> -void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
> -				      struct pt_regs *regs)
> -{
> -	ri->ret_addr = (kprobe_opcode_t *)regs->regs[30];
> -	ri->fp = (void *)regs->regs[29];
> -
> -	/* replace return addr (x30) with trampoline */
> -	regs->regs[30] = (long)&__kretprobe_trampoline;
> -}
> -
>  int __kprobes arch_trampoline_kprobe(struct kprobe *p)
>  {
>  	return 0;
> diff --git a/arch/arm64/kernel/probes/kprobes_trampoline.S b/arch/arm64/kernel/probes/kprobes_trampoline.S
> deleted file mode 100644
> index 9a6499bed58b..000000000000
> --- a/arch/arm64/kernel/probes/kprobes_trampoline.S
> +++ /dev/null
> @@ -1,86 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/*
> - * trampoline entry and return code for kretprobes.
> - */
> -
> -#include <linux/linkage.h>
> -#include <asm/asm-offsets.h>
> -#include <asm/assembler.h>
> -
> -	.text
> -
> -	.macro	save_all_base_regs
> -	stp x0, x1, [sp, #S_X0]
> -	stp x2, x3, [sp, #S_X2]
> -	stp x4, x5, [sp, #S_X4]
> -	stp x6, x7, [sp, #S_X6]
> -	stp x8, x9, [sp, #S_X8]
> -	stp x10, x11, [sp, #S_X10]
> -	stp x12, x13, [sp, #S_X12]
> -	stp x14, x15, [sp, #S_X14]
> -	stp x16, x17, [sp, #S_X16]
> -	stp x18, x19, [sp, #S_X18]
> -	stp x20, x21, [sp, #S_X20]
> -	stp x22, x23, [sp, #S_X22]
> -	stp x24, x25, [sp, #S_X24]
> -	stp x26, x27, [sp, #S_X26]
> -	stp x28, x29, [sp, #S_X28]
> -	add x0, sp, #PT_REGS_SIZE
> -	stp lr, x0, [sp, #S_LR]
> -	/*
> -	 * Construct a useful saved PSTATE
> -	 */
> -	mrs x0, nzcv
> -	mrs x1, daif
> -	orr x0, x0, x1
> -	mrs x1, CurrentEL
> -	orr x0, x0, x1
> -	mrs x1, SPSel
> -	orr x0, x0, x1
> -	stp xzr, x0, [sp, #S_PC]
> -	.endm
> -
> -	.macro	restore_all_base_regs
> -	ldr x0, [sp, #S_PSTATE]
> -	and x0, x0, #(PSR_N_BIT | PSR_Z_BIT | PSR_C_BIT | PSR_V_BIT)
> -	msr nzcv, x0
> -	ldp x0, x1, [sp, #S_X0]
> -	ldp x2, x3, [sp, #S_X2]
> -	ldp x4, x5, [sp, #S_X4]
> -	ldp x6, x7, [sp, #S_X6]
> -	ldp x8, x9, [sp, #S_X8]
> -	ldp x10, x11, [sp, #S_X10]
> -	ldp x12, x13, [sp, #S_X12]
> -	ldp x14, x15, [sp, #S_X14]
> -	ldp x16, x17, [sp, #S_X16]
> -	ldp x18, x19, [sp, #S_X18]
> -	ldp x20, x21, [sp, #S_X20]
> -	ldp x22, x23, [sp, #S_X22]
> -	ldp x24, x25, [sp, #S_X24]
> -	ldp x26, x27, [sp, #S_X26]
> -	ldp x28, x29, [sp, #S_X28]
> -	.endm
> -
> -SYM_CODE_START(__kretprobe_trampoline)
> -	sub sp, sp, #PT_REGS_SIZE
> -
> -	save_all_base_regs
> -
> -	/* Setup a frame pointer. */
> -	add x29, sp, #S_FP
> -
> -	mov x0, sp
> -	bl trampoline_probe_handler
> -	/*
> -	 * Replace trampoline address in lr with actual orig_ret_addr return
> -	 * address.
> -	 */
> -	mov lr, x0
> -
> -	/* The frame pointer (x29) is restored with other registers. */
> -	restore_all_base_regs
> -
> -	add sp, sp, #PT_REGS_SIZE
> -	ret
> -
> -SYM_CODE_END(__kretprobe_trampoline)
> diff --git a/arch/arm64/kernel/rethook.c b/arch/arm64/kernel/rethook.c
> new file mode 100644
> index 000000000000..07d8f6ea34a0
> --- /dev/null
> +++ b/arch/arm64/kernel/rethook.c
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Generic return hook for arm64.
> + * Most of the code is copied from arch/arm64/kernel/probes/kprobes.c
> + */
> +
> +#include <linux/kprobes.h>
> +#include <linux/rethook.h>
> +
> +/* This is called from arch_rethook_trampoline() */
> +unsigned long __used arch_rethook_trampoline_callback(struct pt_regs *regs);
> +
> +unsigned long __used arch_rethook_trampoline_callback(struct pt_regs *regs)
> +{
> +	return rethook_trampoline_handler(regs, regs->regs[29]);
> +}
> +NOKPROBE_SYMBOL(arch_rethook_trampoline_callback);
> +
> +int arch_rethook_prepare(struct rethook_node *rhn, struct pt_regs *regs, bool mcount)

What's the `mcount` paramater for, and why don't we seem to use it below?

The presence of that parameter suggests there is a subtle interaction with
ftrace, but the commit message doesn't mention anything of the sort. I really
worried that has implications for the unwinder.

I also see that (with these patches applied) it's possible to select
CONFIG_FPROBE, but the commit message doesn't describe that either, and I'm not
sure how to test any of that.

> +{
> +	rhn->ret_addr = regs->regs[30];
> +	rhn->frame = regs->regs[29];
> +
> +	/* replace return addr (x30) with trampoline */
> +	regs->regs[30] = (u64)arch_rethook_trampoline;
> +	return 0;
> +}
> +NOKPROBE_SYMBOL(arch_rethook_prepare);
> diff --git a/arch/arm64/kernel/rethook_trampoline.S b/arch/arm64/kernel/rethook_trampoline.S
> new file mode 100644
> index 000000000000..146d4553674c
> --- /dev/null
> +++ b/arch/arm64/kernel/rethook_trampoline.S
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * trampoline entry and return code for rethook.
> + * Renamed from arch/arm64/kernel/probes/kprobes_trampoline.S
> + */
> +
> +#include <linux/linkage.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/assembler.h>
> +
> +	.text
> +
> +	.macro	save_all_base_regs
> +	stp x0, x1, [sp, #S_X0]
> +	stp x2, x3, [sp, #S_X2]
> +	stp x4, x5, [sp, #S_X4]
> +	stp x6, x7, [sp, #S_X6]
> +	stp x8, x9, [sp, #S_X8]
> +	stp x10, x11, [sp, #S_X10]
> +	stp x12, x13, [sp, #S_X12]
> +	stp x14, x15, [sp, #S_X14]
> +	stp x16, x17, [sp, #S_X16]
> +	stp x18, x19, [sp, #S_X18]
> +	stp x20, x21, [sp, #S_X20]
> +	stp x22, x23, [sp, #S_X22]
> +	stp x24, x25, [sp, #S_X24]
> +	stp x26, x27, [sp, #S_X26]
> +	stp x28, x29, [sp, #S_X28]
> +	add x0, sp, #PT_REGS_SIZE
> +	stp lr, x0, [sp, #S_LR]
> +	/*
> +	 * Construct a useful saved PSTATE
> +	 */
> +	mrs x0, nzcv
> +	mrs x1, daif
> +	orr x0, x0, x1
> +	mrs x1, CurrentEL
> +	orr x0, x0, x1
> +	mrs x1, SPSel
> +	orr x0, x0, x1

I realise this is just a copy of the existing kretprobe code, but it would be
*really* nice if we could avoid faking the regs when we don't take an
exception, like the FTRACE_WITH_ARGS thing.

The "useful saved PSTATE" is getting increasingly bogus these days, since it
doesn't contain a bunch of values that are in the real PSTATE, and we don't
restore anything other than NZCV anyway.

> +	stp xzr, x0, [sp, #S_PC]
> +	.endm
> +
> +	.macro	restore_all_base_regs
> +	ldr x0, [sp, #S_PSTATE]
> +	and x0, x0, #(PSR_N_BIT | PSR_Z_BIT | PSR_C_BIT | PSR_V_BIT)
> +	msr nzcv, x0
> +	ldp x0, x1, [sp, #S_X0]
> +	ldp x2, x3, [sp, #S_X2]
> +	ldp x4, x5, [sp, #S_X4]
> +	ldp x6, x7, [sp, #S_X6]
> +	ldp x8, x9, [sp, #S_X8]
> +	ldp x10, x11, [sp, #S_X10]
> +	ldp x12, x13, [sp, #S_X12]
> +	ldp x14, x15, [sp, #S_X14]
> +	ldp x16, x17, [sp, #S_X16]
> +	ldp x18, x19, [sp, #S_X18]
> +	ldp x20, x21, [sp, #S_X20]
> +	ldp x22, x23, [sp, #S_X22]
> +	ldp x24, x25, [sp, #S_X24]
> +	ldp x26, x27, [sp, #S_X26]
> +	ldp x28, x29, [sp, #S_X28]
> +	.endm
> +
> +SYM_CODE_START(arch_rethook_trampoline)
> +	sub sp, sp, #PT_REGS_SIZE
> +
> +	save_all_base_regs
> +
> +	/* Setup a frame pointer. */
> +	add x29, sp, #S_FP
> +
> +	mov x0, sp
> +	bl arch_rethook_trampoline_callback
> +	/*
> +	 * Replace trampoline address in lr with actual orig_ret_addr return
> +	 * address.
> +	 */
> +	mov lr, x0
> +
> +	/* The frame pointer (x29) is restored with other registers. */
> +	restore_all_base_regs
> +
> +	add sp, sp, #PT_REGS_SIZE
> +	ret
> +
> +SYM_CODE_END(arch_rethook_trampoline)
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index e4103e085681..5b717af4b555 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -8,6 +8,7 @@
>  #include <linux/export.h>
>  #include <linux/ftrace.h>
>  #include <linux/kprobes.h>
> +#include <linux/rethook.h>
>  #include <linux/sched.h>
>  #include <linux/sched/debug.h>
>  #include <linux/sched/task_stack.h>
> @@ -38,7 +39,7 @@ static notrace void start_backtrace(struct stackframe *frame, unsigned long fp,
>  {
>  	frame->fp = fp;
>  	frame->pc = pc;
> -#ifdef CONFIG_KRETPROBES
> +#if defined(CONFIG_RETHOOK)
>  	frame->kr_cur = NULL;
>  #endif
>  
> @@ -134,9 +135,9 @@ static int notrace unwind_frame(struct task_struct *tsk,
>  		frame->pc = orig_pc;
>  	}
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> -#ifdef CONFIG_KRETPROBES
> -	if (is_kretprobe_trampoline(frame->pc))
> -		frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
> +#ifdef CONFIG_RETHOOK
> +	if (is_rethook_trampoline(frame->pc))
> +		frame->pc = rethook_find_ret_addr(tsk, frame->fp, &frame->kr_cur);
>  #endif

Is it possible to have an fprobe and a regular ftrace graph caller call on the
same ftrace callsite? ... or are those mutually exclusive?

The existing code here depends on kretprobes and ftrace graph caller
instrumentation nesting in a specific order, and I'm worried that might have
changed.

Thanks,
Mark.

Powered by blists - more mailing lists