Re: [PATCH v7 2/3] arm64: implement ftrace with regs

From: Mark Rutland
Date: Tue Apr 02 2019 - 22:49:03 EST

Next message: Chen Zhou: "[PATCH 3/3] kdump: update Documentation about crashkernel on arm64"
Previous message: Michael Ellerman: "Re: [PATCH 2/2] arch: add pidfd and io_uring syscalls everywhere"
Next in thread: Steven Rostedt: "Re: [PATCH v7 2/3] arm64: implement ftrace with regs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Torsten,

Sorry for the long delay prior to this reply.

On Fri, Jan 18, 2019 at 05:39:08PM +0100, Torsten Duwe wrote:
> Once gcc8 adds 2 NOPs at the beginning of each function, replace the
> first NOP thus generated with a quick LR saver (move it to scratch reg
> x9), so the 2nd replacement insn, the call to ftrace, does not clobber
> the value. Ftrace will then generate the standard stack frames.
>
> Note that patchable-function-entry in GCC disables IPA-RA, which means
> ABI register calling conventions are obeyed *and* scratch registers
> such as x9 are available.
>
> Introduce and handle an ftrace_regs_trampoline for module PLTs, right
> after ftrace_trampoline, and double the size of this special section.
>
> Signed-off-by: Torsten Duwe <duwe@xxxxxxx>
>
> ---
>
> Mark, if you see your ftrace entry macro code being represented correctly
> here, please add your sign-off, As I've initially copied it from your mail.
>
> ---
> arch/arm64/include/asm/ftrace.h | 17 ++++-
> arch/arm64/include/asm/module.h | 3
> arch/arm64/kernel/entry-ftrace.S | 125 +++++++++++++++++++++++++++++++++++++--
> arch/arm64/kernel/ftrace.c | 114 ++++++++++++++++++++++++++---------
> arch/arm64/kernel/module-plts.c | 3
> arch/arm64/kernel/module.c | 2
> 6 files changed, 227 insertions(+), 37 deletions(-)
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -14,9 +14,24 @@
> #include <asm/insn.h>
>
> #define HAVE_FUNCTION_GRAPH_FP_TEST
> -#define MCOUNT_ADDR ((unsigned long)_mcount)
> #define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE
>
> +/*
> + * DYNAMIC_FTRACE_WITH_REGS is implemented by adding 2 NOPs at the beginning
> + * of each function, with the second NOP actually calling ftrace. In contrary
> + * to a classic _mcount call, the call instruction to be modified is thus
> + * the second one, and not the only one.
> + */
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +#define ARCH_SUPPORTS_FTRACE_OPS 1
> +#define REC_IP_BRANCH_OFFSET AARCH64_INSN_SIZE
> +/* All we need is some magic value. Simply use "_mCount:" */
> +#define MCOUNT_ADDR (0x5f6d436f756e743a)

I'm really not keen on having a magic constant, and I'd really like to
see MCOUNT_ADDR disappear entirely when the compiler doesn't generate an
mcount call. I'm concerned that this is confusing and fragile.

I think that it would be better to make the core ftrace API agnostic of
mcount, and to teach it that there's a one-time initialization step for
callsites (which is not necessarily patching a call with a NOP).

We currently have:

ftrace_make_nop(mod, rec, addr)
ftrace_make_call(rec, addr)
ftrace_modify_call(rec, old_addr, new_addr)

... whereas we could have:

ftrace_call_init(mod, rec)
ftrace_call_enable(rec, addr)
ftrace_call_disable(rec, addr)
ftrace_call_modify(rec, old_addr, new_addr)

... so we wouldn't need to special-case anything for initialization (and
don't need MCOUNT_ADDR at all), and it would be clearer as to what was
happening at each stage.

