| Message ID | 1456324115-21144-2-git-send-email-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 25/02/16 01:28, Michael Ellerman wrote: > When a module is loaded, calls out to the kernel go via a stub which is > generated at runtime. One of these stubs is used to call _mcount(), > which is the default target of tracing calls generated by the compiler > with -pg. > > If dynamic ftrace is enabled (which it typicall is), another stub is > used to call ftrace_caller(), which is the target of tracing calls when > ftrace is actually active. > > ftrace then wants to disable the calls to _mcount() at module startup, > and enable/disable the calls to ftrace_caller() when enabling/disablig > tracing - all of these it does by patching the code. > > As part of that code patching, the ftrace code wants to confirm that the > branch it is about to modify, is in fact a call to a module stub which > calls _mcount() or ftrace_caller(). > > Currently it does that by inspecting the instructions and confirming > they are what it expects. Although that works, the code to do it is > pretty intricate because it requires lots of knowledge about the exact > format of the stub. > > We can make that process easier by marking the generated stubs with a > magic value, and then looking for that magic value. Altough this is not > as rigorous as the current method, I believe it is sufficient in > practice. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/include/asm/module.h | 3 +- > arch/powerpc/kernel/ftrace.c | 14 ++----- > arch/powerpc/kernel/module_64.c | 78 +++++++++++++-------------------------- > 3 files changed, 31 insertions(+), 64 deletions(-) > > diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h > index 74d25a749018..5b6b5a427b54 100644 > --- a/arch/powerpc/include/asm/module.h > +++ b/arch/powerpc/include/asm/module.h > @@ -78,8 +78,7 @@ struct mod_arch_specific { > # endif /* MODULE */ > #endif > > -bool is_module_trampoline(u32 *insns); > -int module_trampoline_target(struct module *mod, u32 *trampoline, > +int module_trampoline_target(struct module *mod, unsigned long trampoline, > unsigned long *target); > > #ifdef CONFIG_DYNAMIC_FTRACE > diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c > index 44d4d8eb3c85..4505cbfd0e13 100644 > --- a/arch/powerpc/kernel/ftrace.c > +++ b/arch/powerpc/kernel/ftrace.c > @@ -106,10 +106,9 @@ static int > __ftrace_make_nop(struct module *mod, > struct dyn_ftrace *rec, unsigned long addr) > { > - unsigned int op; > - unsigned long entry, ptr; > + unsigned long entry, ptr, tramp; > unsigned long ip = rec->ip; > - void *tramp; > + unsigned int op; > > /* read where this goes */ > if (probe_kernel_read(&op, (void *)ip, sizeof(int))) > @@ -122,14 +121,9 @@ __ftrace_make_nop(struct module *mod, > } > > /* lets find where the pointer goes */ > - tramp = (void *)find_bl_target(ip, op); > - > - pr_devel("ip:%lx jumps to %p", ip, tramp); > + tramp = find_bl_target(ip, op); > > - if (!is_module_trampoline(tramp)) { > - pr_err("Not a trampoline\n"); > - return -EINVAL; > - } > + pr_devel("ip:%lx jumps to %lx", ip, tramp); > > if (module_trampoline_target(mod, tramp, &ptr)) { > pr_err("Failed to get trampoline target\n"); > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 599c753c7960..9629966e614b 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -96,6 +96,8 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym) > } > #endif > > +#define STUB_MAGIC 0x73747562 /* stub */ > + > /* Like PPC32, we need little trampolines to do > 24-bit jumps (into > the kernel itself). But on PPC64, these need to be used for every > jump, actually, to reset r2 (TOC+0x8000). */ > @@ -105,7 +107,8 @@ struct ppc64_stub_entry > * need 6 instructions on ABIv2 but we always allocate 7 so > * so we don't have to modify the trampoline load instruction. */ > u32 jump[7]; > - u32 unused; > + /* Used by ftrace to identify stubs */ > + u32 magic; > /* Data for the above code */ > func_desc_t funcdata; > }; > @@ -139,70 +142,39 @@ static u32 ppc64_stub_insns[] = { > }; > > #ifdef CONFIG_DYNAMIC_FTRACE > - > -static u32 ppc64_stub_mask[] = { > - 0xffff0000, > - 0xffff0000, > - 0xffffffff, > - 0xffffffff, > -#if !defined(_CALL_ELF) || _CALL_ELF != 2 > - 0xffffffff, > -#endif > - 0xffffffff, > - 0xffffffff > -}; > - > -bool is_module_trampoline(u32 *p) > +int module_trampoline_target(struct module *mod, unsigned long addr, > + unsigned long *target) > { > - unsigned int i; > - u32 insns[ARRAY_SIZE(ppc64_stub_insns)]; > - > - BUILD_BUG_ON(sizeof(ppc64_stub_insns) != sizeof(ppc64_stub_mask)); > + struct ppc64_stub_entry *stub; > + func_desc_t funcdata; > + u32 magic; > > - if (probe_kernel_read(insns, p, sizeof(insns))) > + if (!within_module_core(addr, mod)) { > + pr_err("%s: stub %lx not in module %s\n", __func__, addr, mod->name); > return -EFAULT; -EFAULT or -EINVAL? I wonder if we can recover from a bad trampoline address. Reviewed-by: Balbir Singh <bsingharora@gmail.com>
On Thu, 2016-02-25 at 11:04 +1100, Balbir Singh wrote: > > On 25/02/16 01:28, Michael Ellerman wrote: > > -bool is_module_trampoline(u32 *p) > > +int module_trampoline_target(struct module *mod, unsigned long addr, > > + unsigned long *target) > > { > > - unsigned int i; > > - u32 insns[ARRAY_SIZE(ppc64_stub_insns)]; > > - > > - BUILD_BUG_ON(sizeof(ppc64_stub_insns) != sizeof(ppc64_stub_mask)); > > + struct ppc64_stub_entry *stub; > > + func_desc_t funcdata; > > + u32 magic; > > > > - if (probe_kernel_read(insns, p, sizeof(insns))) > > + if (!within_module_core(addr, mod)) { > > + pr_err("%s: stub %lx not in module %s\n", __func__, addr, mod->name); > > return -EFAULT; > -EFAULT or -EINVAL? I think we want EFAULT. Otherwise ftrace_bug() will try and print the actual instruction, which would then fault. (though I haven't confirmed that by testing) > I wonder if we can recover from a bad trampoline address. We can't recover at the moment. Do you mean is there some way we could recover? cheers
On Thu, Feb 25, 2016 at 01:28:25AM +1100, Michael Ellerman wrote: > > We can make that process easier by marking the generated stubs with a > magic value, and then looking for that magic value. Altough this is not > as rigorous as the current method, I believe it is sufficient in > practice. The actual magic value is sort of debatable; it should be "improbable" enough. But this can be changed easily, for each kernel compile, even. > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> Reviewed-by: Torsten Duwe <duwe@suse.de> [for reference:] > > +#define STUB_MAGIC 0x73747562 /* stub */ > + Torsten
On Thu, 2016-02-25 at 14:17 +0100, Torsten Duwe wrote: > On Thu, Feb 25, 2016 at 01:28:25AM +1100, Michael Ellerman wrote: > > > > We can make that process easier by marking the generated stubs with a > > magic value, and then looking for that magic value. Altough this is not > > as rigorous as the current method, I believe it is sufficient in > > practice. > > The actual magic value is sort of debatable; it should be "improbable" > enough. But this can be changed easily, for each kernel compile, even. Yeah. Given the locations we're trying to patch are computed in the first place from the mcount call sites, I feel like we don't need to be super paranoid here. The only time I've heard of this code (the current version) tripping up is when folks are hacking on ftrace. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > Reviewed-by: Torsten Duwe <duwe@suse.de> > > [for reference:] > > +#define STUB_MAGIC 0x73747562 /* stub */ Which is one of: ori r21,r19,29811 andi. r20,r27,30050 Both of which are pretty improbable. They don't appear in any kernel I have around here. I have more plans for this code, which would hopefully mean we can get rid of the magic checking entirely. But I think this is OK for now. cheers
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h index 74d25a749018..5b6b5a427b54 100644 --- a/arch/powerpc/include/asm/module.h +++ b/arch/powerpc/include/asm/module.h @@ -78,8 +78,7 @@ struct mod_arch_specific { # endif /* MODULE */ #endif -bool is_module_trampoline(u32 *insns); -int module_trampoline_target(struct module *mod, u32 *trampoline, +int module_trampoline_target(struct module *mod, unsigned long trampoline, unsigned long *target); #ifdef CONFIG_DYNAMIC_FTRACE diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c index 44d4d8eb3c85..4505cbfd0e13 100644 --- a/arch/powerpc/kernel/ftrace.c +++ b/arch/powerpc/kernel/ftrace.c @@ -106,10 +106,9 @@ static int __ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long addr) { - unsigned int op; - unsigned long entry, ptr; + unsigned long entry, ptr, tramp; unsigned long ip = rec->ip; - void *tramp; + unsigned int op; /* read where this goes */ if (probe_kernel_read(&op, (void *)ip, sizeof(int))) @@ -122,14 +121,9 @@ __ftrace_make_nop(struct module *mod, } /* lets find where the pointer goes */ - tramp = (void *)find_bl_target(ip, op); - - pr_devel("ip:%lx jumps to %p", ip, tramp); + tramp = find_bl_target(ip, op); - if (!is_module_trampoline(tramp)) { - pr_err("Not a trampoline\n"); - return -EINVAL; - } + pr_devel("ip:%lx jumps to %lx", ip, tramp); if (module_trampoline_target(mod, tramp, &ptr)) { pr_err("Failed to get trampoline target\n"); diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index 599c753c7960..9629966e614b 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -96,6 +96,8 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym) } #endif +#define STUB_MAGIC 0x73747562 /* stub */ + /* Like PPC32, we need little trampolines to do > 24-bit jumps (into the kernel itself). But on PPC64, these need to be used for every jump, actually, to reset r2 (TOC+0x8000). */ @@ -105,7 +107,8 @@ struct ppc64_stub_entry * need 6 instructions on ABIv2 but we always allocate 7 so * so we don't have to modify the trampoline load instruction. */ u32 jump[7]; - u32 unused; + /* Used by ftrace to identify stubs */ + u32 magic; /* Data for the above code */ func_desc_t funcdata; }; @@ -139,70 +142,39 @@ static u32 ppc64_stub_insns[] = { }; #ifdef CONFIG_DYNAMIC_FTRACE - -static u32 ppc64_stub_mask[] = { - 0xffff0000, - 0xffff0000, - 0xffffffff, - 0xffffffff, -#if !defined(_CALL_ELF) || _CALL_ELF != 2 - 0xffffffff, -#endif - 0xffffffff, - 0xffffffff -}; - -bool is_module_trampoline(u32 *p) +int module_trampoline_target(struct module *mod, unsigned long addr, + unsigned long *target) { - unsigned int i; - u32 insns[ARRAY_SIZE(ppc64_stub_insns)]; - - BUILD_BUG_ON(sizeof(ppc64_stub_insns) != sizeof(ppc64_stub_mask)); + struct ppc64_stub_entry *stub; + func_desc_t funcdata; + u32 magic; - if (probe_kernel_read(insns, p, sizeof(insns))) + if (!within_module_core(addr, mod)) { + pr_err("%s: stub %lx not in module %s\n", __func__, addr, mod->name); return -EFAULT; - - for (i = 0; i < ARRAY_SIZE(ppc64_stub_insns); i++) { - u32 insna = insns[i]; - u32 insnb = ppc64_stub_insns[i]; - u32 mask = ppc64_stub_mask[i]; - - if ((insna & mask) != (insnb & mask)) - return false; } - return true; -} + stub = (struct ppc64_stub_entry *)addr; -int module_trampoline_target(struct module *mod, u32 *trampoline, - unsigned long *target) -{ - u32 buf[2]; - u16 upper, lower; - long offset; - void *toc_entry; - - if (probe_kernel_read(buf, trampoline, sizeof(buf))) + if (probe_kernel_read(&magic, &stub->magic, sizeof(magic))) { + pr_err("%s: fault reading magic for stub %lx for %s\n", __func__, addr, mod->name); return -EFAULT; + } - upper = buf[0] & 0xffff; - lower = buf[1] & 0xffff; - - /* perform the addis/addi, both signed */ - offset = ((short)upper << 16) + (short)lower; + if (magic != STUB_MAGIC) { + pr_err("%s: bad magic for stub %lx for %s\n", __func__, addr, mod->name); + return -EFAULT; + } - /* - * Now get the address this trampoline jumps to. This - * is always 32 bytes into our trampoline stub. - */ - toc_entry = (void *)mod->arch.toc + offset + 32; + if (probe_kernel_read(&funcdata, &stub->funcdata, sizeof(funcdata))) { + pr_err("%s: fault reading funcdata for stub %lx for %s\n", __func__, addr, mod->name); + return -EFAULT; + } - if (probe_kernel_read(target, toc_entry, sizeof(*target))) - return -EFAULT; + *target = stub_func_addr(funcdata); return 0; } - #endif /* Count how many different 24-bit relocations (different symbol, @@ -447,6 +419,8 @@ static inline int create_stub(const Elf64_Shdr *sechdrs, entry->jump[0] |= PPC_HA(reladdr); entry->jump[1] |= PPC_LO(reladdr); entry->funcdata = func_desc(addr); + entry->magic = STUB_MAGIC; + return 1; }
When a module is loaded, calls out to the kernel go via a stub which is generated at runtime. One of these stubs is used to call _mcount(), which is the default target of tracing calls generated by the compiler with -pg. If dynamic ftrace is enabled (which it typicall is), another stub is used to call ftrace_caller(), which is the target of tracing calls when ftrace is actually active. ftrace then wants to disable the calls to _mcount() at module startup, and enable/disable the calls to ftrace_caller() when enabling/disablig tracing - all of these it does by patching the code. As part of that code patching, the ftrace code wants to confirm that the branch it is about to modify, is in fact a call to a module stub which calls _mcount() or ftrace_caller(). Currently it does that by inspecting the instructions and confirming they are what it expects. Although that works, the code to do it is pretty intricate because it requires lots of knowledge about the exact format of the stub. We can make that process easier by marking the generated stubs with a magic value, and then looking for that magic value. Altough this is not as rigorous as the current method, I believe it is sufficient in practice. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/include/asm/module.h | 3 +- arch/powerpc/kernel/ftrace.c | 14 ++----- arch/powerpc/kernel/module_64.c | 78 +++++++++++++-------------------------- 3 files changed, 31 insertions(+), 64 deletions(-)