From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ozlabs.org (ozlabs.org [IPv6:2401:3900:2:1::2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id A88E41A054D for ; Thu, 25 Feb 2016 11:04:16 +1100 (AEDT) Received: from mail-pf0-x233.google.com (mail-pf0-x233.google.com [IPv6:2607:f8b0:400e:c00::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id E2E76140778 for ; Thu, 25 Feb 2016 11:04:15 +1100 (AEDT) Received: by mail-pf0-x233.google.com with SMTP id x65so21799950pfb.1 for ; Wed, 24 Feb 2016 16:04:15 -0800 (PST) Subject: Re: [PATCH 02/12] powerpc/module: Mark module stubs with a magic value To: Michael Ellerman , linuxppc-dev@ozlabs.org References: <1456324115-21144-1-git-send-email-mpe@ellerman.id.au> <1456324115-21144-2-git-send-email-mpe@ellerman.id.au> Cc: duwe@lst.de, linux-kernel@vger.kernel.org, rostedt@goodmis.org, kamalesh@linux.vnet.ibm.com, pmladek@suse.com, jeyu@redhat.com, jkosina@suse.cz, live-patching@vger.kernel.org, mbenes@suse.cz From: Balbir Singh Message-ID: <56CE44F8.8070605@gmail.com> Date: Thu, 25 Feb 2016 11:04:08 +1100 MIME-Version: 1.0 In-Reply-To: <1456324115-21144-2-git-send-email-mpe@ellerman.id.au> Content-Type: text/plain; charset=windows-1252 List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 > --- > 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