| Message ID | 20220125153656.1802079-4-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: ftrace fixes and cleanups | expand |
On Tue, Jan 25, 2022 at 7:37 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > Kernel images that are large in comparison to the range of a direct > branch may fail to work as expected with ftrace, as patching a direct > branch to one of the core ftrace routines may not be possible from the > .init.text section, if it is emitted too far away from the normal .text > section. > > This is more likely to affect Thumb2 builds, given that its range is > only -/+ 16 MiB (as opposed to ARM which has -/+ 32 MiB), but may occur > in either ISA. > > To work around this, add a couple of trampolines to .init.text and > swap these in when the ftrace patching code is operating on callers in > .init.text. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/arm/kernel/entry-ftrace.S | 16 ++++++++++++++++ > arch/arm/kernel/ftrace.c | 19 +++++++++++++++++-- > 2 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S > index dca12a09322a..237d435e29aa 100644 > --- a/arch/arm/kernel/entry-ftrace.S > +++ b/arch/arm/kernel/entry-ftrace.S > @@ -270,3 +270,19 @@ ENTRY(ftrace_stub) > .Lftrace_stub: > ret lr > ENDPROC(ftrace_stub) > + > +#ifdef CONFIG_DYNAMIC_FTRACE > + > + __INIT > + > + .macro init_tramp, dst:req > +ENTRY(\dst\()_from_init) > + ldr pc, =\dst > +ENDPROC(\dst\()_from_init) > + .endm > + > + init_tramp ftrace_caller > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > + init_tramp ftrace_regs_caller > +#endif Consider adding __FINIT here; while it's not necessary now, folks who append new code to this file might be surprised to find new code in .init.text. I would also tighten up the __INIT/__FINIT to only surround the invocations of init_tramp and not the macro definition itself. Either way, Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > +#endif > diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c > index db72d3a6522d..d2326794fd09 100644 > --- a/arch/arm/kernel/ftrace.c > +++ b/arch/arm/kernel/ftrace.c > @@ -51,9 +51,18 @@ static unsigned long ftrace_nop_replace(struct dyn_ftrace *rec) > return NOP; > } > > +void ftrace_caller_from_init(void); > +void ftrace_regs_caller_from_init(void); > + > static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr) > { > - return addr; > + if (!IS_ENABLED(CONFIG_DYNAMIC_FTRACE) || > + likely(!is_kernel_inittext(rec->ip))) > + return addr; > + if (!IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS) || > + addr == (unsigned long)&ftrace_caller) > + return (unsigned long)&ftrace_caller_from_init; > + return (unsigned long)&ftrace_regs_caller_from_init; > } > > int ftrace_arch_code_modify_prepare(void) > @@ -189,7 +198,13 @@ int ftrace_make_nop(struct module *mod, > #endif > > new = ftrace_nop_replace(rec); > - ret = ftrace_modify_code(ip, old, new, true); > + /* > + * Locations in .init.text may call __gnu_mcount_mc via a linker > + * emitted veneer if they are too far away from its implementation, and > + * so validation may fail spuriously in such cases. Let's work around > + * this by omitting those from validation. > + */ > + ret = ftrace_modify_code(ip, old, new, !is_kernel_inittext(ip)); > > return ret; > } > -- > 2.30.2 >
On Tue, Jan 25, 2022 at 4:37 PM Ard Biesheuvel <ardb@kernel.org> wrote: > Kernel images that are large in comparison to the range of a direct > branch may fail to work as expected with ftrace, as patching a direct > branch to one of the core ftrace routines may not be possible from the > .init.text section, if it is emitted too far away from the normal .text > section. > > This is more likely to affect Thumb2 builds, given that its range is > only -/+ 16 MiB (as opposed to ARM which has -/+ 32 MiB), but may occur > in either ISA. > > To work around this, add a couple of trampolines to .init.text and > swap these in when the ftrace patching code is operating on callers in > .init.text. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> LGTM Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I suppose part of me wonder what happens if .text itself grows larger than 16MB under thumb2, but I guess then the linker will just fail. Yours, Linus Walleij
diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S index dca12a09322a..237d435e29aa 100644 --- a/arch/arm/kernel/entry-ftrace.S +++ b/arch/arm/kernel/entry-ftrace.S @@ -270,3 +270,19 @@ ENTRY(ftrace_stub) .Lftrace_stub: ret lr ENDPROC(ftrace_stub) + +#ifdef CONFIG_DYNAMIC_FTRACE + + __INIT + + .macro init_tramp, dst:req +ENTRY(\dst\()_from_init) + ldr pc, =\dst +ENDPROC(\dst\()_from_init) + .endm + + init_tramp ftrace_caller +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS + init_tramp ftrace_regs_caller +#endif +#endif diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c index db72d3a6522d..d2326794fd09 100644 --- a/arch/arm/kernel/ftrace.c +++ b/arch/arm/kernel/ftrace.c @@ -51,9 +51,18 @@ static unsigned long ftrace_nop_replace(struct dyn_ftrace *rec) return NOP; } +void ftrace_caller_from_init(void); +void ftrace_regs_caller_from_init(void); + static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr) { - return addr; + if (!IS_ENABLED(CONFIG_DYNAMIC_FTRACE) || + likely(!is_kernel_inittext(rec->ip))) + return addr; + if (!IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS) || + addr == (unsigned long)&ftrace_caller) + return (unsigned long)&ftrace_caller_from_init; + return (unsigned long)&ftrace_regs_caller_from_init; } int ftrace_arch_code_modify_prepare(void) @@ -189,7 +198,13 @@ int ftrace_make_nop(struct module *mod, #endif new = ftrace_nop_replace(rec); - ret = ftrace_modify_code(ip, old, new, true); + /* + * Locations in .init.text may call __gnu_mcount_mc via a linker + * emitted veneer if they are too far away from its implementation, and + * so validation may fail spuriously in such cases. Let's work around + * this by omitting those from validation. + */ + ret = ftrace_modify_code(ip, old, new, !is_kernel_inittext(ip)); return ret; }
Kernel images that are large in comparison to the range of a direct branch may fail to work as expected with ftrace, as patching a direct branch to one of the core ftrace routines may not be possible from the .init.text section, if it is emitted too far away from the normal .text section. This is more likely to affect Thumb2 builds, given that its range is only -/+ 16 MiB (as opposed to ARM which has -/+ 32 MiB), but may occur in either ISA. To work around this, add a couple of trampolines to .init.text and swap these in when the ftrace patching code is operating on callers in .init.text. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm/kernel/entry-ftrace.S | 16 ++++++++++++++++ arch/arm/kernel/ftrace.c | 19 +++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-)