Changes since v5:
* extra "std r0,LRSAVE(r1)" for gcc-6
This makes the code compiler-agnostic.
* Follow Petr Mladek's suggestion to avoid
redefinition of HAVE_LIVEPATCH
Changes since v4:
* change comment style in entry_64.S to C89
(nobody is using assembler syntax comments there).
* the bool function restore_r2 shouldn't return 2,
that's a little confusing.
* Test whether the compiler supports -mprofile-kernel
and only then define CC_USING_MPROFILE_KERNEL
* also make the return value of klp_check_compiler_support
depend on that.
Major changes since v3:
* the graph tracer works now.
It turned out the stack frame it tried to manipulate does not
exist at that point.
* changes only needed in order to support -mprofile-kernel are now
in a separate patch, prepended.
* Kconfig cleanup so this is only selectable on ppc64le.
Torsten Duwe (9):
ppc64 (le): prepare for -mprofile-kernel
ppc64le FTRACE_WITH_REGS implementation
ppc use ftrace_modify_all_code default
ppc64 ftrace_with_regs configuration variables
ppc64 ftrace_with_regs: spare early boot and low level
ppc64 ftrace: disable profiling for some functions
ppc64 ftrace: disable profiling for some files
Implement kernel live patching for ppc64le (ABIv2)
Enable LIVEPATCH to be configured on ppc64le and add livepatch.o if it
is selected.
arch/powerpc/Kconfig | 7 ++
arch/powerpc/Makefile | 10 +++
arch/powerpc/include/asm/ftrace.h | 5 ++
arch/powerpc/include/asm/livepatch.h | 45 ++++++++++
arch/powerpc/kernel/Makefile | 13 +--
arch/powerpc/kernel/entry_64.S | 166 ++++++++++++++++++++++++++++++++++-
arch/powerpc/kernel/ftrace.c | 88 ++++++++++++++-----
arch/powerpc/kernel/livepatch.c | 38 ++++++++
arch/powerpc/kernel/module_64.c | 39 +++++++-
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/lib/Makefile | 4 +-
arch/powerpc/mm/fault.c | 2 +-
arch/powerpc/mm/hash_utils_64.c | 18 ++--
arch/powerpc/mm/hugetlbpage-hash64.c | 2 +-
arch/powerpc/mm/hugetlbpage.c | 4 +-
arch/powerpc/mm/mem.c | 2 +-
arch/powerpc/mm/pgtable_64.c | 2 +-
arch/powerpc/mm/slb.c | 6 +-
arch/powerpc/mm/slice.c | 8 +-
kernel/trace/Kconfig | 5 ++
20 files changed, 412 insertions(+), 54 deletions(-)
create mode 100644 arch/powerpc/include/asm/livepatch.h
create mode 100644 arch/powerpc/kernel/livepatch.c
--
1.8.5.6
The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
allows to call _mcount very early in the function, which low-level
ASM code and code patching functions need to consider.
Especially the link register and the parameter registers are still
alive and not yet saved into a new stack frame.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/kernel/entry_64.S | 45 +++++++++++++++++++++++++++++++++++++++--
arch/powerpc/kernel/ftrace.c | 12 +++++++++--
arch/powerpc/kernel/module_64.c | 14 +++++++++++++
3 files changed, 67 insertions(+), 4 deletions(-)
@@ -306,11 +306,19 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)*TheloadoffsetisdifferentdependingontheABI.Forsimplicity*justmaskitoutwhendoingthecompare.*/+#ifndef CC_USING_MPROFILE_KERNELif((op[0]!=0x48000008)||((op[1]&0xffff0000)!=0xe8410000)){-pr_err("Unexpected call sequence: %x %x\n",op[0],op[1]);+pr_err("Unexpected call sequence at %p: %x %x\n",+ip,op[0],op[1]);return-EINVAL;}-+#else+/* look for patched "NOP" on ppc64 with -mprofile-kernel */+if(op[0]!=0x60000000){+pr_err("Unexpected call at %p: %x\n",ip,op[0]);+return-EINVAL;+}+#endif/* If we never set up a trampoline to ftrace_caller, then bail */if(!rec->arch.mod->arch.tramp){pr_err("No ftrace trampoline\n");
Implement FTRACE_WITH_REGS for powerpc64, on ELF ABI v2.
Initial work started by Vojtech Pavlik, used with permission.
* arch/powerpc/kernel/entry_64.S:
- Implement an effective ftrace_caller that works from
within the kernel binary as well as from modules.
* arch/powerpc/kernel/ftrace.c:
- be prepared to deal with ppc64 ELF ABI v2, especially
calls to _mcount that result from gcc -mprofile-kernel
- a little more error verbosity
* arch/powerpc/kernel/module_64.c:
- do not save the TOC pointer on the trampoline when the
destination is ftrace_caller. This trampoline jump happens from
a function prologue before a new stack frame is set up, so bad
things may happen otherwise...
- relax is_module_trampoline() to recognise the modified
trampoline.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/include/asm/ftrace.h | 5 +++
arch/powerpc/kernel/entry_64.S | 78 +++++++++++++++++++++++++++++++++++++++
arch/powerpc/kernel/ftrace.c | 60 +++++++++++++++++++++++++++---
arch/powerpc/kernel/module_64.c | 25 ++++++++++++-
4 files changed, 161 insertions(+), 7 deletions(-)
@@ -46,6 +46,8 @@externvoid_mcount(void);#ifdef CONFIG_DYNAMIC_FTRACE+# define FTRACE_ADDR ((unsigned long)ftrace_caller)+# define FTRACE_REGS_ADDR FTRACE_ADDRstaticinlineunsignedlongftrace_call_adjust(unsignedlongaddr){/* reloction of mcount call site is the same as the address */
@@ -61,8 +61,11 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)return-EFAULT;/* Make sure it is what we expect it to be */-if(replaced!=old)+if(replaced!=old){+pr_err("%p: replaced (%#x) != old (%#x)",+(void*)ip,replaced,old);return-EINVAL;+}/* replace the text with the new text */if(patch_instruction((unsignedint*)ip,new))
@@ -106,14 +109,16 @@ static int__ftrace_make_nop(structmodule*mod,structdyn_ftrace*rec,unsignedlongaddr){-unsignedintop;+unsignedintop,op0,op1,pop;unsignedlongentry,ptr;unsignedlongip=rec->ip;void*tramp;/* read where this goes */-if(probe_kernel_read(&op,(void*)ip,sizeof(int)))+if(probe_kernel_read(&op,(void*)ip,sizeof(int))){+pr_err("Fetching opcode failed.\n");return-EFAULT;+}/* Make sure that that this is still a 24bit jump */if(!is_bl_op(op)){
@@ -158,10 +163,46 @@ __ftrace_make_nop(struct module *mod,**Useab+8tojumpovertheload.*/-op=0x48000008;/* b +8 */-if(patch_instruction((unsignedint*)ip,op))+pop=0x48000008;/* b +8 */++/*+*Checkwhatisinthenextinstruction.Wecanseeldr2,40(r1),but+*onfirstpassafterbootwewillseemflrr0.+*/+if(probe_kernel_read(&op,(void*)(ip+4),MCOUNT_INSN_SIZE)){+pr_err("Fetching op failed.\n");+return-EFAULT;+}++if(op!=0xe8410028){/* ld r2,STACK_OFFSET(r1) */++if(probe_kernel_read(&op0,(void*)(ip-8),MCOUNT_INSN_SIZE)){+pr_err("Fetching op0 failed.\n");+return-EFAULT;+}++if(probe_kernel_read(&op1,(void*)(ip-4),MCOUNT_INSN_SIZE)){+pr_err("Fetching op1 failed.\n");+return-EFAULT;+}++/* mflr r0 ; std r0,LRSAVE(r1) */+if(op0!=0x7c0802a6&&op1!=0xf8010010){+pr_err("Unexpected instructions around bl\n"+"when enabling dynamic ftrace!\t"+"(%08x,%08x,bl,%08x)\n",op0,op1,op);+return-EINVAL;+}++/* When using -mkernel_profile there is no load to jump over */+pop=PPC_INST_NOP;+}++if(patch_instruction((unsignedint*)ip,pop)){+pr_err("Patching NOP failed.\n");return-EPERM;+}return0;}
@@ -138,12 +138,25 @@ static u32 ppc64_stub_insns[] = {0x4e800420/* bctr */};+#ifdef CC_USING_MPROFILE_KERNEL+/* In case of _mcount calls or dynamic ftracing, Do not save the+*currentcallee'sTOC(inR2)againintotheoriginalcaller'sstack+*frameduringthistrampolinehop.Thestackframealreadyholds+*thatoftheoriginalcaller._mcountandftrace_callerwilltake+*careofthisTOCvaluethemselves.+*/+#define SQUASH_TOC_SAVE_INSN(trampoline_addr) \+(((structppc64_stub_entry*)(trampoline_addr))->jump[2]=PPC_INST_NOP)+#else+#define SQUASH_TOC_SAVE_INSN(trampoline_addr)+#endif+#ifdef CONFIG_DYNAMIC_FTRACEstaticu32ppc64_stub_mask[]={0xffff0000,0xffff0000,-0xffffffff,+0x00000000,0xffffffff,#if !defined(_CALL_ELF) || _CALL_ELF != 20xffffffff,
@@ -619,6 +635,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,return-ENOENT;if(!restore_r2((u32*)location+1,me))return-ENOEXEC;+/* Squash the TOC saver for profiler calls */+if(!strcmp("_mcount",strtab+sym->st_name))+SQUASH_TOC_SAVE_INSN(value);}elsevalue+=local_entry_offset(sym);
@@ -679,6 +698,10 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,me->arch.tramp=stub_for_addr(sechdrs,(unsignedlong)ftrace_caller,me);+/* ftrace_caller will take care of the TOC;+*donotclobberoriginalcaller'svalue.+*/+SQUASH_TOC_SAVE_INSN(me->arch.tramp);#endifreturn0;
Convert ppc's arch_ftrace_update_code from its own function copy
to use the generic default functionality (without stop_machine --
our instructions are properly aligned and the replacements atomic ;)
With this we gain error checking and the much-needed function_trace_op
handling.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/kernel/ftrace.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
@@ -511,20 +511,12 @@ void ftrace_replace_code(int enable)}}+/* Use the default ftrace_modify_all_code, but without+*stop_machine().+*/voidarch_ftrace_update_code(intcommand){-if(command&FTRACE_UPDATE_CALLS)-ftrace_replace_code(1);-elseif(command&FTRACE_DISABLE_CALLS)-ftrace_replace_code(0);--if(command&FTRACE_UPDATE_TRACE_FUNC)-ftrace_update_ftrace_func(ftrace_trace_function);--if(command&FTRACE_START_FUNC_RET)-ftrace_enable_ftrace_graph_caller();-elseif(command&FTRACE_STOP_FUNC_RET)-ftrace_disable_ftrace_graph_caller();+ftrace_modify_all_code(command);}int__initftrace_dyn_arch_init(void)
Using -mprofile-kernel on early boot code not only confuses the
checker but is also useless, as the infrastructure is not yet in
place. Proceed like with -pg (remove it from CFLAGS), equally with
time.o and ftrace itself.
* arch/powerpc/kernel/Makefile:
- remove -mprofile-kernel from low level and boot code objects'
CFLAGS for FUNCTION_TRACER configurations.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/kernel/Makefile | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -16,14 +16,14 @@ endififdef CONFIG_FUNCTION_TRACER# Do not trace early boot code-CFLAGS_REMOVE_cputable.o=-pg-mno-sched-epilog-CFLAGS_REMOVE_prom_init.o=-pg-mno-sched-epilog-CFLAGS_REMOVE_btext.o=-pg-mno-sched-epilog-CFLAGS_REMOVE_prom.o=-pg-mno-sched-epilog+CFLAGS_REMOVE_cputable.o=-pg-mno-sched-epilog-mprofile-kernel+CFLAGS_REMOVE_prom_init.o=-pg-mno-sched-epilog-mprofile-kernel+CFLAGS_REMOVE_btext.o=-pg-mno-sched-epilog-mprofile-kernel+CFLAGS_REMOVE_prom.o=-pg-mno-sched-epilog-mprofile-kernel# do not trace tracer code-CFLAGS_REMOVE_ftrace.o=-pg-mno-sched-epilog+CFLAGS_REMOVE_ftrace.o=-pg-mno-sched-epilog-mprofile-kernel# timers used by tracing-CFLAGS_REMOVE_time.o=-pg-mno-sched-epilog+CFLAGS_REMOVE_time.o=-pg-mno-sched-epilog-mprofile-kernelendifobj-y:=cputable.optrace.osyscalls.o\
At least POWER7/8 have MMUs that don't completely autoload;
a normal, recoverable memory fault might pass through these functions.
If a dynamic tracer function causes such a fault, any of these functions
being traced with -mprofile-kernel may cause an endless recursion.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/mm/fault.c | 2 +-
arch/powerpc/mm/hash_utils_64.c | 18 +++++++++---------
arch/powerpc/mm/hugetlbpage-hash64.c | 2 +-
arch/powerpc/mm/hugetlbpage.c | 4 ++--
arch/powerpc/mm/mem.c | 2 +-
arch/powerpc/mm/pgtable_64.c | 2 +-
arch/powerpc/mm/slb.c | 6 +++---
arch/powerpc/mm/slice.c | 8 ++++----
9 files changed, 23 insertions(+), 23 deletions(-)
@@ -205,7 +205,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault)*Thereturnvalueis0ifthefaultwashandled,orthesignal*numberifthisisakernelfaultthatcan'tbehandledhere.*/-int__kprobesdo_page_fault(structpt_regs*regs,unsignedlongaddress,+notraceint__kprobesdo_page_fault(structpt_regs*regs,unsignedlongaddress,unsignedlongerror_code){enumctx_stateprev_state=exception_enter();
@@ -870,7 +870,7 @@ unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)}#ifdef CONFIG_PPC_MM_SLICES-staticunsignedintget_paca_psize(unsignedlongaddr)+staticnotraceunsignedintget_paca_psize(unsignedlongaddr){u64lpsizes;unsignedchar*hpsizes;
@@ -899,7 +899,7 @@ unsigned int get_paca_psize(unsigned long addr)*Fornowthismakesthewholeprocessuse4kpages.*/#ifdef CONFIG_PPC_64K_PAGES-voiddemote_segment_4k(structmm_struct*mm,unsignedlongaddr)+notracevoiddemote_segment_4k(structmm_struct*mm,unsignedlongaddr){if(get_slice_psize(mm,addr)==MMU_PAGE_4K)return;
@@ -1289,7 +1289,7 @@ out_exit:/* WARNING: This is called from hash_low_64.S, if you change this prototype,*donotforgettoupdatetheassemblycallsite!*/-voidflush_hash_page(unsignedlongvpn,real_pte_tpte,intpsize,intssize,+notracevoidflush_hash_page(unsignedlongvpn,real_pte_tpte,intpsize,intssize,unsignedlongflags){unsignedlonghash,index,shift,hidx,slot;
@@ -1437,7 +1437,7 @@ void low_hash_fault(struct pt_regs *regs, unsigned long address, int rc)exception_exit(prev_state);}-longhpte_insert_repeating(unsignedlonghash,unsignedlongvpn,+notracelonghpte_insert_repeating(unsignedlonghash,unsignedlongvpn,unsignedlongpa,unsignedlongrflags,unsignedlongvflags,intpsize,intssize){
@@ -18,7 +18,7 @@ extern long hpte_insert_repeating(unsigned long hash, unsigned long vpn,unsignedlongpa,unsignedlongrlags,unsignedlongvflags,intpsize,intssize);-int__hash_page_huge(unsignedlongea,unsignedlongaccess,unsignedlongvsid,+notraceint__hash_page_huge(unsignedlongea,unsignedlongaccess,unsignedlongvsid,pte_t*ptep,unsignedlongtrap,unsignedlongflags,intssize,unsignedintshift,unsignedintmmu_psize){
@@ -96,7 +96,7 @@ static inline void create_shadowed_slbe(unsigned long ea, int ssize,:"memory");}-staticvoid__slb_flush_and_rebolt(void)+staticnotracevoid__slb_flush_and_rebolt(void){/* If you change this make sure you change SLB_NUM_BOLTED*andPRKVMappropriatelytoo.*/
* create the appropriate files+functions
arch/powerpc/include/asm/livepatch.h
klp_check_compiler_support,
klp_arch_set_pc
arch/powerpc/kernel/livepatch.c with a stub for
klp_write_module_reloc
This is architecture-independent work in progress.
* introduce a fixup in arch/powerpc/kernel/entry_64.S
for local calls that are becoming global due to live patching.
And of course do the main KLP thing: return to a maybe different
address, possibly altered by the live patching ftrace op.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/include/asm/livepatch.h | 45 +++++++++++++++++++++++++++++++
arch/powerpc/kernel/entry_64.S | 51 +++++++++++++++++++++++++++++++++---
arch/powerpc/kernel/livepatch.c | 38 +++++++++++++++++++++++++++
3 files changed, 130 insertions(+), 4 deletions(-)
create mode 100644 arch/powerpc/include/asm/livepatch.h
create mode 100644 arch/powerpc/kernel/livepatch.c
@@ -1305,15 +1332,31 @@ _GLOBAL(ftrace_graph_stub)addir1,r1,112#endif-mflrr0/*movethisLRtoCTR*/-mtctrr0-ldr0,LRSAVE(r1)/*restorecallee's lr at _mcount site */mtlrr0bctr/*jumpafter_mcountsite*/#endif /* CC_USING_MPROFILE_KERNEL */ _GLOBAL(ftrace_stub)blr++#ifdef CONFIG_LIVEPATCH+/*Helperfunctionforlocalcallsthatarebecomingglobal+duetolivepatching.+Wecan't simply patch the NOP after the original call,+because,dependingontheconsistencymodel,somekernel+threadsmaystillhavecalledtheoriginal,localfunction+*without*savingtheirTOCintherespectivestackframeslot,+sothedecisionismadeper-threadduringfunctionreturnby+maybeinsertingaKLP_return_helperframeornot.+*/+KLP_return_helper:+ldr2,24(r1)/*restoreTOC (savedbyftrace_caller)*/+addir1,r1,32/*destroyministackframe*/+ldr0,LRSAVE(r1)/*gettherealreturnaddress*/+mtlrr0+blr+#endif+#else _GLOBAL_TOC(_mcount)/*Takenfromoutputofobjdumpfromlib64/glibc*/
@@ -0,0 +1,38 @@+/*+*livepatch.c-powerpc-specificKernelLivePatchingCore+*+*Copyright(C)2015SUSE+*+*Thisprogramisfreesoftware;youcanredistributeitand/or+*modifyitunderthetermsoftheGNUGeneralPublicLicense+*aspublishedbytheFreeSoftwareFoundation;eitherversion2+*oftheLicense,or(atyouroption)anylaterversion.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,+*butWITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.Seethe+*GNUGeneralPublicLicenseformoredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicense+*alongwiththisprogram;ifnot,see<http://www.gnu.org/licenses/>.+*/+#include<linux/module.h>+#include<asm/livepatch.h>++/**+*klp_write_module_reloc()-writearelocationinamodule+*@mod:moduleinwhichthesectiontobemodifiedisfound+*@type:ELFrelocationtype(seeasm/elf.h)+*@loc:addressthattherelocationshouldbewrittento+*@value:relocationvalue(symaddress+addend)+*+*Thisfunctionwritesarelocationtothespecifiedlocationfor+*aparticularmodule.+*/+intklp_write_module_reloc(structmodule*mod,unsignedlongtype,+unsignedlongloc,unsignedlongvalue)+{+/* This requires infrastructure changes; we need the loadinfos. */+pr_err("lpc_write_module_reloc not yet supported\n");+return-ENOSYS;+}
This patch complements the "notrace" attribute for selected functions.
It adds -mprofile-kernel to the cc flags to be stripped from the command
line for code-patching.o and feature-fixups.o, in addition to "-pg"
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/lib/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Miroslav Benes <mbenes@suse.cz> Date: 2016-01-26 10:50:33
[ added Petr to CC list ]
On Mon, 25 Jan 2016, Torsten Duwe wrote:
* create the appropriate files+functions
arch/powerpc/include/asm/livepatch.h
klp_check_compiler_support,
klp_arch_set_pc
arch/powerpc/kernel/livepatch.c with a stub for
klp_write_module_reloc
This is architecture-independent work in progress.
* introduce a fixup in arch/powerpc/kernel/entry_64.S
for local calls that are becoming global due to live patching.
And of course do the main KLP thing: return to a maybe different
address, possibly altered by the live patching ftrace op.
Signed-off-by: Torsten Duwe <redacted>
Hi,
I have a few questions...
We still need Petr's patch from [1] to make livepatch work, right? Could
you, please, add it to this patch set to make it self-sufficient?
Second, what is the situation with mcount prologue between gcc < 6 and
gcc-6? Are there only 12 bytes in gcc-6 prologue? If yes, we need to
change Petr's patch to make it more general and to be able to cope with
different prologues. This is unfortunate. Either way, please mention it
somewhere in a changelog.
I haven't reviewed the patch properly yet, but there is a comment below.
[1] http://lkml.kernel.org/g/20151203160004.GE8047@pathway.suse.cz
+/**
+ * klp_write_module_reloc() - write a relocation in a module
+ * @mod: module in which the section to be modified is found
+ * @type: ELF relocation type (see asm/elf.h)
+ * @loc: address that the relocation should be written to
+ * @value: relocation value (sym address + addend)
+ *
+ * This function writes a relocation to the specified location for
+ * a particular module.
+ */
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value)
+{
+ /* This requires infrastructure changes; we need the loadinfos. */
+ pr_err("lpc_write_module_reloc not yet supported\n");
This is a nit, but there is no lpc_write_module_reloc. It should be
klp_write_module_reloc.
Thanks,
Miroslav
From: Petr Mladek <pmladek@suse.com> Date: 2016-01-26 12:49:00
On Tue 2016-01-26 11:50:25, Miroslav Benes wrote:
[ added Petr to CC list ]
On Mon, 25 Jan 2016, Torsten Duwe wrote:
quoted
* create the appropriate files+functions
arch/powerpc/include/asm/livepatch.h
klp_check_compiler_support,
klp_arch_set_pc
arch/powerpc/kernel/livepatch.c with a stub for
klp_write_module_reloc
This is architecture-independent work in progress.
* introduce a fixup in arch/powerpc/kernel/entry_64.S
for local calls that are becoming global due to live patching.
And of course do the main KLP thing: return to a maybe different
address, possibly altered by the live patching ftrace op.
Signed-off-by: Torsten Duwe <redacted>
Hi,
I have a few questions...
We still need Petr's patch from [1] to make livepatch work, right? Could
you, please, add it to this patch set to make it self-sufficient?
Second, what is the situation with mcount prologue between gcc < 6 and
gcc-6? Are there only 12 bytes in gcc-6 prologue? If yes, we need to
change Petr's patch to make it more general and to be able to cope with
different prologues. This is unfortunate. Either way, please mention it
somewhere in a changelog.
I am going to update the extra patch. There is an idea to detect the
offset during build by scrips/recordmcount. This tool looks for the
ftrace locations. The offset should always be a constant that depends
on the used architecture, compiler, and compiler flags.
The tool is called post build. We might need to pass the constant
as a symbol added to the binary. The tool already adds some symbols.
Best Regards,
Petr
On Tue, Jan 26, 2016 at 01:48:53PM +0100, Petr Mladek wrote:
On Tue 2016-01-26 11:50:25, Miroslav Benes wrote:
quoted
We still need Petr's patch from [1] to make livepatch work, right? Could
you, please, add it to this patch set to make it self-sufficient?
It's Petr's patch, I don't want to decide how to best tackle this, see below.
I think Michael is already aware that it is needed, too.
quoted
Second, what is the situation with mcount prologue between gcc < 6 and
gcc-6? Are there only 12 bytes in gcc-6 prologue? If yes, we need to
Precisely, it's commit e95d0248daced44 (in http://repo.or.cz/official-gcc.git)
or svn trunk change 222352 "No need for -mprofile-kernel to save LR to stack."
It's efficient, I like it.
I am going to update the extra patch. There is an idea to detect the
offset during build by scrips/recordmcount. This tool looks for the
ftrace locations. The offset should always be a constant that depends
on the used architecture, compiler, and compiler flags.
My first idea was to check for compiler version defines, but some vendors
are rumoured to patch their compilers ;-)
The tool is called post build. We might need to pass the constant
as a symbol added to the binary. The tool already adds some symbols.
On Tue, Jan 26, 2016 at 11:50:25AM +0100, Miroslav Benes wrote:
quoted
+ */
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value)
+{
+ /* This requires infrastructure changes; we need the loadinfos. */
+ pr_err("lpc_write_module_reloc not yet supported\n");
This is a nit, but there is no lpc_write_module_reloc. It should be
klp_write_module_reloc.
Indeed. Michael, feel free to fix this on the fly or not. It needs to
disappear anyway and be replaced with functionality.
Torsten
From: Miroslav Benes <mbenes@suse.cz> Date: 2016-01-26 14:14:25
[ Jessica added to CC list so she is aware that there are plans to
implement livepatch on ppc64le ]
On Tue, 26 Jan 2016, Torsten Duwe wrote:
On Tue, Jan 26, 2016 at 11:50:25AM +0100, Miroslav Benes wrote:
quoted
quoted
+ */
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value)
+{
+ /* This requires infrastructure changes; we need the loadinfos. */
+ pr_err("lpc_write_module_reloc not yet supported\n");
This is a nit, but there is no lpc_write_module_reloc. It should be
klp_write_module_reloc.
Indeed. Michael, feel free to fix this on the fly or not. It needs to
disappear anyway and be replaced with functionality.
[ Jessica added to CC list so she is aware that there are plans to
implement livepatch on ppc64le ]
On Tue, 26 Jan 2016, Torsten Duwe wrote:
quoted
On Tue, Jan 26, 2016 at 11:50:25AM +0100, Miroslav Benes wrote:
quoted
quoted
+ */
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value)
+{
+ /* This requires infrastructure changes; we need the loadinfos. */
+ pr_err("lpc_write_module_reloc not yet supported\n");
This is a nit, but there is no lpc_write_module_reloc. It should be
klp_write_module_reloc.
Indeed. Michael, feel free to fix this on the fly or not. It needs to
disappear anyway and be replaced with functionality.
Miroslav, thanks for the CC. Indeed, if things go well, there may be
no need to implement klp_write_module_reloc() anymore in the near
future. :-)
Jessica
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-01-27 10:19:40
Hi Torsten,
On Mon, 2016-01-25 at 16:26 +0100, Torsten Duwe wrote:
quoted hunk
The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
allows to call _mcount very early in the function, which low-level
ASM code and code patching functions need to consider.
Especially the link register and the parameter registers are still
alive and not yet saved into a new stack frame.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/kernel/entry_64.S | 45 +++++++++++++++++++++++++++++++++++++++--
arch/powerpc/kernel/ftrace.c | 12 +++++++++--
arch/powerpc/kernel/module_64.c | 14 +++++++++++++
3 files changed, 67 insertions(+), 4 deletions(-)
Can we use r11 instead? eg:
_GLOBAL(_mcount)
mflr r11
mtctr r11
mtlr r0
bctr
Otherwise I worry the std/ld is going to cause a load-hit-store. And it's just
plain more instructions too.
I don't quite grok the gcc code enough to tell if that's always safe, GCC does
use r11 sometimes, but I don't think it ever expects it to survive across
_mcount()?
quoted hunk
@@ -1262,13 +1267,28 @@ _GLOBAL(ftrace_stub) #ifdef CONFIG_FUNCTION_GRAPH_TRACER _GLOBAL(ftrace_graph_caller)+#ifdef CC_USING_MPROFILE_KERNEL+ /* with -mprofile-kernel, parameter regs are still alive at _mcount */+ std r10, 104(r1)+ std r9, 96(r1)+ std r8, 88(r1)+ std r7, 80(r1)+ std r6, 72(r1)+ std r5, 64(r1)+ std r4, 56(r1)+ std r3, 48(r1)+ mfctr r4 /* ftrace_caller has moved local addr here */+ std r4, 40(r1)+ mflr r3 /* ftrace_caller has restored LR from stack */+#else /* load r4 with local address */ ld r4, 128(r1)- subi r4, r4, MCOUNT_INSN_SIZE /* Grab the LR out of the caller stack frame */ ld r11, 112(r1) ld r3, 16(r11)+#endif+ subi r4, r4, MCOUNT_INSN_SIZE bl prepare_ftrace_return nop
AFAICS these end up being the only instructions shared between the two
versions. Which I don't think is worth the semantic burden of all the #ifdefs.
So please just write it as two separate functions, one for
CC_USING_MPROFILE_KERNEL and one for not.
quoted hunk
@@ -1277,6 +1297,26 @@ _GLOBAL(ftrace_graph_caller) * prepare_ftrace_return gives us the address we divert to. * Change the LR in the callers stack frame to this. */++#ifdef CC_USING_MPROFILE_KERNEL+ mtlr r3++ ld r0, 40(r1)+ mtctr r0+ ld r10, 104(r1)+ ld r9, 96(r1)+ ld r8, 88(r1)+ ld r7, 80(r1)+ ld r6, 72(r1)+ ld r5, 64(r1)+ ld r4, 56(r1)+ ld r3, 48(r1)++ addi r1, r1, 112+ mflr r0+ std r0, LRSAVE(r1)+ bctr+#else ld r11, 112(r1) std r3, 16(r11)
@@ -1284,6 +1324,7 @@ _GLOBAL(ftrace_graph_caller) mtlr r0 addi r1, r1, 112 blr+#endif _GLOBAL(return_to_handler) /* need to save return values */
Can we use r11 instead? eg:
_GLOBAL(_mcount)
mflr r11
mtctr r11
mtlr r0
bctr
Otherwise I worry the std/ld is going to cause a load-hit-store. And it's just
plain more instructions too.
I don't quite grok the gcc code enough to tell if that's always safe, GCC does
use r11 sometimes, but I don't think it ever expects it to survive across
_mcount()?
I used r11 in that area once, and it crashed, but I don't recall the deatils.
We'll see. The performance shouldn't be critical, as the code is only used
during boot-up. With DYNAMIC_FTRACE, The calls will be replaced by
0x600000^W PPC_INST_NOP :)
quoted
bl prepare_ftrace_return
nop
AFAICS these end up being the only instructions shared between the two
versions. Which I don't think is worth the semantic burden of all the #ifdefs.
So please just write it as two separate functions, one for
CC_USING_MPROFILE_KERNEL and one for not.
On Mon, 25 Jan 2016 16:38:48 +0100
Torsten Duwe [off-list ref] wrote:
Changes since v5:
* extra "std r0,LRSAVE(r1)" for gcc-6
This makes the code compiler-agnostic.
* Follow Petr Mladek's suggestion to avoid
redefinition of HAVE_LIVEPATCH
I looked at the patches - well mostly patches 1 and 2, some quick questions
1. I know -mprofile-kernel is a big optimization win, do we need it or can
we incrementally add it?
2. Some of the hardcoded checks for opcode are hard to review, I know they've
been there in similar forms for a while. May be as an iterative step we should
give the numbers some meaning and use proper helpers for it.
I am going to give the patches a spin
Balbir Singh.
On Wed, Jan 27, 2016 at 09:51:12PM +1100, Balbir Singh wrote:
On Mon, 25 Jan 2016 16:38:48 +0100
Torsten Duwe [off-list ref] wrote:
quoted
Changes since v5:
* extra "std r0,LRSAVE(r1)" for gcc-6
This makes the code compiler-agnostic.
* Follow Petr Mladek's suggestion to avoid
redefinition of HAVE_LIVEPATCH
I looked at the patches - well mostly patches 1 and 2, some quick questions
1. I know -mprofile-kernel is a big optimization win, do we need it or can
we incrementally add it?
There's a reason why these are first ;-)
The following ones assume -mprofile-kernel is used.
The disadvantage is all relevant registers need to be saved before calling
further C code in between functions. On the Pro side, no stack frame has been
created at that point. These are assumptions made all over the ftrace-with-regs
and live patching code here.
2. Some of the hardcoded checks for opcode are hard to review, I know they've
been there in similar forms for a while. May be as an iterative step we should
give the numbers some meaning and use proper helpers for it.
Yes, Michael has already criticised that. No further literal hex constants, I promise.
I am going to give the patches a spin
Thanks! Make sure you use a compiler that can disable -mprofile-kernel with "notrace".
Torsten
From: Alan Modra <hidden> Date: 2016-01-27 12:59:14
On Wed, Jan 27, 2016 at 09:19:27PM +1100, Michael Ellerman wrote:
Hi Torsten,
On Mon, 2016-01-25 at 16:26 +0100, Torsten Duwe wrote:
quoted
The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
allows to call _mcount very early in the function, which low-level
ASM code and code patching functions need to consider.
Especially the link register and the parameter registers are still
alive and not yet saved into a new stack frame.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/kernel/entry_64.S | 45 +++++++++++++++++++++++++++++++++++++++--
arch/powerpc/kernel/ftrace.c | 12 +++++++++--
arch/powerpc/kernel/module_64.c | 14 +++++++++++++
3 files changed, 67 insertions(+), 4 deletions(-)
Can we use r11 instead? eg:
_GLOBAL(_mcount)
mflr r11
mtctr r11
mtlr r0
bctr
Depends on what you need to support. As Torsten says, the code to
call _mcount when -mprofile-kernel is emitted before the prologue of a
function (similar to -m32), but after the ELFv2 global entry point
code. If you trash r11 here you're killing the static chain pointer,
used by C nested functions or other languages that use a static chain,
eg. Pascal. r11 has *not* been saved for ELFv2.
r12 might be a better choice for a temp reg.
--
Alan Modra
Australia Development Lab, IBM
On Wed, Jan 27, 2016 at 11:28:09PM +1030, Alan Modra wrote:
On Wed, Jan 27, 2016 at 09:19:27PM +1100, Michael Ellerman wrote:
quoted
Can we use r11 instead? eg:
_GLOBAL(_mcount)
mflr r11
mtctr r11
mtlr r0
bctr
Depends on what you need to support. As Torsten says, the code to
call _mcount when -mprofile-kernel is emitted before the prologue of a
function (similar to -m32), but after the ELFv2 global entry point
code. If you trash r11 here you're killing the static chain pointer,
used by C nested functions or other languages that use a static chain,
eg. Pascal. r11 has *not* been saved for ELFv2.
Even if nested functions aren't supported in the Linux kernel(?), I think
it was an earlier version of mcount when r11 usage ruined my day.
r12 might be a better choice for a temp reg.
Good idea. r12 holds the function entry point and is used to calculate the
new TOC value just _before_ the call. It should be available.
I'll try, thanks for the hint.
Torsten
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-01-28 03:32:02
On Thu, 2016-01-28 at 13:41 +1100, Balbir Singh wrote:
On Wed, 27 Jan 2016 13:19:04 +0100
Torsten Duwe [off-list ref] wrote:
quoted
Thanks! Make sure you use a compiler that can disable -mprofile-kernel with "notrace".
gcc-6? I have gcc-5.2.1
That should work.
But that's a good point.
We need to have some Makefile logic to only enable -mprofile-kernel when it's
known to work, ie. for versions where the notrace fix is in.
Looking at GCC history it looks like the fix is in 4.9.0 and anything later.
But a version check doesn't work with patched distro/vendor toolchains. So we
probably need some sort of runtime check.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-01-28 03:39:44
On Wed, 2016-01-27 at 23:28 +1030, Alan Modra wrote:
On Wed, Jan 27, 2016 at 09:19:27PM +1100, Michael Ellerman wrote:
quoted
Hi Torsten,
On Mon, 2016-01-25 at 16:26 +0100, Torsten Duwe wrote:
quoted
The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
allows to call _mcount very early in the function, which low-level
ASM code and code patching functions need to consider.
Especially the link register and the parameter registers are still
alive and not yet saved into a new stack frame.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/kernel/entry_64.S | 45 +++++++++++++++++++++++++++++++++++++++--
arch/powerpc/kernel/ftrace.c | 12 +++++++++--
arch/powerpc/kernel/module_64.c | 14 +++++++++++++
3 files changed, 67 insertions(+), 4 deletions(-)
Can we use r11 instead? eg:
_GLOBAL(_mcount)
mflr r11
mtctr r11
mtlr r0
bctr
Depends on what you need to support. As Torsten says, the code to
call _mcount when -mprofile-kernel is emitted before the prologue of a
function (similar to -m32), but after the ELFv2 global entry point
code. If you trash r11 here you're killing the static chain pointer,
used by C nested functions or other languages that use a static chain,
eg. Pascal. r11 has *not* been saved for ELFv2.
OK, thanks for clarfiying. Pascal is not a big concern :D. But although I
don't think we use nested functions anywhere, someone somewhere could be, or at
least might one day.
Can we use r11 instead? eg:
_GLOBAL(_mcount)
mflr r11
mtctr r11
mtlr r0
bctr
Otherwise I worry the std/ld is going to cause a load-hit-store. And it's just
plain more instructions too.
I don't quite grok the gcc code enough to tell if that's always safe, GCC does
use r11 sometimes, but I don't think it ever expects it to survive across
_mcount()?
I used r11 in that area once, and it crashed, but I don't recall the deatils.
We'll see. The performance shouldn't be critical, as the code is only used
during boot-up. With DYNAMIC_FTRACE, The calls will be replaced by
0x600000^W PPC_INST_NOP :)
True.
That raises an interesting question, how does it work *without* DYNAMIC_FTRACE?
It looks like you haven't updated that version of _mcount at all? Or maybe I'm
missing an #ifdef somewhere?
_GLOBAL_TOC(_mcount)
/* Taken from output of objdump from lib64/glibc */
mflr r3
ld r11, 0(r1)
stdu r1, -112(r1)
std r3, 128(r1)
ld r4, 16(r11)
subi r3, r3, MCOUNT_INSN_SIZE
LOAD_REG_ADDR(r5,ftrace_trace_function)
ld r5,0(r5)
ld r5,0(r5)
mtctr r5
bctrl
nop
It doesn't look like that will work right with the -mprofile-kernel ABI. And
indeed it doesn't boot.
So we'll need to work that out. I guess the minimum would be to disable
-mprofile-kernel if DYNAMIC_FTRACE is disabled.
Frankly I think we'd be happy to *only* support DYNAMIC_FTRACE, but the generic
code doesn't let us do that at the moment.
On Thu, Jan 28, 2016 at 02:31:58PM +1100, Michael Ellerman wrote:
Looking at GCC history it looks like the fix is in 4.9.0 and anything later.
Good. But 4.8.5 has a buggy -mprofile-kernel, and there will be no 4.8.6, Bad.
But a version check doesn't work with patched distro/vendor toolchains. So we
probably need some sort of runtime check.
Agreed.
/bin/echo -e '#include <linux/compiler.h>\nnotrace int func() { return 0; }' |
gcc -D__KERNEL__ -Iinclude -p -mprofile-kernel -x c -O2 - -S -o - | grep mcount
should be empty. If it yields "bl _mcount" your compiler is buggy.
I haven't looked at the kernel's "autoconf" yet, but it's probably capable
of testing this.
Torsten
On Thu, Jan 28, 2016 at 03:26:59PM +1100, Michael Ellerman wrote:
That raises an interesting question, how does it work *without* DYNAMIC_FTRACE?
It looks like you haven't updated that version of _mcount at all? Or maybe I'm
missing an #ifdef somewhere?
You didn't, I did. I haven't considered that combination.
It doesn't look like that will work right with the -mprofile-kernel ABI. And
indeed it doesn't boot.
The lean _mcount should handle it and boot, had I not misplaced it in
the #ifdefs, but then of course profiling wouldn't work.
So we'll need to work that out. I guess the minimum would be to disable
-mprofile-kernel if DYNAMIC_FTRACE is disabled.
I feel that supporting all combinations of ABIv1/ABIv2, FTRACE/DYNAMIC_FTRACE,
-p/-mprofile-kernel will get us into #ifdef hell, and at least one kernel
developer will go insane. That will probably be the one porting this
to ppc64be (ABIv1).
Frankly I think we'd be happy to *only* support DYNAMIC_FTRACE, but the generic
code doesn't let us do that at the moment.
Seconded.
I'll have a look at the Kconfigs.
But it's better than the previous version which didn't boot :)
That's your fault, you picked the wrong compiler ;-)
Also ftracetest fails at step 8:
...
[8] ftrace - function graph filters with stack tracer
Unable to handle kernel paging request for data at address 0xd0000000033d7f70
[...]
That doesn't happen without your series applied, though that doesn't 100% mean
it's your bug. I haven't had time to dig any deeper.
From: Petr Mladek <pmladek@suse.com> Date: 2016-02-02 12:12:30
On Tue 2016-01-26 13:48:53, Petr Mladek wrote:
On Tue 2016-01-26 11:50:25, Miroslav Benes wrote:
quoted
[ added Petr to CC list ]
On Mon, 25 Jan 2016, Torsten Duwe wrote:
quoted
* create the appropriate files+functions
arch/powerpc/include/asm/livepatch.h
klp_check_compiler_support,
klp_arch_set_pc
arch/powerpc/kernel/livepatch.c with a stub for
klp_write_module_reloc
This is architecture-independent work in progress.
* introduce a fixup in arch/powerpc/kernel/entry_64.S
for local calls that are becoming global due to live patching.
And of course do the main KLP thing: return to a maybe different
address, possibly altered by the live patching ftrace op.
Signed-off-by: Torsten Duwe <redacted>
Hi,
I have a few questions...
We still need Petr's patch from [1] to make livepatch work, right? Could
you, please, add it to this patch set to make it self-sufficient?
Second, what is the situation with mcount prologue between gcc < 6 and
gcc-6? Are there only 12 bytes in gcc-6 prologue? If yes, we need to
change Petr's patch to make it more general and to be able to cope with
different prologues. This is unfortunate. Either way, please mention it
somewhere in a changelog.
I am going to update the extra patch. There is an idea to detect the
offset during build by scrips/recordmcount. This tool looks for the
ftrace locations. The offset should always be a constant that depends
on the used architecture, compiler, and compiler flags.
The tool is called post build. We might need to pass the constant
as a symbol added to the binary. The tool already adds some symbols.
Hmm, the size of the offset is not a constant. In particular, leaf
functions do not set TOC before the mcount location.
For example, the code generated for int_to_scsilun() looks like:
00000000000002d0 <int_to_scsilun>:
2d0: a6 02 08 7c mflr r0
2d4: 10 00 01 f8 std r0,16(r1)
2d8: 01 00 00 48 bl 2d8 <int_to_scsilun+0x8>
2d8: R_PPC64_REL24 _mcount
2dc: a6 02 08 7c mflr r0
2e0: 10 00 01 f8 std r0,16(r1)
2e4: e1 ff 21 f8 stdu r1,-32(r1)
2e8: 00 00 20 39 li r9,0
2ec: 00 00 24 f9 std r9,0(r4)
2f0: 04 00 20 39 li r9,4
2f4: a6 03 29 7d mtctr r9
2f8: 00 00 40 39 li r10,0
2fc: 02 c2 68 78 rldicl r8,r3,56,8
300: 78 23 89 7c mr r9,r4
304: ee 51 09 7d stbux r8,r9,r10
308: 02 00 4a 39 addi r10,r10,2
30c: 01 00 69 98 stb r3,1(r9)
310: 02 84 63 78 rldicl r3,r3,48,16
314: e8 ff 00 42 bdnz 2fc <int_to_scsilun+0x2c>
318: 20 00 21 38 addi r1,r1,32
31c: 10 00 01 e8 ld r0,16(r1)
320: a6 03 08 7c mtlr r0
324: 20 00 80 4e blr
328: 00 00 00 60 nop
32c: 00 00 42 60 ori r2,r2,0
Note that non-leaf functions starts with
0000000000000330 <scsi_set_sense_information>:
330: 00 00 4c 3c addis r2,r12,0
330: R_PPC64_REL16_HA .TOC.
334: 00 00 42 38 addi r2,r2,0
334: R_PPC64_REL16_LO .TOC.+0x4
338: a6 02 08 7c mflr r0
33c: 10 00 01 f8 std r0,16(r1)
340: 01 00 00 48 bl 340 <scsi_set_sense_information+0x10>
340: R_PPC64_REL24 _mcount
The above code is generated from kernel-4.5-rc1 sources using
$> gcc --version
gcc (SUSE Linux) 4.8.5
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
But I get similar code also with
$> gcc-6 --version
gcc-6 (SUSE Linux) 6.0.0 20160121 (experimental) [trunk revision 232670]
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
The result is that kernel crashes when trying to trace leaf function
from modules. The mcount location is replaced with a call (branch)
that does not work without the TOC stuff.
By other words, it seems that the code generated with -mprofile-kernel
option has been buggy in all gcc versions.
I am curious that nobody found this earlier. Do I something wrong,
please?
Best Regards,
Petr
* create the appropriate files+functions
arch/powerpc/include/asm/livepatch.h
klp_check_compiler_support,
klp_arch_set_pc
arch/powerpc/kernel/livepatch.c with a stub for
klp_write_module_reloc
This is architecture-independent work in progress.
* introduce a fixup in arch/powerpc/kernel/entry_64.S
for local calls that are becoming global due to live patching.
And of course do the main KLP thing: return to a maybe different
address, possibly altered by the live patching ftrace op.
Signed-off-by: Torsten Duwe <redacted>
---
arch/powerpc/include/asm/livepatch.h | 45 +++++++++++++++++++++++++++++++
arch/powerpc/kernel/entry_64.S | 51
+++++++++++++++++++++++++++++++++---
arch/powerpc/kernel/livepatch.c | 38 +++++++++++++++++++++++++++
3 files changed, 130 insertions(+), 4 deletions(-)
create mode 100644 arch/powerpc/include/asm/livepatch.h
create mode 100644 arch/powerpc/kernel/livepatch.c
diff --git a/arch/powerpc/include/asm/livepatch.h
b/arch/powerpc/include/asm/livepatch.h
new file mode 100644
index 0000000..44e8a2d
@@ -1305,15 +1332,31 @@ _GLOBAL(ftrace_graph_stub)addir1,r1,112#endif-mflrr0/*movethisLRtoCTR*/-mtctrr0-ldr0,LRSAVE(r1)/*restorecallee's lr at _mcount site */mtlrr0bctr/*jumpafter_mcountsite*/#endif /* CC_USING_MPROFILE_KERNEL */ _GLOBAL(ftrace_stub)blr++#ifdef CONFIG_LIVEPATCH+/*Helperfunctionforlocalcallsthatarebecomingglobal+duetolivepatching.+Wecan't simply patch the NOP after the original call,+because,dependingontheconsistencymodel,somekernel+threadsmaystillhavecalledtheoriginal,localfunction+*without*savingtheirTOCintherespectivestackframeslot,+sothedecisionismadeper-threadduringfunctionreturnby+maybeinsertingaKLP_return_helperframeornot.+*/+KLP_return_helper:+ldr2,24(r1)/*restoreTOC (savedbyftrace_caller)*/+addir1,r1,32/*destroyministackframe*/+ldr0,LRSAVE(r1)/*gettherealreturnaddress*/+mtlrr0+blr+#endif+#else _GLOBAL_TOC(_mcount)/*Takenfromoutputofobjdumpfromlib64/glibc*/
diff --git a/arch/powerpc/kernel/livepatch.c
b/arch/powerpc/kernel/livepatch.c
new file mode 100644
index 0000000..564eafa
@@ -0,0 +1,38 @@+/*+*livepatch.c-powerpc-specificKernelLivePatchingCore+*+*Copyright(C)2015SUSE+*+*Thisprogramisfreesoftware;youcanredistributeitand/or+*modifyitunderthetermsoftheGNUGeneralPublicLicense+*aspublishedbytheFreeSoftwareFoundation;eitherversion2+*oftheLicense,or(atyouroption)anylaterversion.+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,+*butWITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.Seethe+*GNUGeneralPublicLicenseformoredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicense+*alongwiththisprogram;ifnot,see<http://www.gnu.org/licenses/>.+*/+#include<linux/module.h>+#include<asm/livepatch.h>++/**+*klp_write_module_reloc()-writearelocationinamodule+*@mod:moduleinwhichthesectiontobemodifiedisfound+*@type:ELFrelocationtype(seeasm/elf.h)+*@loc:addressthattherelocationshouldbewrittento+*@value:relocationvalue(symaddress+addend)+*+*Thisfunctionwritesarelocationtothespecifiedlocationfor+*aparticularmodule.+*/+intklp_write_module_reloc(structmodule*mod,unsignedlongtype,+unsignedlongloc,unsignedlongvalue)+{+/* This requires infrastructure changes; we need the loadinfos. */+pr_err("lpc_write_module_reloc not yet supported\n");+return-ENOSYS;+}--
On Tue, Feb 02, 2016 at 01:12:24PM +0100, Petr Mladek wrote:
Hmm, the size of the offset is not a constant. In particular, leaf
functions do not set TOC before the mcount location.
To be slightly more precise, a leaf function that additionally uses
no global data. No global function calls, no global data access =>
no need to load the TOC.
For example, the code generated for int_to_scsilun() looks like:
00000000000002d0 <int_to_scsilun>:
2d0: a6 02 08 7c mflr r0
2d4: 10 00 01 f8 std r0,16(r1)
2d8: 01 00 00 48 bl 2d8 <int_to_scsilun+0x8>
2d8: R_PPC64_REL24 _mcount
[...]
The above code is generated from kernel-4.5-rc1 sources using
$> gcc --version
gcc (SUSE Linux) 4.8.5
But I get similar code also with
$> gcc-6 --version
gcc-6 (SUSE Linux) 6.0.0 20160121 (experimental) [trunk revision 232670]
The result is that kernel crashes when trying to trace leaf function
The trampoline *requires* a proper TOC pointer to find the remote function
entry point. If you jump onto the trampoline with the TOC from the caller's
caller you'll grab some address from somewhere and jump into nirvana.
By other words, it seems that the code generated with -mprofile-kernel
option has been buggy in all gcc versions.
Either that or we need bigger trampolines for everybody.
Michael, should we grow every module trampoline to always load R2,
or fix GCC to recognise the generated bl _mcount as a global function call?
Anton, what do you think?
Torsten
From: Petr Mladek <pmladek@suse.com> Date: 2016-02-02 16:47:21
On Tue 2016-02-02 16:45:23, Torsten Duwe wrote:
On Tue, Feb 02, 2016 at 01:12:24PM +0100, Petr Mladek wrote:
quoted
Hmm, the size of the offset is not a constant. In particular, leaf
functions do not set TOC before the mcount location.
To be slightly more precise, a leaf function that additionally uses
no global data. No global function calls, no global data access =>
no need to load the TOC.
Thanks for explanation.
quoted
The result is that kernel crashes when trying to trace leaf function
The trampoline *requires* a proper TOC pointer to find the remote function
entry point. If you jump onto the trampoline with the TOC from the caller's
caller you'll grab some address from somewhere and jump into nirvana.
The dmesg messages suggested someting like this.
quoted
By other words, it seems that the code generated with -mprofile-kernel
option has been buggy in all gcc versions.
Either that or we need bigger trampolines for everybody.
Michael, should we grow every module trampoline to always load R2,
or fix GCC to recognise the generated bl _mcount as a global function call?
Anton, what do you think?
BTW: Is the trampoline used also for classic probes? If not, we might need
a trampoline for them as well.
Note that TOC is not set only when the problematic functions are
compiled with --mprofile-kernel. I still see the TOC stuff when
compiling only with -pg.
Best Regards,
Petr
Note that TOC is not set only when the problematic functions are
compiled with --mprofile-kernel. I still see the TOC stuff when
compiling only with -pg.
I don't see how this wouldn't be a gcc bug.
No matter whether it's plain profiling call (-pg) or kernel profiling call
(-mprofile-kernel), gcc must always assume that global function (that will
typically have just one instance for the whole address space) will be
called.
--
Jiri Kosina
SUSE Labs
The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
allows to call _mcount very early in the function, which low-level
ASM code and code patching functions need to consider.
Especially the link register and the parameter registers are still
alive and not yet saved into a new stack frame.
I'm thinking of implementing live patch support *for arm64*, and as part of
those efforts, we are proposing[1] a new *generic* gcc option, -fprolog-add=N.
This option will insert N nop instructions at the beginning of each function.
So we have to initialize those codes at the boot time to later utilize
them for FTRACE_WITH_REGS. Other than that, it will work similarly
with -mfentry on x86 (and -mprofile-kernel?).
I'm totally unfamiliar with ppc architecture, but just wondering
whether this option will also be useful for other architectures.
I will really appreciate you if you share your thoughts with me, please?
[1] https://gcc.gnu.org/ml/gcc/2015-05/msg00267.html, and
https://gcc.gnu.org/ml/gcc/2015-10/msg00090.html
Thanks,
-Takahiro AKASHI
@@ -306,11 +306,19 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)*TheloadoffsetisdifferentdependingontheABI.Forsimplicity*justmaskitoutwhendoingthecompare.*/+#ifndef CC_USING_MPROFILE_KERNELif((op[0]!=0x48000008)||((op[1]&0xffff0000)!=0xe8410000)){-pr_err("Unexpected call sequence: %x %x\n",op[0],op[1]);+pr_err("Unexpected call sequence at %p: %x %x\n",+ip,op[0],op[1]);return-EINVAL;}-+#else+/* look for patched "NOP" on ppc64 with -mprofile-kernel */+if(op[0]!=0x60000000){+pr_err("Unexpected call at %p: %x\n",ip,op[0]);+return-EINVAL;+}+#endif/* If we never set up a trampoline to ftrace_caller, then bail */if(!rec->arch.mod->arch.tramp){pr_err("No ftrace trampoline\n");
The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
allows to call _mcount very early in the function, which low-level
ASM code and code patching functions need to consider.
Especially the link register and the parameter registers are still
alive and not yet saved into a new stack frame.
I'm thinking of implementing live patch support *for arm64*, and as part of
those efforts, we are proposing[1] a new *generic* gcc option, -fprolog-add=N.
This option will insert N nop instructions at the beginning of each function.
So we have to initialize those codes at the boot time to later utilize
them for FTRACE_WITH_REGS. Other than that, it will work similarly
with -mfentry on x86 (and -mprofile-kernel?).
I'm totally unfamiliar with ppc architecture, but just wondering
whether this option will also be useful for other architectures.
The interesting part of the story with ppc64 is that you indeed want to
create the callsite before the *most* of the prologue, but not really :)
The part of the prologue where TOC pointer is saved needs to happen before
the fentry/profiling call.
I don't think this will be an issue on ARM64, but it definitely is
something that should be taken into account in case the gcc option is
meant to be really generic.
Thanks,
--
Jiri Kosina
SUSE Labs
On Wed, Feb 03, 2016 at 09:55:11AM +0100, Jiri Kosina wrote:
On Wed, 3 Feb 2016, AKASHI Takahiro wrote:
quoted
those efforts, we are proposing[1] a new *generic* gcc option, -fprolog-add=N.
This option will insert N nop instructions at the beginning of each function.
The interesting part of the story with ppc64 is that you indeed want to
create the callsite before the *most* of the prologue, but not really :)
I was silently assuming that GCC would do this right on ppc64le; add the NOPs
right after the TOC load. Or after TOC load and LR save? ...
The part of the prologue where TOC pointer is saved needs to happen before
the fentry/profiling call.
Yes, any call, to any profiler/tracer/live patcher is potentially global
and needs the _new_ TOC value.
This proposal, if implemented in a too naive fashion, will worsen the problem
we currently discuss: a few NOPs _never_ cause any global reference. GCC might
be even more inclined to not load a new TOC value. That change would need to be
fairly smart on ppc64le.
Torsten
Jiri, Torsten
Thank you for your explanation.
On 02/03/2016 08:24 PM, Torsten Duwe wrote:
On Wed, Feb 03, 2016 at 09:55:11AM +0100, Jiri Kosina wrote:
quoted
On Wed, 3 Feb 2016, AKASHI Takahiro wrote:
quoted
those efforts, we are proposing[1] a new *generic* gcc option, -fprolog-add=N.
This option will insert N nop instructions at the beginning of each function.
quoted
The interesting part of the story with ppc64 is that you indeed want to
create the callsite before the *most* of the prologue, but not really :)
I was silently assuming that GCC would do this right on ppc64le; add the NOPs
right after the TOC load. Or after TOC load and LR save? ...
On arm/arm64, link register must be saved before any function call. So anyhow
we will have to add something, 3 instructions at the minimum, like:
save lr
branch _mcount
restore lr
<prologue>
...
<body>
...
quoted
The part of the prologue where TOC pointer is saved needs to happen before
the fentry/profiling call.
Yes, any call, to any profiler/tracer/live patcher is potentially global
and needs the _new_ TOC value.
I don't want to bother you, but for my better understandings, could you show me
an example of asm instructions for a function prologue under -mprofile-kernel, please?
-Takahiro AKASHI
This proposal, if implemented in a too naive fashion, will worsen the problem
we currently discuss: a few NOPs _never_ cause any global reference. GCC might
be even more inclined to not load a new TOC value. That change would need to be
fairly smart on ppc64le.
Torsten
From: Petr Mladek <pmladek@suse.com> Date: 2016-02-04 11:03:47
On Thu 2016-02-04 18:31:40, AKASHI Takahiro wrote:
Jiri, Torsten
Thank you for your explanation.
On 02/03/2016 08:24 PM, Torsten Duwe wrote:
quoted
On Wed, Feb 03, 2016 at 09:55:11AM +0100, Jiri Kosina wrote:
quoted
On Wed, 3 Feb 2016, AKASHI Takahiro wrote:
quoted
those efforts, we are proposing[1] a new *generic* gcc option, -fprolog-add=N.
This option will insert N nop instructions at the beginning of each function.
quoted
The interesting part of the story with ppc64 is that you indeed want to
create the callsite before the *most* of the prologue, but not really :)
I was silently assuming that GCC would do this right on ppc64le; add the NOPs
right after the TOC load. Or after TOC load and LR save? ...
On arm/arm64, link register must be saved before any function call. So anyhow
we will have to add something, 3 instructions at the minimum, like:
save lr
branch _mcount
restore lr
<prologue>
...
<body>
...
So, it is similar to PPC that has to handle LR as well.
quoted
quoted
The part of the prologue where TOC pointer is saved needs to happen before
the fentry/profiling call.
Yes, any call, to any profiler/tracer/live patcher is potentially global
and needs the _new_ TOC value.
The code below is generated for PPC64LE with -mprofile-kernel using:
$> gcc --version
gcc (SUSE Linux) 6.0.0 20160121 (experimental) [trunk revision 232670]
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
0000000000000050 <cmdline_proc_show>:
50: 00 00 4c 3c addis r2,r12,0
50: R_PPC64_REL16_HA .TOC.
54: 00 00 42 38 addi r2,r2,0
54: R_PPC64_REL16_LO .TOC.+0x4
58: a6 02 08 7c mflr r0
5c: 01 00 00 48 bl 5c <cmdline_proc_show+0xc>
5c: R_PPC64_REL24 _mcount
60: a6 02 08 7c mflr r0
64: 10 00 01 f8 std r0,16(r1)
68: a1 ff 21 f8 stdu r1,-96(r1)
6c: 00 00 22 3d addis r9,r2,0
6c: R_PPC64_TOC16_HA .toc
70: 00 00 82 3c addis r4,r2,0
70: R_PPC64_TOC16_HA .rodata.str1.8
74: 00 00 29 e9 ld r9,0(r9)
74: R_PPC64_TOC16_LO_DS .toc
78: 00 00 84 38 addi r4,r4,0
78: R_PPC64_TOC16_LO .rodata.str1.8
7c: 00 00 a9 e8 ld r5,0(r9)
80: 01 00 00 48 bl 80 <cmdline_proc_show+0x30>
80: R_PPC64_REL24 seq_printf
84: 00 00 00 60 nop
88: 00 00 60 38 li r3,0
8c: 60 00 21 38 addi r1,r1,96
90: 10 00 01 e8 ld r0,16(r1)
94: a6 03 08 7c mtlr r0
98: 20 00 80 4e blr
And the same function compiled using:
$> gcc --version
gcc (SUSE Linux) 4.8.5
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
0000000000000050 <cmdline_proc_show>:
50: 00 00 4c 3c addis r2,r12,0
50: R_PPC64_REL16_HA .TOC.
54: 00 00 42 38 addi r2,r2,0
54: R_PPC64_REL16_LO .TOC.+0x4
58: a6 02 08 7c mflr r0
5c: 10 00 01 f8 std r0,16(r1)
60: 01 00 00 48 bl 60 <cmdline_proc_show+0x10>
60: R_PPC64_REL24 _mcount
64: a6 02 08 7c mflr r0
68: 10 00 01 f8 std r0,16(r1)
6c: a1 ff 21 f8 stdu r1,-96(r1)
70: 00 00 42 3d addis r10,r2,0
70: R_PPC64_TOC16_HA .toc
74: 00 00 82 3c addis r4,r2,0
74: R_PPC64_TOC16_HA .rodata.str1.8
78: 00 00 2a e9 ld r9,0(r10)
78: R_PPC64_TOC16_LO_DS .toc
7c: 00 00 84 38 addi r4,r4,0
7c: R_PPC64_TOC16_LO .rodata.str1.8
80: 00 00 a9 e8 ld r5,0(r9)
84: 01 00 00 48 bl 84 <cmdline_proc_show+0x34>
84: R_PPC64_REL24 seq_printf
88: 00 00 00 60 nop
8c: 00 00 60 38 li r3,0
90: 60 00 21 38 addi r1,r1,96
94: 10 00 01 e8 ld r0,16(r1)
98: a6 03 08 7c mtlr r0
9c: 20 00 80 4e blr
Please, note that are used either 3 or 4 instructions before the
mcount location depending on the compiler version.
Best Regards,
Petr
On arm/arm64, link register must be saved before any function call. So anyhow
we will have to add something, 3 instructions at the minimum, like:
save lr
branch _mcount
restore lr
<prologue>
...
<body>
...
This means that we have at least two architectures that need one
instruction before the mcount/mfentry call, and the rest of the prologue
to follow afterwards. On x86, we don't need any "pre-prologue".
Persumably the corresponding opcodes have different sizes. This nicely
demonstrates my point -- if this one-gcc-option-to-rule-them-all would
exist, it needs to be generic enough to describe these kinds of
constraints (who knows what other restrictions will pop up when exploring
other, more exotic, architectures later).
Thanks,
--
Jiri Kosina
SUSE Labs
On Thu, Feb 4, 2016 at 10:02 PM, Petr Mladek [off-list ref] wrote:
On Thu 2016-02-04 18:31:40, AKASHI Takahiro wrote:
quoted
Jiri, Torsten
Thank you for your explanation.
On 02/03/2016 08:24 PM, Torsten Duwe wrote:
quoted
On Wed, Feb 03, 2016 at 09:55:11AM +0100, Jiri Kosina wrote:
quoted
On Wed, 3 Feb 2016, AKASHI Takahiro wrote:
quoted
those efforts, we are proposing[1] a new *generic* gcc option, -fprolog-add=N.
This option will insert N nop instructions at the beginning of each function.
quoted
The interesting part of the story with ppc64 is that you indeed want to
create the callsite before the *most* of the prologue, but not really :)
I was silently assuming that GCC would do this right on ppc64le; add the NOPs
right after the TOC load. Or after TOC load and LR save? ...
On arm/arm64, link register must be saved before any function call. So anyhow
we will have to add something, 3 instructions at the minimum, like:
save lr
branch _mcount
restore lr
<prologue>
...
<body>
...
So, it is similar to PPC that has to handle LR as well.
quoted
quoted
quoted
The part of the prologue where TOC pointer is saved needs to happen before
the fentry/profiling call.
Yes, any call, to any profiler/tracer/live patcher is potentially global
and needs the _new_ TOC value.
The code below is generated for PPC64LE with -mprofile-kernel using:
$> gcc --version
gcc (SUSE Linux) 6.0.0 20160121 (experimental) [trunk revision 232670]
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
0000000000000050 <cmdline_proc_show>:
50: 00 00 4c 3c addis r2,r12,0
50: R_PPC64_REL16_HA .TOC.
54: 00 00 42 38 addi r2,r2,0
54: R_PPC64_REL16_LO .TOC.+0x4
58: a6 02 08 7c mflr r0
5c: 01 00 00 48 bl 5c <cmdline_proc_show+0xc>
5c: R_PPC64_REL24 _mcount
60: a6 02 08 7c mflr r0
64: 10 00 01 f8 std r0,16(r1)
68: a1 ff 21 f8 stdu r1,-96(r1)
6c: 00 00 22 3d addis r9,r2,0
6c: R_PPC64_TOC16_HA .toc
70: 00 00 82 3c addis r4,r2,0
70: R_PPC64_TOC16_HA .rodata.str1.8
74: 00 00 29 e9 ld r9,0(r9)
74: R_PPC64_TOC16_LO_DS .toc
78: 00 00 84 38 addi r4,r4,0
78: R_PPC64_TOC16_LO .rodata.str1.8
7c: 00 00 a9 e8 ld r5,0(r9)
80: 01 00 00 48 bl 80 <cmdline_proc_show+0x30>
80: R_PPC64_REL24 seq_printf
84: 00 00 00 60 nop
88: 00 00 60 38 li r3,0
8c: 60 00 21 38 addi r1,r1,96
90: 10 00 01 e8 ld r0,16(r1)
94: a6 03 08 7c mtlr r0
98: 20 00 80 4e blr
And the same function compiled using:
$> gcc --version
gcc (SUSE Linux) 4.8.5
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
0000000000000050 <cmdline_proc_show>:
50: 00 00 4c 3c addis r2,r12,0
50: R_PPC64_REL16_HA .TOC.
54: 00 00 42 38 addi r2,r2,0
54: R_PPC64_REL16_LO .TOC.+0x4
58: a6 02 08 7c mflr r0
5c: 10 00 01 f8 std r0,16(r1)
60: 01 00 00 48 bl 60 <cmdline_proc_show+0x10>
60: R_PPC64_REL24 _mcount
64: a6 02 08 7c mflr r0
68: 10 00 01 f8 std r0,16(r1)
6c: a1 ff 21 f8 stdu r1,-96(r1)
70: 00 00 42 3d addis r10,r2,0
70: R_PPC64_TOC16_HA .toc
74: 00 00 82 3c addis r4,r2,0
74: R_PPC64_TOC16_HA .rodata.str1.8
78: 00 00 2a e9 ld r9,0(r10)
78: R_PPC64_TOC16_LO_DS .toc
7c: 00 00 84 38 addi r4,r4,0
7c: R_PPC64_TOC16_LO .rodata.str1.8
80: 00 00 a9 e8 ld r5,0(r9)
84: 01 00 00 48 bl 84 <cmdline_proc_show+0x34>
84: R_PPC64_REL24 seq_printf
88: 00 00 00 60 nop
8c: 00 00 60 38 li r3,0
90: 60 00 21 38 addi r1,r1,96
94: 10 00 01 e8 ld r0,16(r1)
98: a6 03 08 7c mtlr r0
9c: 20 00 80 4e blr
Please, note that are used either 3 or 4 instructions before the
mcount location depending on the compiler version.
Thanks Petr
For big endian builds I saw
Dump of assembler code for function alloc_pages_current:
0xc000000000256f00 <+0>: mflr r0
0xc000000000256f04 <+4>: std r0,16(r1)
0xc000000000256f08 <+8>: bl 0xc000000000009e5c <.mcount>
0xc000000000256f0c <+12>: mflr r0
The offset is 8 bytes. Your earlier patch handled this by adding 16, I
suspect it needs revisiting
Balbir
From: Petr Mladek <pmladek@suse.com> Date: 2016-02-05 10:22:31
On Fri 2016-02-05 15:40:27, Balbir Singh wrote:
On Thu, Feb 4, 2016 at 10:02 PM, Petr Mladek [off-list ref] wrote:
For big endian builds I saw
Dump of assembler code for function alloc_pages_current:
0xc000000000256f00 <+0>: mflr r0
0xc000000000256f04 <+4>: std r0,16(r1)
0xc000000000256f08 <+8>: bl 0xc000000000009e5c <.mcount>
0xc000000000256f0c <+12>: mflr r0
The offset is 8 bytes. Your earlier patch handled this by adding 16, I
suspect it needs revisiting
It seems to be one of the funcitons that do not access any global
symbol. gcc does not produce TOC handling in this case when
compiled with -mprofile-kernel. I believe that it is a gcc bug.
More details can be found in the thread starting at
http://thread.gmane.org/gmane.linux.kernel/2134759/focus=2141996
Best Regards,
Petr