> +#else
> +#define REC_IP_BRANCH_OFFSET 0
> +#define MCOUNT_ADDR ((unsigned long)_mcount)
> +#endif
> +
> #ifndef __ASSEMBLY__
> #include <linux/compat.h>
>
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -10,6 +10,7 @@
> */
>
> #include <linux/linkage.h>
> +#include <asm/asm-offsets.h>
> #include <asm/assembler.h>
> #include <asm/ftrace.h>
> #include <asm/insn.h>
> @@ -124,6 +125,7 @@ EXPORT_SYMBOL(_mcount)
> NOKPROBE(_mcount)
>
> #else /* CONFIG_DYNAMIC_FTRACE */
> +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> /*
> * _mcount() is used to build the kernel with -pg option, but all the branch
> * instructions to _mcount() are replaced to NOP initially at kernel start up,
> @@ -163,11 +165,6 @@ GLOBAL(ftrace_graph_call) // ftrace_gra
>
> mcount_exit
> ENDPROC(ftrace_caller)
> -#endif /* CONFIG_DYNAMIC_FTRACE */
> -
> -ENTRY(ftrace_stub)
> - ret
> -ENDPROC(ftrace_stub)
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> /*
> @@ -187,7 +184,125 @@ ENTRY(ftrace_graph_caller)
>
> mcount_exit
> ENDPROC(ftrace_graph_caller)
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +
> +#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> +
> + .macro ftrace_regs_entry, allregs=0
> + /* make room for pt_regs, plus a callee frame */
> + sub sp, sp, #(S_FRAME_SIZE + 16)
> +
> + /* save function arguments */
> + 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]
>
> + .if \allregs == 1
> + 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]
> + .endif
> +
> + /* Save fp and x28, which is used in this function. */
> + stp x28, x29, [sp, #S_X28]
> +
> + /* The stack pointer as it was on ftrace_caller entry... */
> + add x28, sp, #(S_FRAME_SIZE + 16)
> + /* ...and the link Register at callee entry */
> + stp x9, x28, [sp, #S_LR] /* to pt_regs.r[30] and .sp */
> +
> + /* The program counter just after the ftrace call site */
> + str lr, [sp, #S_PC]

For consistency with the existing ftrace assembly, please use 'x30'
rather than 'lr'.

> +
> + /* Now fill in callee's preliminary stackframe. */
> + stp x29, x9, [sp, #S_FRAME_SIZE]
> + /* Let FP point to it. */
> + add x29, sp, #S_FRAME_SIZE
> +
> + /* Our stackframe, stored inside pt_regs. */
> + stp x29, x30, [sp, #S_STACKFRAME]
> + add x29, sp, #S_STACKFRAME
> + .endm
> +
> +ENTRY(ftrace_regs_caller)
> + ftrace_regs_entry 1
> + b ftrace_common
> +ENDPROC(ftrace_regs_caller)
> +
> +ENTRY(ftrace_caller)
> + ftrace_regs_entry 0
> + b ftrace_common
> +ENDPROC(ftrace_caller)
> +
> +ENTRY(ftrace_common)
> +
> + mov x3, sp /* pt_regs are @sp */
> + ldr_l x2, function_trace_op, x0
> + mov x1, x9 /* parent IP */
> + sub x0, lr, #8 /* function entry == IP */

Please use 'x30' rather than 'lr'.

> +
> +GLOBAL(ftrace_call)
> + bl ftrace_stub
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +GLOBAL(ftrace_graph_call) // ftrace_graph_caller();
> + nop // If enabled, this will be replaced
> + // "b ftrace_graph_caller"
> +#endif
> +
> +/*
> + * GCC's patchable-function-entry implicitly disables IPA-RA,
> + * so all non-argument registers are either scratch / dead
> + * or callee-saved (within the ftrace framework). Function
> + * arguments of the call we are intercepting right now however
> + * need to be preserved in any case.
> + */
> +ftrace_common_return:
> + /* restore function args */
> + ldp x0, x1, [sp]
> + ldp x2, x3, [sp, #S_X2]
> + ldp x4, x5, [sp, #S_X4]
> + ldp x6, x7, [sp, #S_X6]
> + ldr x8, [sp, #S_X8]
> +
> + /* restore fp and x28 */
> + ldp x28, x29, [sp, #S_X28]
> +
> + ldr lr, [sp, #S_LR]

Please use 'x30' rather than 'lr'.

> + ldr x9, [sp, #S_PC]
> + /* clean up both frames, ours and callee preliminary */
> + add sp, sp, #S_FRAME_SIZE + 16
> +
> + ret x9
> +ENDPROC(ftrace_common)
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +ENTRY(ftrace_graph_caller)
> + ldr x0, [sp, #S_PC] /* pc */
> + sub x0, x0, #8 /* start of the ftrace call site */
> + add x1, sp, #S_LR /* &lr */
> + ldr x2, [sp, #S_FRAME_SIZE] /* fp */
> + bl prepare_ftrace_return
> + b ftrace_common_return
> +ENDPROC(ftrace_graph_caller)
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> +#endif /* CONFIG_DYNAMIC_FTRACE */
> +
> +ENTRY(ftrace_stub)
> + ret
> +ENDPROC(ftrace_stub)
> +
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> /*
> * void return_to_handler(void)
> *
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -65,19 +65,67 @@ int ftrace_update_ftrace_func(ftrace_fun
> return ftrace_modify_code(pc, 0, new, false);
> }
>
> +#ifdef CONFIG_ARM64_MODULE_PLTS
> +static int install_ftrace_trampoline(struct module *mod, unsigned long *addr)
> +{
> + struct plt_entry trampoline, *mod_trampoline;
> +
> + /*
> + * Iterate over
> + * mod->arch.ftrace_trampolines[MOD_ARCH_NR_FTRACE_TRAMPOLINES]
> + * The assignment to various ftrace functions happens here.
> + */
> + if (*addr == FTRACE_ADDR)
> + mod_trampoline = &mod->arch.ftrace_trampolines[0];
> + else if (*addr == FTRACE_REGS_ADDR)
> + mod_trampoline = &mod->arch.ftrace_trampolines[1];
> + else
> + return -EINVAL;
> +
> + trampoline = get_plt_entry(*addr, mod_trampoline);
> +
> + if (!plt_entries_equal(mod_trampoline, &trampoline)) {
> + /* point the trampoline at our ftrace entry point */
> + module_disable_ro(mod);
> + *mod_trampoline = trampoline;
> + module_enable_ro(mod, true);
> +
> + /* update trampoline before patching in the branch */
> + smp_wmb();
> + }
> + *addr = (unsigned long)(void *)mod_trampoline;
> +
> + return 0;
> +}
> +#endif
> +
> +/*
> + * Ftrace with regs generates the tracer calls as close as possible to
> + * the function entry; no stack frame has been set up at that point.
> + * In order to make another call e.g to ftrace_caller, the LR must be
> + * saved from being overwritten.
> + * Between two functions, and with IPA-RA turned off, the scratch registers
> + * are available, so move the LR to x9 before calling into ftrace.
> + * "mov x9, lr" is officially aliased from "orr x9, xzr, lr".
> + */
> +#define MOV_X9_X30 aarch64_insn_gen_logical_shifted_reg( \
> + AARCH64_INSN_REG_9, AARCH64_INSN_REG_ZR, \
> + AARCH64_INSN_REG_LR, 0, AARCH64_INSN_VARIANT_64BIT, \
> + AARCH64_INSN_LOGIC_ORR)
> +
> /*
> * Turn on the call to ftrace_caller() in instrumented function
> */
> int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> {
> - unsigned long pc = rec->ip;
> + unsigned long pc = rec->ip + REC_IP_BRANCH_OFFSET;
> u32 old, new;
> long offset = (long)pc - (long)addr;
>
> if (offset < -SZ_128M || offset >= SZ_128M) {
> #ifdef CONFIG_ARM64_MODULE_PLTS
> - struct plt_entry trampoline;
> struct module *mod;
> + int ret;
>
> /*
> * On kernels that support module PLTs, the offset between the
> @@ -96,32 +144,14 @@ int ftrace_make_call(struct dyn_ftrace *
> if (WARN_ON(!mod))
> return -EINVAL;
>
> - /*
> - * There is only one ftrace trampoline per module. For now,
> - * this is not a problem since on arm64, all dynamic ftrace
> - * invocations are routed via ftrace_caller(). This will need
> - * to be revisited if support for multiple ftrace entry points
> - * is added in the future, but for now, the pr_err() below
> - * deals with a theoretical issue only.
> - */
> - trampoline = get_plt_entry(addr, mod->arch.ftrace_trampoline);
> - if (!plt_entries_equal(mod->arch.ftrace_trampoline,
> - &trampoline)) {
> - if (!plt_entries_equal(mod->arch.ftrace_trampoline,
> - &(struct plt_entry){})) {
> - pr_err("ftrace: far branches to multiple entry points unsupported inside a single module\n");
> - return -EINVAL;
> - }
> -
> - /* point the trampoline to our ftrace entry point */
> - module_disable_ro(mod);
> - *mod->arch.ftrace_trampoline = trampoline;
> - module_enable_ro(mod, true);
> + /* Check against our well-known list of ftrace entry points */
> + if (addr == FTRACE_ADDR || addr == FTRACE_REGS_ADDR) {

These checks exist within install_ftrace_trampoline(), so we don't need
to duplicate them here.

> + ret = install_ftrace_trampoline(mod, &addr);
> + if (ret < 0)
> + return ret;
> + } else
> + return -EINVAL;

Per coding style, if either branch of an if-else has braces, the other
side should too.

However, as above, I think you can get rid of this entirely, and rely on
install_ftrace_trampoline() to return an error for a bad addr.

Thanks,
Mark.

Next message: Chen Zhou: "[PATCH 3/3] kdump: update Documentation about crashkernel on arm64"
Previous message: Michael Ellerman: "Re: [PATCH 2/2] arch: add pidfd and io_uring syscalls everywhere"
Next in thread: Steven Rostedt: "Re: [PATCH v7 2/3] arm64: implement ftrace with regs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]