* [PATCH v4 0/2] ARM: Implement MODULE_PLT support in FTRACE @ 2018-03-13 13:53 Alexander Sverdlin 2018-03-13 13:53 ` [PATCH v4 1/2] ARM: PLT: Move struct plt_entries definition to header Alexander Sverdlin 2018-03-13 13:53 ` [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support Alexander Sverdlin 0 siblings, 2 replies; 14+ messages in thread From: Alexander Sverdlin @ 2018-03-13 13:53 UTC (permalink / raw) To: linux-arm-kernel Cc: Alexander Sverdlin, linux-kernel, Russell King, Steven Rostedt, Ingo Molnar, Ard Biesheuvel FTRACE's function tracer currently doesn't always work on ARM with MODULE_PLT option enabled. If the module is loaded too far, FTRACE's code modifier cannot cope with introduced veneers and turns the function tracer off globally. ARM64 already has a solution for the problem, refer to the following patches: arm64: ftrace: emit ftrace-mod.o contents through code arm64: module-plts: factor out PLT generation code for ftrace arm64: ftrace: fix !CONFIG_ARM64_MODULE_PLTS kernels arm64: ftrace: fix building without CONFIG_MODULES arm64: ftrace: add support for far branches to dynamic ftrace arm64: ftrace: don't validate branch via PLT in ftrace_make_nop() But the presented ARM variant has just a half of the footprint in terms of the changed LoCs. It also retains the code validation-before-modification instead of switching it off. Changelog: v4: * Fixed build without CONFIG_FUNCTION_TRACER * Reorganized pre-allocated PLTs handling in get_module_plt(), now compiler eliminates the whole FTRACE-related handling code if ARRAY_SIZE(fixed_plts) == 0 v3: * Only extend struct dyn_arch_ftrace when ARM_MODULE_PLTS is enabled v2: * As suggested by Steven Rostedt, refrain from tree-wide API modification, save module pointer in struct dyn_arch_ftrace instead (PowerPC way) Alexander Sverdlin (2): ARM: PLT: Move struct plt_entries definition to header ARM: ftrace: Add MODULE_PLTS support arch/arm/include/asm/ftrace.h | 3 +++ arch/arm/include/asm/module.h | 10 +++++++ arch/arm/kernel/ftrace.c | 62 ++++++++++++++++++++++++++++++++++++------- arch/arm/kernel/module-plts.c | 52 ++++++++++++++++++++++++++++-------- 4 files changed, 107 insertions(+), 20 deletions(-) -- 2.4.6 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 1/2] ARM: PLT: Move struct plt_entries definition to header 2018-03-13 13:53 [PATCH v4 0/2] ARM: Implement MODULE_PLT support in FTRACE Alexander Sverdlin @ 2018-03-13 13:53 ` Alexander Sverdlin 2018-03-13 13:53 ` [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support Alexander Sverdlin 1 sibling, 0 replies; 14+ messages in thread From: Alexander Sverdlin @ 2018-03-13 13:53 UTC (permalink / raw) To: linux-arm-kernel Cc: Alexander Sverdlin, linux-kernel, Russell King, Steven Rostedt, Ingo Molnar, Ard Biesheuvel No functional change, later it will be re-used in several files. Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> --- arch/arm/include/asm/module.h | 9 +++++++++ arch/arm/kernel/module-plts.c | 9 --------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h index 89ad059..6996405 100644 --- a/arch/arm/include/asm/module.h +++ b/arch/arm/include/asm/module.h @@ -19,6 +19,15 @@ enum { }; #endif +#define PLT_ENT_STRIDE L1_CACHE_BYTES +#define PLT_ENT_COUNT (PLT_ENT_STRIDE / sizeof(u32)) +#define PLT_ENT_SIZE (sizeof(struct plt_entries) / PLT_ENT_COUNT) + +struct plt_entries { + u32 ldr[PLT_ENT_COUNT]; + u32 lit[PLT_ENT_COUNT]; +}; + struct mod_plt_sec { struct elf32_shdr *plt; int plt_count; diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c index 3d0c2e4..f272711 100644 --- a/arch/arm/kernel/module-plts.c +++ b/arch/arm/kernel/module-plts.c @@ -14,10 +14,6 @@ #include <asm/cache.h> #include <asm/opcodes.h> -#define PLT_ENT_STRIDE L1_CACHE_BYTES -#define PLT_ENT_COUNT (PLT_ENT_STRIDE / sizeof(u32)) -#define PLT_ENT_SIZE (sizeof(struct plt_entries) / PLT_ENT_COUNT) - #ifdef CONFIG_THUMB2_KERNEL #define PLT_ENT_LDR __opcode_to_mem_thumb32(0xf8dff000 | \ (PLT_ENT_STRIDE - 4)) @@ -26,11 +22,6 @@ (PLT_ENT_STRIDE - 8)) #endif -struct plt_entries { - u32 ldr[PLT_ENT_COUNT]; - u32 lit[PLT_ENT_COUNT]; -}; - static bool in_init(const struct module *mod, unsigned long loc) { return loc - (u32)mod->init_layout.base < mod->init_layout.size; -- 2.4.6 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 13:53 [PATCH v4 0/2] ARM: Implement MODULE_PLT support in FTRACE Alexander Sverdlin 2018-03-13 13:53 ` [PATCH v4 1/2] ARM: PLT: Move struct plt_entries definition to header Alexander Sverdlin @ 2018-03-13 13:53 ` Alexander Sverdlin 2018-03-13 16:12 ` Ard Biesheuvel 1 sibling, 1 reply; 14+ messages in thread From: Alexander Sverdlin @ 2018-03-13 13:53 UTC (permalink / raw) To: linux-arm-kernel Cc: Alexander Sverdlin, linux-kernel, Russell King, Steven Rostedt, Ingo Molnar, Ard Biesheuvel Teach ftrace_make_call() and ftrace_make_nop() about PLTs. Teach PLT code about FTRACE and all its callbacks. Otherwise the following might happen: ------------[ cut here ]------------ WARNING: CPU: 14 PID: 2265 at .../arch/arm/kernel/insn.c:14 __arm_gen_branch+0x83/0x8c() ... Hardware name: LSI Axxia AXM55XX [<c0314a49>] (unwind_backtrace) from [<c03115e9>] (show_stack+0x11/0x14) [<c03115e9>] (show_stack) from [<c0519f51>] (dump_stack+0x81/0xa8) [<c0519f51>] (dump_stack) from [<c032185d>] (warn_slowpath_common+0x69/0x90) [<c032185d>] (warn_slowpath_common) from [<c03218f3>] (warn_slowpath_null+0x17/0x1c) [<c03218f3>] (warn_slowpath_null) from [<c03143cf>] (__arm_gen_branch+0x83/0x8c) [<c03143cf>] (__arm_gen_branch) from [<c0314337>] (ftrace_make_nop+0xf/0x24) [<c0314337>] (ftrace_make_nop) from [<c038ebcb>] (ftrace_process_locs+0x27b/0x3e8) [<c038ebcb>] (ftrace_process_locs) from [<c0378d79>] (load_module+0x11e9/0x1a44) [<c0378d79>] (load_module) from [<c037974d>] (SyS_finit_module+0x59/0x84) [<c037974d>] (SyS_finit_module) from [<c030e981>] (ret_fast_syscall+0x1/0x18) ---[ end trace e1b64ced7a89adcc ]--- ------------[ cut here ]------------ WARNING: CPU: 14 PID: 2265 at .../kernel/trace/ftrace.c:1979 ftrace_bug+0x1b1/0x234() ... Hardware name: LSI Axxia AXM55XX [<c0314a49>] (unwind_backtrace) from [<c03115e9>] (show_stack+0x11/0x14) [<c03115e9>] (show_stack) from [<c0519f51>] (dump_stack+0x81/0xa8) [<c0519f51>] (dump_stack) from [<c032185d>] (warn_slowpath_common+0x69/0x90) [<c032185d>] (warn_slowpath_common) from [<c03218f3>] (warn_slowpath_null+0x17/0x1c) [<c03218f3>] (warn_slowpath_null) from [<c038e87d>] (ftrace_bug+0x1b1/0x234) [<c038e87d>] (ftrace_bug) from [<c038ebd5>] (ftrace_process_locs+0x285/0x3e8) [<c038ebd5>] (ftrace_process_locs) from [<c0378d79>] (load_module+0x11e9/0x1a44) [<c0378d79>] (load_module) from [<c037974d>] (SyS_finit_module+0x59/0x84) [<c037974d>] (SyS_finit_module) from [<c030e981>] (ret_fast_syscall+0x1/0x18) ---[ end trace e1b64ced7a89adcd ]--- ftrace failed to modify [<e9ef7006>] 0xe9ef7006 actual: 02:f0:3b:fa ftrace record flags: 0 (0) expected tramp: c0314265 Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> --- arch/arm/include/asm/ftrace.h | 3 +++ arch/arm/include/asm/module.h | 1 + arch/arm/kernel/ftrace.c | 62 ++++++++++++++++++++++++++++++++++++------- arch/arm/kernel/module-plts.c | 47 +++++++++++++++++++++++++++++--- 4 files changed, 100 insertions(+), 13 deletions(-) diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h index 9e842ff..faeb6b1 100644 --- a/arch/arm/include/asm/ftrace.h +++ b/arch/arm/include/asm/ftrace.h @@ -19,6 +19,9 @@ struct dyn_arch_ftrace { #ifdef CONFIG_OLD_MCOUNT bool old_mcount; #endif +#ifdef CONFIG_ARM_MODULE_PLTS + struct module *mod; +#endif }; static inline unsigned long ftrace_call_adjust(unsigned long addr) diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h index 6996405..e3d7a51 100644 --- a/arch/arm/include/asm/module.h +++ b/arch/arm/include/asm/module.h @@ -30,6 +30,7 @@ struct plt_entries { struct mod_plt_sec { struct elf32_shdr *plt; + struct plt_entries *plt_ent; int plt_count; }; diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c index 5617932..b55355f 100644 --- a/arch/arm/kernel/ftrace.c +++ b/arch/arm/kernel/ftrace.c @@ -98,6 +98,19 @@ int ftrace_arch_code_modify_post_process(void) static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr) { + s32 offset = addr - pc; + s32 blim = 0xfe000008; + s32 flim = 0x02000004; + + if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) { + blim = 0xff000004; + flim = 0x01000002; + } + + if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS) && + (offset < blim || offset > flim)) + return 0; + return arm_gen_branch_link(pc, addr); } @@ -166,10 +179,22 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) { unsigned long new, old; unsigned long ip = rec->ip; + unsigned long aaddr = adjust_address(rec, addr); old = ftrace_nop_replace(rec); - new = ftrace_call_replace(ip, adjust_address(rec, addr)); + new = ftrace_call_replace(ip, aaddr); + +#ifdef CONFIG_ARM_MODULE_PLTS + if (!new) { + struct module *mod = rec->arch.mod; + + if (mod) { + aaddr = get_module_plt(mod, ip, aaddr); + new = ftrace_call_replace(ip, aaddr); + } + } +#endif return ftrace_modify_code(rec->ip, old, new, true); } @@ -199,20 +224,39 @@ int ftrace_make_nop(struct module *mod, unsigned long new; int ret; - old = ftrace_call_replace(ip, adjust_address(rec, addr)); - new = ftrace_nop_replace(rec); - ret = ftrace_modify_code(ip, old, new, true); +#ifdef CONFIG_ARM_MODULE_PLTS + /* mod is only supplied during module loading */ + if (!mod) + mod = rec->arch.mod; + else + rec->arch.mod = mod; +#endif -#ifdef CONFIG_OLD_MCOUNT - if (ret == -EINVAL && addr == MCOUNT_ADDR) { - rec->arch.old_mcount = true; + for (;;) { + unsigned long aaddr = adjust_address(rec, addr); + + old = ftrace_call_replace(ip, aaddr); + +#ifdef CONFIG_ARM_MODULE_PLTS + if (!old && mod) { + aaddr = get_module_plt(mod, ip, aaddr); + old = ftrace_call_replace(ip, aaddr); + } +#endif - old = ftrace_call_replace(ip, adjust_address(rec, addr)); new = ftrace_nop_replace(rec); ret = ftrace_modify_code(ip, old, new, true); - } + +#ifdef CONFIG_OLD_MCOUNT + if (ret == -EINVAL && !rec->arch.old_mcount) { + rec->arch.old_mcount = true; + continue; + } #endif + break; + } + return ret; } diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c index f272711..0951270 100644 --- a/arch/arm/kernel/module-plts.c +++ b/arch/arm/kernel/module-plts.c @@ -7,6 +7,7 @@ */ #include <linux/elf.h> +#include <linux/ftrace.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/sort.h> @@ -22,18 +23,54 @@ (PLT_ENT_STRIDE - 8)) #endif +static const u32 fixed_plts[] = { +#ifdef CONFIG_FUNCTION_TRACER + FTRACE_ADDR, + MCOUNT_ADDR, +#ifdef CONFIG_OLD_MCOUNT + (unsigned long)ftrace_caller_old, + (unsigned long)mcount, +#endif +#endif +}; + static bool in_init(const struct module *mod, unsigned long loc) { return loc - (u32)mod->init_layout.base < mod->init_layout.size; } +static void prealloc_fixed(struct mod_plt_sec *pltsec, struct plt_entries *plt) +{ + int i; + + if (!ARRAY_SIZE(fixed_plts)) + return; + + for (i = 0; i < ARRAY_SIZE(plt->ldr); ++i) + plt->ldr[i] = PLT_ENT_LDR; + memcpy(plt->lit, fixed_plts, sizeof(fixed_plts)); + pltsec->plt_count = ARRAY_SIZE(fixed_plts); +} + u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val) { struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core : &mod->arch.init; + struct plt_entries *plt; + int idx; - struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr; - int idx = 0; + /* cache the address, ELF header is available only during module load */ + if (!pltsec->plt_ent) + pltsec->plt_ent = (struct plt_entries *)pltsec->plt->sh_addr; + plt = pltsec->plt_ent; + + if (!pltsec->plt_count) + prealloc_fixed(pltsec, plt); + + idx = ARRAY_SIZE(fixed_plts); + while (idx) + if (plt->lit[--idx] == val) + return (u32)&plt->ldr[idx]; /* * Look for an existing entry pointing to 'val'. Given that the @@ -182,8 +219,8 @@ static unsigned int count_plts(const Elf32_Sym *syms, Elf32_Addr base, int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, char *secstrings, struct module *mod) { - unsigned long core_plts = 0; - unsigned long init_plts = 0; + unsigned long core_plts = ARRAY_SIZE(fixed_plts); + unsigned long init_plts = ARRAY_SIZE(fixed_plts); Elf32_Shdr *s, *sechdrs_end = sechdrs + ehdr->e_shnum; Elf32_Sym *syms = NULL; @@ -238,6 +275,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, mod->arch.core.plt->sh_size = round_up(core_plts * PLT_ENT_SIZE, sizeof(struct plt_entries)); mod->arch.core.plt_count = 0; + mod->arch.core.plt_ent = NULL; mod->arch.init.plt->sh_type = SHT_NOBITS; mod->arch.init.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC; @@ -245,6 +283,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, mod->arch.init.plt->sh_size = round_up(init_plts * PLT_ENT_SIZE, sizeof(struct plt_entries)); mod->arch.init.plt_count = 0; + mod->arch.init.plt_ent = NULL; pr_debug("%s: plt=%x, init.plt=%x\n", __func__, mod->arch.core.plt->sh_size, mod->arch.init.plt->sh_size); -- 2.4.6 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 13:53 ` [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support Alexander Sverdlin @ 2018-03-13 16:12 ` Ard Biesheuvel 2018-03-13 17:13 ` Alexander Sverdlin 0 siblings, 1 reply; 14+ messages in thread From: Ard Biesheuvel @ 2018-03-13 16:12 UTC (permalink / raw) To: Alexander Sverdlin Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar On 13 March 2018 at 13:53, Alexander Sverdlin <alexander.sverdlin@nokia.com> wrote: > Teach ftrace_make_call() and ftrace_make_nop() about PLTs. > Teach PLT code about FTRACE and all its callbacks. > Otherwise the following might happen: > > ------------[ cut here ]------------ > WARNING: CPU: 14 PID: 2265 at .../arch/arm/kernel/insn.c:14 __arm_gen_branch+0x83/0x8c() > ... > Hardware name: LSI Axxia AXM55XX > [<c0314a49>] (unwind_backtrace) from [<c03115e9>] (show_stack+0x11/0x14) > [<c03115e9>] (show_stack) from [<c0519f51>] (dump_stack+0x81/0xa8) > [<c0519f51>] (dump_stack) from [<c032185d>] (warn_slowpath_common+0x69/0x90) > [<c032185d>] (warn_slowpath_common) from [<c03218f3>] (warn_slowpath_null+0x17/0x1c) > [<c03218f3>] (warn_slowpath_null) from [<c03143cf>] (__arm_gen_branch+0x83/0x8c) > [<c03143cf>] (__arm_gen_branch) from [<c0314337>] (ftrace_make_nop+0xf/0x24) > [<c0314337>] (ftrace_make_nop) from [<c038ebcb>] (ftrace_process_locs+0x27b/0x3e8) > [<c038ebcb>] (ftrace_process_locs) from [<c0378d79>] (load_module+0x11e9/0x1a44) > [<c0378d79>] (load_module) from [<c037974d>] (SyS_finit_module+0x59/0x84) > [<c037974d>] (SyS_finit_module) from [<c030e981>] (ret_fast_syscall+0x1/0x18) > ---[ end trace e1b64ced7a89adcc ]--- > ------------[ cut here ]------------ > WARNING: CPU: 14 PID: 2265 at .../kernel/trace/ftrace.c:1979 ftrace_bug+0x1b1/0x234() > ... > Hardware name: LSI Axxia AXM55XX > [<c0314a49>] (unwind_backtrace) from [<c03115e9>] (show_stack+0x11/0x14) > [<c03115e9>] (show_stack) from [<c0519f51>] (dump_stack+0x81/0xa8) > [<c0519f51>] (dump_stack) from [<c032185d>] (warn_slowpath_common+0x69/0x90) > [<c032185d>] (warn_slowpath_common) from [<c03218f3>] (warn_slowpath_null+0x17/0x1c) > [<c03218f3>] (warn_slowpath_null) from [<c038e87d>] (ftrace_bug+0x1b1/0x234) > [<c038e87d>] (ftrace_bug) from [<c038ebd5>] (ftrace_process_locs+0x285/0x3e8) > [<c038ebd5>] (ftrace_process_locs) from [<c0378d79>] (load_module+0x11e9/0x1a44) > [<c0378d79>] (load_module) from [<c037974d>] (SyS_finit_module+0x59/0x84) > [<c037974d>] (SyS_finit_module) from [<c030e981>] (ret_fast_syscall+0x1/0x18) > ---[ end trace e1b64ced7a89adcd ]--- > ftrace failed to modify [<e9ef7006>] 0xe9ef7006 > actual: 02:f0:3b:fa > ftrace record flags: 0 > (0) expected tramp: c0314265 > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > --- > arch/arm/include/asm/ftrace.h | 3 +++ > arch/arm/include/asm/module.h | 1 + > arch/arm/kernel/ftrace.c | 62 ++++++++++++++++++++++++++++++++++++------- > arch/arm/kernel/module-plts.c | 47 +++++++++++++++++++++++++++++--- > 4 files changed, 100 insertions(+), 13 deletions(-) > ... > diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c > index f272711..0951270 100644 > --- a/arch/arm/kernel/module-plts.c > +++ b/arch/arm/kernel/module-plts.c > @@ -7,6 +7,7 @@ > */ > > #include <linux/elf.h> > +#include <linux/ftrace.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/sort.h> > @@ -22,18 +23,54 @@ > (PLT_ENT_STRIDE - 8)) > #endif > > +static const u32 fixed_plts[] = { > +#ifdef CONFIG_FUNCTION_TRACER > + FTRACE_ADDR, > + MCOUNT_ADDR, > +#ifdef CONFIG_OLD_MCOUNT > + (unsigned long)ftrace_caller_old, > + (unsigned long)mcount, > +#endif > +#endif > +}; > + > static bool in_init(const struct module *mod, unsigned long loc) > { > return loc - (u32)mod->init_layout.base < mod->init_layout.size; > } > > +static void prealloc_fixed(struct mod_plt_sec *pltsec, struct plt_entries *plt) > +{ > + int i; > + > + if (!ARRAY_SIZE(fixed_plts)) > + return; > + > + for (i = 0; i < ARRAY_SIZE(plt->ldr); ++i) > + plt->ldr[i] = PLT_ENT_LDR; > + memcpy(plt->lit, fixed_plts, sizeof(fixed_plts)); This is slightly dodgy. You are assuming that sizeof(plt->lit) >= sizeof(fixed_plts), which may change depending on configuration or future changes. Could you add a BUILD_BUG_ON() here to ensure that this remains the case? > + pltsec->plt_count = ARRAY_SIZE(fixed_plts); > +} > + > u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val) > { > struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core : > &mod->arch.init; > + struct plt_entries *plt; > + int idx; > > - struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr; > - int idx = 0; > + /* cache the address, ELF header is available only during module load */ > + if (!pltsec->plt_ent) > + pltsec->plt_ent = (struct plt_entries *)pltsec->plt->sh_addr; > + plt = pltsec->plt_ent; > + Where is plt_ent ever used? > + if (!pltsec->plt_count) > + prealloc_fixed(pltsec, plt); > + Please move the if () check into prealloc_fixed(), and only keep the loop below > + idx = ARRAY_SIZE(fixed_plts); > + while (idx) > + if (plt->lit[--idx] == val) > + return (u32)&plt->ldr[idx]; Please use a normal for loop here and iterate upward starting at 0 > > /* > * Look for an existing entry pointing to 'val'. Given that the > @@ -182,8 +219,8 @@ static unsigned int count_plts(const Elf32_Sym *syms, Elf32_Addr base, > int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, > char *secstrings, struct module *mod) > { > - unsigned long core_plts = 0; > - unsigned long init_plts = 0; > + unsigned long core_plts = ARRAY_SIZE(fixed_plts); > + unsigned long init_plts = ARRAY_SIZE(fixed_plts); > Elf32_Shdr *s, *sechdrs_end = sechdrs + ehdr->e_shnum; > Elf32_Sym *syms = NULL; > > @@ -238,6 +275,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, > mod->arch.core.plt->sh_size = round_up(core_plts * PLT_ENT_SIZE, > sizeof(struct plt_entries)); > mod->arch.core.plt_count = 0; > + mod->arch.core.plt_ent = NULL; > > mod->arch.init.plt->sh_type = SHT_NOBITS; > mod->arch.init.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC; > @@ -245,6 +283,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, > mod->arch.init.plt->sh_size = round_up(init_plts * PLT_ENT_SIZE, > sizeof(struct plt_entries)); > mod->arch.init.plt_count = 0; > + mod->arch.init.plt_ent = NULL; > > pr_debug("%s: plt=%x, init.plt=%x\n", __func__, > mod->arch.core.plt->sh_size, mod->arch.init.plt->sh_size); > -- > 2.4.6 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 16:12 ` Ard Biesheuvel @ 2018-03-13 17:13 ` Alexander Sverdlin 2018-03-13 17:18 ` Ard Biesheuvel 0 siblings, 1 reply; 14+ messages in thread From: Alexander Sverdlin @ 2018-03-13 17:13 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar Hello Ard, On 13/03/18 17:12, Ard Biesheuvel wrote: >> u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val) >> { >> struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core : >> &mod->arch.init; >> + struct plt_entries *plt; >> + int idx; >> >> - struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr; ^^^^^^^^^^^ (*) >> - int idx = 0; >> + /* cache the address, ELF header is available only during module load */ >> + if (!pltsec->plt_ent) >> + pltsec->plt_ent = (struct plt_entries *)pltsec->plt->sh_addr; >> + plt = pltsec->plt_ent; >> + > Where is plt_ent ever used? Above is exactly the place it's used. I need to cache it because after the module load is finished the ELF header is freed, pltsec->plt pointer (*) is not valid any more. With the above modification it's possible to call the function during the whole life time of the module. >> + if (!pltsec->plt_count) >> + prealloc_fixed(pltsec, plt); I'll prepare v5 based on your other comments. -- Best regards, Alexander Sverdlin. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 17:13 ` Alexander Sverdlin @ 2018-03-13 17:18 ` Ard Biesheuvel 2018-03-13 17:32 ` Alexander Sverdlin 0 siblings, 1 reply; 14+ messages in thread From: Ard Biesheuvel @ 2018-03-13 17:18 UTC (permalink / raw) To: Alexander Sverdlin Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar On 13 March 2018 at 17:13, Alexander Sverdlin <alexander.sverdlin@nokia.com> wrote: > Hello Ard, > > On 13/03/18 17:12, Ard Biesheuvel wrote: >>> u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val) >>> { >>> struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core : >>> &mod->arch.init; >>> + struct plt_entries *plt; >>> + int idx; >>> >>> - struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr; > ^^^^^^^^^^^ (*) > >>> - int idx = 0; >>> + /* cache the address, ELF header is available only during module load */ >>> + if (!pltsec->plt_ent) >>> + pltsec->plt_ent = (struct plt_entries *)pltsec->plt->sh_addr; >>> + plt = pltsec->plt_ent; >>> + >> Where is plt_ent ever used? > > Above is exactly the place it's used. > I need to cache it because after the module load is finished the ELF header is freed, > pltsec->plt pointer (*) is not valid any more. > With the above modification it's possible to call the function during the whole life > time of the module. > Right, ok. That's a problem. This means that you are relying on get_module_plt() being called at least once at module load time, which is not guaranteed. Instead, you should set this member (and perhaps the entire prealloc routine) in a module_finalize() implementation. >>> + if (!pltsec->plt_count) >>> + prealloc_fixed(pltsec, plt); > > I'll prepare v5 based on your other comments. > > -- > Best regards, > Alexander Sverdlin. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 17:18 ` Ard Biesheuvel @ 2018-03-13 17:32 ` Alexander Sverdlin 2018-03-13 17:39 ` Ard Biesheuvel 2018-03-20 12:28 ` Alexander Sverdlin 0 siblings, 2 replies; 14+ messages in thread From: Alexander Sverdlin @ 2018-03-13 17:32 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar Hi Ard! On 13/03/18 18:18, Ard Biesheuvel wrote: >>>> u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val) >>>> { >>>> struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core : >>>> &mod->arch.init; >>>> + struct plt_entries *plt; >>>> + int idx; >>>> >>>> - struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr; >> ^^^^^^^^^^^ (*) >> >>>> - int idx = 0; >>>> + /* cache the address, ELF header is available only during module load */ >>>> + if (!pltsec->plt_ent) >>>> + pltsec->plt_ent = (struct plt_entries *)pltsec->plt->sh_addr; >>>> + plt = pltsec->plt_ent; >>>> + >>> Where is plt_ent ever used? >> Above is exactly the place it's used. >> I need to cache it because after the module load is finished the ELF header is freed, >> pltsec->plt pointer (*) is not valid any more. >> With the above modification it's possible to call the function during the whole life >> time of the module. >> > Right, ok. That's a problem. > > This means that you are relying on get_module_plt() being called at > least once at module load time, which is not guaranteed. This is indeed guaranteed. For FTRACE use case. If it's being called from FTRACE in run time, this would mean there were long calls in this module section, which in turn means, get_module_plt() was called at least once for this module and this section. This doesn't hold in general, though. In any case, if you insist, I can try to rework the whole stuff implementing module_finalize(). -- Best regards, Alexander Sverdlin. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 17:32 ` Alexander Sverdlin @ 2018-03-13 17:39 ` Ard Biesheuvel 2018-03-13 17:49 ` Alexander Sverdlin 2018-03-13 18:28 ` Alexander Sverdlin 2018-03-20 12:28 ` Alexander Sverdlin 1 sibling, 2 replies; 14+ messages in thread From: Ard Biesheuvel @ 2018-03-13 17:39 UTC (permalink / raw) To: Alexander Sverdlin Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar On 13 March 2018 at 17:32, Alexander Sverdlin <alexander.sverdlin@nokia.com> wrote: > Hi Ard! > > On 13/03/18 18:18, Ard Biesheuvel wrote: >>>>> u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val) >>>>> { >>>>> struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core : >>>>> &mod->arch.init; >>>>> + struct plt_entries *plt; >>>>> + int idx; >>>>> >>>>> - struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr; >>> ^^^^^^^^^^^ (*) >>> >>>>> - int idx = 0; >>>>> + /* cache the address, ELF header is available only during module load */ >>>>> + if (!pltsec->plt_ent) >>>>> + pltsec->plt_ent = (struct plt_entries *)pltsec->plt->sh_addr; >>>>> + plt = pltsec->plt_ent; >>>>> + >>>> Where is plt_ent ever used? >>> Above is exactly the place it's used. >>> I need to cache it because after the module load is finished the ELF header is freed, >>> pltsec->plt pointer (*) is not valid any more. >>> With the above modification it's possible to call the function during the whole life >>> time of the module. >>> >> Right, ok. That's a problem. >> >> This means that you are relying on get_module_plt() being called at >> least once at module load time, which is not guaranteed. > > This is indeed guaranteed. For FTRACE use case. If it's being called from FTRACE in > run time, this would mean there were long calls in this module section, which in > turn means, get_module_plt() was called at least once for this module and this > section. > > This doesn't hold in general, though. > > In any case, if you insist, I can try to rework the whole stuff implementing module_finalize(). > I think it would be much better to use the module_finalize() hook for this, given that it is only called once already, and all the data you need is still available. Note that ARM already has such a function, so you'll need to add a call there, i.e., if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS)) module_plt_alloc_fixed(); or something like that. The CONFIG_FTRACE dependency can be kept local to module-plts.c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 17:39 ` Ard Biesheuvel @ 2018-03-13 17:49 ` Alexander Sverdlin 2018-03-13 17:51 ` Ard Biesheuvel 2018-03-13 18:28 ` Alexander Sverdlin 1 sibling, 1 reply; 14+ messages in thread From: Alexander Sverdlin @ 2018-03-13 17:49 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar On 13/03/18 18:39, Ard Biesheuvel wrote: > if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS)) > module_plt_alloc_fixed(); Do you consider this a legal C code if without module-plts.o the function would not exist at all? That's too much relying on optimizer I think... -- Best regards, Alexander Sverdlin. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 17:49 ` Alexander Sverdlin @ 2018-03-13 17:51 ` Ard Biesheuvel 2018-03-13 18:24 ` Alexander Sverdlin 0 siblings, 1 reply; 14+ messages in thread From: Ard Biesheuvel @ 2018-03-13 17:51 UTC (permalink / raw) To: Alexander Sverdlin Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar On 13 March 2018 at 17:49, Alexander Sverdlin <alexander.sverdlin@nokia.com> wrote: > On 13/03/18 18:39, Ard Biesheuvel wrote: >> if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS)) >> module_plt_alloc_fixed(); > > Do you consider this a legal C code if without module-plts.o the function would not exist at all? > That's too much relying on optimizer I think... > Yes, we rely on that in many different places in the kernel. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 17:51 ` Ard Biesheuvel @ 2018-03-13 18:24 ` Alexander Sverdlin 2018-03-13 18:29 ` Ard Biesheuvel 0 siblings, 1 reply; 14+ messages in thread From: Alexander Sverdlin @ 2018-03-13 18:24 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar Hi! On 13/03/18 18:51, Ard Biesheuvel wrote: >>> if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS)) >>> module_plt_alloc_fixed(); >> Do you consider this a legal C code if without module-plts.o the function would not exist at all? >> That's too much relying on optimizer I think... >> > Yes, we rely on that in many different places in the kernel. https://www.kernel.org/doc/Documentation/process/coding-style.rst: "However, this approach still allows the C compiler to see the code inside the block, and check it for correctness (syntax, types, symbol references, etc). Thus, you still have to use an #ifdef if the code inside the block references symbols that will not exist if the condition is not met." But we can of course ignore it. -- Best regards, Alexander Sverdlin. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 18:24 ` Alexander Sverdlin @ 2018-03-13 18:29 ` Ard Biesheuvel 0 siblings, 0 replies; 14+ messages in thread From: Ard Biesheuvel @ 2018-03-13 18:29 UTC (permalink / raw) To: Alexander Sverdlin Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar On 13 March 2018 at 18:24, Alexander Sverdlin <alexander.sverdlin@nokia.com> wrote: > Hi! > > On 13/03/18 18:51, Ard Biesheuvel wrote: >>>> if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS)) >>>> module_plt_alloc_fixed(); >>> Do you consider this a legal C code if without module-plts.o the function would not exist at all? >>> That's too much relying on optimizer I think... >>> >> Yes, we rely on that in many different places in the kernel. > > https://www.kernel.org/doc/Documentation/process/coding-style.rst: > "However, this approach still allows the C compiler to see the code > inside the block, and check it for correctness (syntax, types, symbol > references, etc). Thus, you still have to use an #ifdef if the code inside the > block references symbols that will not exist if the condition is not met." > "will not exist" is ambiguous here. It is rather common to declare symbols, but only define them conditionally, and use IS_ENABLED() to refer to them. As the documentation says, this gets rid of #ifdefs, making the code always visible to the compiler which is a good thing. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 17:39 ` Ard Biesheuvel 2018-03-13 17:49 ` Alexander Sverdlin @ 2018-03-13 18:28 ` Alexander Sverdlin 1 sibling, 0 replies; 14+ messages in thread From: Alexander Sverdlin @ 2018-03-13 18:28 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar Hi! On 13/03/18 18:39, Ard Biesheuvel wrote: >>>>>> u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val) >>>>>> { >>>>>> struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core : >>>>>> &mod->arch.init; >>>>>> + struct plt_entries *plt; >>>>>> + int idx; >>>>>> >>>>>> - struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr; >>>> ^^^^^^^^^^^ (*) >>>> >>>>>> - int idx = 0; >>>>>> + /* cache the address, ELF header is available only during module load */ >>>>>> + if (!pltsec->plt_ent) >>>>>> + pltsec->plt_ent = (struct plt_entries *)pltsec->plt->sh_addr; >>>>>> + plt = pltsec->plt_ent; >>>>>> + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (*) >>>>> Where is plt_ent ever used? >>>> Above is exactly the place it's used. >>>> I need to cache it because after the module load is finished the ELF header is freed, >>>> pltsec->plt pointer (*) is not valid any more. >>>> With the above modification it's possible to call the function during the whole life >>>> time of the module. >>>> >>> Right, ok. That's a problem. >>> >>> This means that you are relying on get_module_plt() being called at >>> least once at module load time, which is not guaranteed. >> This is indeed guaranteed. For FTRACE use case. If it's being called from FTRACE in >> run time, this would mean there were long calls in this module section, which in >> turn means, get_module_plt() was called at least once for this module and this >> section. >> >> This doesn't hold in general, though. >> >> In any case, if you insist, I can try to rework the whole stuff implementing module_finalize(). >> > I think it would be much better to use the module_finalize() hook for > this, given that it is only called once already, and all the data you > need is still available. No problem, but some kind of (*) block would still be required, because get_module_plt() has to work after module_finalize() as well as *before* it. So before module_finalize() we would have to dereference pltsec->plt->sh_addr conditionally. > Note that ARM already has such a function, so you'll need to add a > call there, i.e., > > if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS)) > module_plt_alloc_fixed(); > > or something like that. The CONFIG_FTRACE dependency can be kept local > to module-plts.c > -- Best regards, Alexander Sverdlin. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support 2018-03-13 17:32 ` Alexander Sverdlin 2018-03-13 17:39 ` Ard Biesheuvel @ 2018-03-20 12:28 ` Alexander Sverdlin 1 sibling, 0 replies; 14+ messages in thread From: Alexander Sverdlin @ 2018-03-20 12:28 UTC (permalink / raw) To: Ard Biesheuvel Cc: linux-arm-kernel, Linux Kernel Mailing List, Russell King, Steven Rostedt, Ingo Molnar Hello Ard, On 13/03/18 18:32, Alexander Sverdlin wrote: >>>>> u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val) >>>>> { >>>>> struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core : >>>>> &mod->arch.init; >>>>> + struct plt_entries *plt; >>>>> + int idx; >>>>> >>>>> - struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr; >>> ^^^^^^^^^^^ (*) >>> >>>>> - int idx = 0; >>>>> + /* cache the address, ELF header is available only during module load */ >>>>> + if (!pltsec->plt_ent) >>>>> + pltsec->plt_ent = (struct plt_entries *)pltsec->plt->sh_addr; >>>>> + plt = pltsec->plt_ent; >>>>> + >>>> Where is plt_ent ever used? >>> Above is exactly the place it's used. >>> I need to cache it because after the module load is finished the ELF header is freed, >>> pltsec->plt pointer (*) is not valid any more. >>> With the above modification it's possible to call the function during the whole life >>> time of the module. >>> >> Right, ok. That's a problem. >> >> This means that you are relying on get_module_plt() being called at >> least once at module load time, which is not guaranteed. > This is indeed guaranteed. For FTRACE use case. If it's being called from FTRACE in > run time, this would mean there were long calls in this module section, which in > turn means, get_module_plt() was called at least once for this module and this > section. > > This doesn't hold in general, though. > > In any case, if you insist, I can try to rework the whole stuff implementing module_finalize(). now when I have a new implementation via module_finalize(), I must admit it's not possible to do it sanely this way. module_finalize() can only add entries at the end of PLT, which means, they will be different from the entries module loader/relocator has created before, which means, FTRACE will not be able to replace these entries with NOPs. As I don't want to do O(N) search on every dynamic ftrace operation, seems this is not an option. Either v4 has to be accepted, or I cannot propose a solution for upstream FTRACE+MODULES_PLT combination. -- Best regards, Alexander Sverdlin. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-03-20 12:29 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-13 13:53 [PATCH v4 0/2] ARM: Implement MODULE_PLT support in FTRACE Alexander Sverdlin 2018-03-13 13:53 ` [PATCH v4 1/2] ARM: PLT: Move struct plt_entries definition to header Alexander Sverdlin 2018-03-13 13:53 ` [PATCH v4 2/2] ARM: ftrace: Add MODULE_PLTS support Alexander Sverdlin 2018-03-13 16:12 ` Ard Biesheuvel 2018-03-13 17:13 ` Alexander Sverdlin 2018-03-13 17:18 ` Ard Biesheuvel 2018-03-13 17:32 ` Alexander Sverdlin 2018-03-13 17:39 ` Ard Biesheuvel 2018-03-13 17:49 ` Alexander Sverdlin 2018-03-13 17:51 ` Ard Biesheuvel 2018-03-13 18:24 ` Alexander Sverdlin 2018-03-13 18:29 ` Ard Biesheuvel 2018-03-13 18:28 ` Alexander Sverdlin 2018-03-20 12:28 ` Alexander Sverdlin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).