* [PATCH 0/3] arm64 live patching
@ 2018-08-10 16:00 Torsten Duwe
2018-08-10 16:02 ` [PATCH 1/3] arm64: implement ftrace with regs Torsten Duwe
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Torsten Duwe @ 2018-08-10 16:00 UTC (permalink / raw)
To: Will Deacon, Catalin Marinas, Julien Thierry, Steven Rostedt,
Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro
Cc: linux-arm-kernel, linux-kernel, live-patching
Hi all,
with gcc-8 now being out which includes the patchable-function-entries feature,
I can now propose the live patching framework based on it. The series consists
of 3 parts:
1st: Implement ftrace with regs -- uses gcc-8's nop insertions to patch in
ftrace calls.
2nd: "Classic" live patching infrastructure, as far as it's needed for backporting.
3rd: An attempt at reliable stack traces for the consistency model. I'm a bit
uncertain whether the approach is correct.
Torsten
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] arm64: implement ftrace with regs 2018-08-10 16:00 [PATCH 0/3] arm64 live patching Torsten Duwe @ 2018-08-10 16:02 ` Torsten Duwe 2018-08-10 19:27 ` Steven Rostedt 2018-08-13 10:54 ` Julien Thierry 2018-08-10 16:03 ` [PATCH 2/3] arm64: implement live patching Torsten Duwe 2018-08-10 16:03 ` [PATCH 3/3] arm64: reliable stacktraces Torsten Duwe 2 siblings, 2 replies; 12+ messages in thread From: Torsten Duwe @ 2018-08-10 16:02 UTC (permalink / raw) To: Will Deacon, Catalin Marinas, Julien Thierry, Steven Rostedt, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro Cc: linux-arm-kernel, linux-kernel, live-patching Check for compiler support of -fpatchable-function-entry and use it to intercept functions immediately on entry, saving the LR in x9. Disable ftracing in efi/libstub, because this triggers cross-section linker errors now (-pg used to be disabled already). Add an ftrace_caller which can handle LR in x9. This code has successfully passed the FTRACE_STARTUP_TEST. Signed-off-by: Torsten Duwe <duwe@suse.de> --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -110,6 +110,7 @@ config ARM64 select HAVE_DEBUG_KMEMLEAK select HAVE_DMA_CONTIGUOUS select HAVE_DYNAMIC_FTRACE + select HAVE_DYNAMIC_FTRACE_WITH_REGS select HAVE_EFFICIENT_UNALIGNED_ACCESS select HAVE_FTRACE_MCOUNT_RECORD select HAVE_FUNCTION_TRACER --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -78,6 +78,15 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y) KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/arm64/kernel/module.lds endif +ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS + CC_FLAGS_FTRACE := -fpatchable-function-entry=2 + KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY + ifeq ($(call cc-option,-fpatchable-function-entry=2),) + $(warning Cannot use CONFIG_DYNAMIC_FTRACE_WITH_REGS: \ + -fpatchable-function-entry not supported by compiler) + endif +endif + # Default value head-y := arch/arm64/kernel/head.o --- a/arch/arm64/include/asm/ftrace.h +++ b/arch/arm64/include/asm/ftrace.h @@ -16,6 +16,14 @@ #define MCOUNT_ADDR ((unsigned long)_mcount) #define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS +#define ARCH_SUPPORTS_FTRACE_OPS 1 +#define REC_IP_BRANCH_OFFSET 4 +#define FTRACE_REGS_ADDR FTRACE_ADDR +#else +#define REC_IP_BRANCH_OFFSET 0 +#endif + #ifndef __ASSEMBLY__ #include <linux/compat.h> --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -7,9 +7,9 @@ CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$( AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET) CFLAGS_armv8_deprecated.o := -I$(src) -CFLAGS_REMOVE_ftrace.o = -pg -CFLAGS_REMOVE_insn.o = -pg -CFLAGS_REMOVE_return_address.o = -pg +CFLAGS_REMOVE_ftrace.o = -pg $(CC_FLAGS_FTRACE) +CFLAGS_REMOVE_insn.o = -pg $(CC_FLAGS_FTRACE) +CFLAGS_REMOVE_return_address.o = -pg $(CC_FLAGS_FTRACE) # Object file lists. arm64-obj-y := debug-monitors.o entry.o irq.o fpsimd.o \ --- a/drivers/firmware/efi/libstub/Makefile +++ b/drivers/firmware/efi/libstub/Makefile @@ -11,7 +11,8 @@ -fPIC -fno-strict-aliasing -mno-red-zone \ -mno-mmx -mno-sse -fshort-wchar -cflags-$(CONFIG_ARM64) := $(subst -pg,,$(KBUILD_CFLAGS)) -fpie +cflags-$(CONFIG_ARM64) := $(filter-out -pg $(CC_FLAGS_FTRACE)\ + ,$(KBUILD_CFLAGS)) -fpie cflags-$(CONFIG_ARM) := $(subst -pg,,$(KBUILD_CFLAGS)) \ -fno-builtin -fpic -mno-single-pic-base --- a/arch/arm64/kernel/entry-ftrace.S +++ b/arch/arm64/kernel/entry-ftrace.S @@ -13,6 +13,8 @@ #include <asm/assembler.h> #include <asm/ftrace.h> #include <asm/insn.h> +#include <asm/asm-offsets.h> +#include <asm/assembler.h> /* * Gcc with -pg will put the following code in the beginning of each function: @@ -123,6 +125,7 @@ skip_ftrace_call: // } ENDPROC(_mcount) #else /* CONFIG_DYNAMIC_FTRACE */ +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS /* * _mcount() is used to build the kernel with -pg option, but all the branch * instructions to _mcount() are replaced to NOP initially at kernel start up, @@ -162,6 +165,84 @@ ftrace_graph_call: // ftrace_graph_cal mcount_exit ENDPROC(ftrace_caller) +#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ +ENTRY(_mcount) + mov x10, lr + mov lr, x9 + ret x10 +ENDPROC(_mcount) + +ENTRY(ftrace_caller) + stp x29, x9, [sp, #-16]! + sub sp, sp, #S_FRAME_SIZE + + stp x0, x1, [sp] + stp x2, x3, [sp, #16] + stp x4, x5, [sp, #32] + stp x6, x7, [sp, #48] + stp x8, x9, [sp, #64] + stp x10, x11, [sp, #80] + stp x12, x13, [sp, #96] + stp x14, x15, [sp, #112] + stp x16, x17, [sp, #128] + stp x18, x19, [sp, #144] + stp x20, x21, [sp, #160] + stp x22, x23, [sp, #176] + stp x24, x25, [sp, #192] + stp x26, x27, [sp, #208] + stp x28, x29, [sp, #224] + /* The link Register at callee entry */ + str x9, [sp, #S_LR] + /* The program counter just after the ftrace call site */ + str lr, [sp, #S_PC] + /* The stack pointer as it was on ftrace_caller entry... */ + add x29, sp, #S_FRAME_SIZE+16 /* ...is also our new FP */ + str x29, [sp, #S_SP] + + adrp x0, function_trace_op + ldr x2, [x0, #:lo12:function_trace_op] + mov x1, x9 /* saved LR == parent IP */ + sub x0, lr, #8 /* function entry == IP */ + mov x3, sp /* complete pt_regs are @sp */ + + .global ftrace_call +ftrace_call: + + bl ftrace_stub + +#ifdef CONFIG_FUNCTION_GRAPH_TRACER + .global ftrace_graph_call +ftrace_graph_call: // ftrace_graph_caller(); + nop // If enabled, this will be replaced + // "b ftrace_graph_caller" +#endif + +ftrace_regs_return: + ldp x0, x1, [sp] + ldp x2, x3, [sp, #16] + ldp x4, x5, [sp, #32] + ldp x6, x7, [sp, #48] + ldp x8, x9, [sp, #64] + ldp x10, x11, [sp, #80] + ldp x12, x13, [sp, #96] + ldp x14, x15, [sp, #112] + ldp x16, x17, [sp, #128] + ldp x18, x19, [sp, #144] + ldp x20, x21, [sp, #160] + ldp x22, x23, [sp, #176] + ldp x24, x25, [sp, #192] + ldp x26, x27, [sp, #208] + ldp x28, x29, [sp, #224] + + ldr x9, [sp, #S_PC] + ldr lr, [sp, #S_LR] + add sp, sp, #S_FRAME_SIZE+16 + + ret x9 + +ENDPROC(ftrace_caller) + +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ #endif /* CONFIG_DYNAMIC_FTRACE */ ENTRY(ftrace_stub) @@ -197,12 +278,20 @@ ENDPROC(ftrace_stub) * and run return_to_handler() later on its exit. */ ENTRY(ftrace_graph_caller) +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS mcount_get_lr_addr x0 // pointer to function's saved lr mcount_get_pc x1 // function's pc mcount_get_parent_fp x2 // parent's fp bl prepare_ftrace_return // prepare_ftrace_return(&lr, pc, fp) mcount_exit +#else + add x0, sp, #S_LR /* address of (LR pointing into caller) */ + ldr x1, [sp, #S_PC] + ldr x2, [sp, #232] /* caller's frame pointer */ + bl prepare_ftrace_return + b ftrace_regs_return +#endif ENDPROC(ftrace_graph_caller) /* --- a/arch/arm64/kernel/ftrace.c +++ b/arch/arm64/kernel/ftrace.c @@ -70,7 +70,8 @@ int ftrace_update_ftrace_func(ftrace_fun */ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) { - unsigned long pc = rec->ip; + unsigned long pc = rec->ip+REC_IP_BRANCH_OFFSET; + int ret; u32 old, new; long offset = (long)pc - (long)addr; @@ -127,23 +128,51 @@ int ftrace_make_call(struct dyn_ftrace * #endif /* CONFIG_ARM64_MODULE_PLTS */ } - old = aarch64_insn_gen_nop(); +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS + old = aarch64_insn_gen_nop(); + new = 0xaa1e03e9; /* mov x9,x30 */ + ret = ftrace_modify_code(pc-REC_IP_BRANCH_OFFSET, old, new, true); + if (ret) + return ret; + smp_wmb(); /* ensure LR saver is in place before ftrace call */ +#endif new = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK); return ftrace_modify_code(pc, old, new, true); } +int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, + unsigned long addr) +{ + unsigned long pc = rec->ip+REC_IP_BRANCH_OFFSET; + u32 old, new; + + old = aarch64_insn_gen_branch_imm(pc, old_addr, true); + new = aarch64_insn_gen_branch_imm(pc, addr, true); + + return ftrace_modify_code(pc, old, new, true); +} + /* * Turn off the call to ftrace_caller() in instrumented function */ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long addr) { - unsigned long pc = rec->ip; + unsigned long pc = rec->ip+REC_IP_BRANCH_OFFSET; bool validate = true; + int ret; u32 old = 0, new; long offset = (long)pc - (long)addr; +#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY + /* -fpatchable-function-entry= does not generate a profiling call + * initially; the NOPs are already there. + */ + if (addr == MCOUNT_ADDR) + return 0; +#endif + if (offset < -SZ_128M || offset >= SZ_128M) { #ifdef CONFIG_ARM64_MODULE_PLTS u32 replaced; @@ -188,7 +218,16 @@ int ftrace_make_nop(struct module *mod, new = aarch64_insn_gen_nop(); - return ftrace_modify_code(pc, old, new, validate); + ret = ftrace_modify_code(pc, old, new, validate); + if (ret) + return ret; +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS + smp_wmb(); /* ftrace call must not remain without LR saver. */ + old = 0xaa1e03e9; /* mov x9,x30 */ + new = aarch64_insn_gen_nop(); + ret = ftrace_modify_code(pc-REC_IP_BRANCH_OFFSET, old, new, true); +#endif + return ret; } void arch_ftrace_update_code(int command) --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -115,6 +115,7 @@ #define MCOUNT_REC() . = ALIGN(8); \ __start_mcount_loc = .; \ KEEP(*(__mcount_loc)) \ + KEEP(*(__patchable_function_entries)) \ __stop_mcount_loc = .; #else #define MCOUNT_REC() --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -61,8 +61,12 @@ extern void __chk_io_ptr(const volatile #if defined(CC_USING_HOTPATCH) && !defined(__CHECKER__) #define notrace __attribute__((hotpatch(0,0))) #else +#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY +#define notrace __attribute__((patchable_function_entry(0))) +#else #define notrace __attribute__((no_instrument_function)) #endif +#endif /* Intel compiler defines __GNUC__. So we will overwrite implementations * coming from above header files here ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] arm64: implement ftrace with regs 2018-08-10 16:02 ` [PATCH 1/3] arm64: implement ftrace with regs Torsten Duwe @ 2018-08-10 19:27 ` Steven Rostedt 2018-08-13 10:54 ` Julien Thierry 1 sibling, 0 replies; 12+ messages in thread From: Steven Rostedt @ 2018-08-10 19:27 UTC (permalink / raw) To: Torsten Duwe Cc: Will Deacon, Catalin Marinas, Julien Thierry, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro, linux-arm-kernel, linux-kernel, live-patching On Fri, 10 Aug 2018 18:02:23 +0200 (CEST) Torsten Duwe <duwe@lst.de> wrote: > --- a/arch/arm64/kernel/entry-ftrace.S > +++ b/arch/arm64/kernel/entry-ftrace.S > @@ -13,6 +13,8 @@ > #include <asm/assembler.h> > #include <asm/ftrace.h> > #include <asm/insn.h> > +#include <asm/asm-offsets.h> > +#include <asm/assembler.h> > > /* > * Gcc with -pg will put the following code in the beginning of each function: > @@ -123,6 +125,7 @@ skip_ftrace_call: // } > ENDPROC(_mcount) > > #else /* CONFIG_DYNAMIC_FTRACE */ > +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS > /* > * _mcount() is used to build the kernel with -pg option, but all the branch > * instructions to _mcount() are replaced to NOP initially at kernel start up, > @@ -162,6 +165,84 @@ ftrace_graph_call: // ftrace_graph_cal > > mcount_exit > ENDPROC(ftrace_caller) > +#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ > +ENTRY(_mcount) > + mov x10, lr > + mov lr, x9 > + ret x10 > +ENDPROC(_mcount) > + > +ENTRY(ftrace_caller) > + stp x29, x9, [sp, #-16]! > + sub sp, sp, #S_FRAME_SIZE > + > + stp x0, x1, [sp] > + stp x2, x3, [sp, #16] > + stp x4, x5, [sp, #32] > + stp x6, x7, [sp, #48] > + stp x8, x9, [sp, #64] > + stp x10, x11, [sp, #80] > + stp x12, x13, [sp, #96] > + stp x14, x15, [sp, #112] > + stp x16, x17, [sp, #128] > + stp x18, x19, [sp, #144] > + stp x20, x21, [sp, #160] > + stp x22, x23, [sp, #176] > + stp x24, x25, [sp, #192] > + stp x26, x27, [sp, #208] > + stp x28, x29, [sp, #224] Wait! You are having ftrace_caller *always* perform the regs save? Why? This is why I have a ftrace_caller and a ftrace_regs_caller, so that normal function tracing doesn't take the overhead hit of tracing all functions. And note, the generic code will differentiate per function. The regs calling is usually only done for a handful of functions, the normal tracing of all functions doesn't need it. Or am I missing something? > + /* The link Register at callee entry */ > + str x9, [sp, #S_LR] > + /* The program counter just after the ftrace call site */ > + str lr, [sp, #S_PC] > + /* The stack pointer as it was on ftrace_caller entry... */ > + add x29, sp, #S_FRAME_SIZE+16 /* ...is also our new FP */ > + str x29, [sp, #S_SP] > + > + adrp x0, function_trace_op > + ldr x2, [x0, #:lo12:function_trace_op] > + mov x1, x9 /* saved LR == parent IP */ > + sub x0, lr, #8 /* function entry == IP */ > + mov x3, sp /* complete pt_regs are @sp */ > + > + .global ftrace_call > +ftrace_call: > + > + bl ftrace_stub > + > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + .global ftrace_graph_call > +ftrace_graph_call: // ftrace_graph_caller(); > + nop // If enabled, this will be replaced > + // "b ftrace_graph_caller" > +#endif > + > +ftrace_regs_return: > + ldp x0, x1, [sp] > + ldp x2, x3, [sp, #16] > + ldp x4, x5, [sp, #32] > + ldp x6, x7, [sp, #48] > + ldp x8, x9, [sp, #64] > + ldp x10, x11, [sp, #80] > + ldp x12, x13, [sp, #96] > + ldp x14, x15, [sp, #112] > + ldp x16, x17, [sp, #128] > + ldp x18, x19, [sp, #144] > + ldp x20, x21, [sp, #160] > + ldp x22, x23, [sp, #176] > + ldp x24, x25, [sp, #192] > + ldp x26, x27, [sp, #208] > + ldp x28, x29, [sp, #224] > + > + ldr x9, [sp, #S_PC] > + ldr lr, [sp, #S_LR] > + add sp, sp, #S_FRAME_SIZE+16 > + > + ret x9 > + > +ENDPROC(ftrace_caller) > + > +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ > #endif /* CONFIG_DYNAMIC_FTRACE */ > > ENTRY(ftrace_stub) > @@ -197,12 +278,20 @@ ENDPROC(ftrace_stub) > * and run return_to_handler() later on its exit. > */ > ENTRY(ftrace_graph_caller) > +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS > mcount_get_lr_addr x0 // pointer to function's saved lr > mcount_get_pc x1 // function's pc > mcount_get_parent_fp x2 // parent's fp > bl prepare_ftrace_return // prepare_ftrace_return(&lr, pc, fp) > > mcount_exit > +#else > + add x0, sp, #S_LR /* address of (LR pointing into caller) */ > + ldr x1, [sp, #S_PC] > + ldr x2, [sp, #232] /* caller's frame pointer */ > + bl prepare_ftrace_return > + b ftrace_regs_return > +#endif > ENDPROC(ftrace_graph_caller) > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] arm64: implement ftrace with regs 2018-08-10 16:02 ` [PATCH 1/3] arm64: implement ftrace with regs Torsten Duwe 2018-08-10 19:27 ` Steven Rostedt @ 2018-08-13 10:54 ` Julien Thierry 2018-08-14 2:03 ` Steven Rostedt 1 sibling, 1 reply; 12+ messages in thread From: Julien Thierry @ 2018-08-13 10:54 UTC (permalink / raw) To: Torsten Duwe, Will Deacon, Catalin Marinas, Steven Rostedt, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro Cc: linux-arm-kernel, linux-kernel, live-patching Hi Torsten, On 10/08/18 17:02, Torsten Duwe wrote: > Check for compiler support of -fpatchable-function-entry and use it > to intercept functions immediately on entry, saving the LR in x9. > Disable ftracing in efi/libstub, because this triggers cross-section > linker errors now (-pg used to be disabled already). > Add an ftrace_caller which can handle LR in x9. > This code has successfully passed the FTRACE_STARTUP_TEST. > > Signed-off-by: Torsten Duwe <duwe@suse.de> > > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -110,6 +110,7 @@ config ARM64 > select HAVE_DEBUG_KMEMLEAK > select HAVE_DMA_CONTIGUOUS > select HAVE_DYNAMIC_FTRACE > + select HAVE_DYNAMIC_FTRACE_WITH_REGS > select HAVE_EFFICIENT_UNALIGNED_ACCESS > select HAVE_FTRACE_MCOUNT_RECORD > select HAVE_FUNCTION_TRACER > --- a/arch/arm64/Makefile > +++ b/arch/arm64/Makefile > @@ -78,6 +78,15 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y) > KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/arm64/kernel/module.lds > endif > > +ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > + CC_FLAGS_FTRACE := -fpatchable-function-entry=2 > + KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY > + ifeq ($(call cc-option,-fpatchable-function-entry=2),) > + $(warning Cannot use CONFIG_DYNAMIC_FTRACE_WITH_REGS: \ > + -fpatchable-function-entry not supported by compiler) Shouldn't this be an error? The option -fpatchable-function-entry has been added to the CC_FLAGS_FTRACE, so any call to the compiler is gonna break anyway. Or am I missing something? > + endif > +endif > + > # Default value > head-y := arch/arm64/kernel/head.o > > --- a/arch/arm64/include/asm/ftrace.h > +++ b/arch/arm64/include/asm/ftrace.h > @@ -16,6 +16,14 @@ > #define MCOUNT_ADDR ((unsigned long)_mcount) > #define MCOUNT_INSN_SIZE AARCH64_INSN_SIZE > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > +#define ARCH_SUPPORTS_FTRACE_OPS 1 > +#define REC_IP_BRANCH_OFFSET 4 > +#define FTRACE_REGS_ADDR FTRACE_ADDR > +#else > +#define REC_IP_BRANCH_OFFSET 0 > +#endif > + > #ifndef __ASSEMBLY__ > #include <linux/compat.h> > > --- a/arch/arm64/kernel/Makefile > +++ b/arch/arm64/kernel/Makefile > @@ -7,9 +7,9 @@ CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$( > AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET) > CFLAGS_armv8_deprecated.o := -I$(src) > > -CFLAGS_REMOVE_ftrace.o = -pg > -CFLAGS_REMOVE_insn.o = -pg > -CFLAGS_REMOVE_return_address.o = -pg > +CFLAGS_REMOVE_ftrace.o = -pg $(CC_FLAGS_FTRACE) > +CFLAGS_REMOVE_insn.o = -pg $(CC_FLAGS_FTRACE) > +CFLAGS_REMOVE_return_address.o = -pg $(CC_FLAGS_FTRACE) > > # Object file lists. > arm64-obj-y := debug-monitors.o entry.o irq.o fpsimd.o \ > --- a/drivers/firmware/efi/libstub/Makefile > +++ b/drivers/firmware/efi/libstub/Makefile > @@ -11,7 +11,8 @@ > -fPIC -fno-strict-aliasing -mno-red-zone \ > -mno-mmx -mno-sse -fshort-wchar > > -cflags-$(CONFIG_ARM64) := $(subst -pg,,$(KBUILD_CFLAGS)) -fpie > +cflags-$(CONFIG_ARM64) := $(filter-out -pg $(CC_FLAGS_FTRACE)\ > + ,$(KBUILD_CFLAGS)) -fpie > cflags-$(CONFIG_ARM) := $(subst -pg,,$(KBUILD_CFLAGS)) \ > -fno-builtin -fpic -mno-single-pic-base > > --- a/arch/arm64/kernel/entry-ftrace.S > +++ b/arch/arm64/kernel/entry-ftrace.S > @@ -13,6 +13,8 @@ > #include <asm/assembler.h> > #include <asm/ftrace.h> > #include <asm/insn.h> > +#include <asm/asm-offsets.h> > +#include <asm/assembler.h> > > /* > * Gcc with -pg will put the following code in the beginning of each function: > @@ -123,6 +125,7 @@ skip_ftrace_call: // } > ENDPROC(_mcount) > > #else /* CONFIG_DYNAMIC_FTRACE */ > +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS > /* > * _mcount() is used to build the kernel with -pg option, but all the branch > * instructions to _mcount() are replaced to NOP initially at kernel start up, > @@ -162,6 +165,84 @@ ftrace_graph_call: // ftrace_graph_cal > > mcount_exit > ENDPROC(ftrace_caller) > +#else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ > +ENTRY(_mcount) > + mov x10, lr > + mov lr, x9 > + ret x10 > +ENDPROC(_mcount) > + > +ENTRY(ftrace_caller) > + stp x29, x9, [sp, #-16]! > + sub sp, sp, #S_FRAME_SIZE > + > + stp x0, x1, [sp] > + stp x2, x3, [sp, #16] > + stp x4, x5, [sp, #32] > + stp x6, x7, [sp, #48] > + stp x8, x9, [sp, #64] > + stp x10, x11, [sp, #80] > + stp x12, x13, [sp, #96] > + stp x14, x15, [sp, #112] > + stp x16, x17, [sp, #128] > + stp x18, x19, [sp, #144] > + stp x20, x21, [sp, #160] > + stp x22, x23, [sp, #176] > + stp x24, x25, [sp, #192] > + stp x26, x27, [sp, #208] > + stp x28, x29, [sp, #224] > + /* The link Register at callee entry */ > + str x9, [sp, #S_LR] > + /* The program counter just after the ftrace call site */ > + str lr, [sp, #S_PC] > + /* The stack pointer as it was on ftrace_caller entry... */ > + add x29, sp, #S_FRAME_SIZE+16 /* ...is also our new FP */ > + str x29, [sp, #S_SP] > + > + adrp x0, function_trace_op > + ldr x2, [x0, #:lo12:function_trace_op] > + mov x1, x9 /* saved LR == parent IP */ > + sub x0, lr, #8 /* function entry == IP */ > + mov x3, sp /* complete pt_regs are @sp */ > + > + .global ftrace_call > +ftrace_call: > + > + bl ftrace_stub > + > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + .global ftrace_graph_call > +ftrace_graph_call: // ftrace_graph_caller(); > + nop // If enabled, this will be replaced > + // "b ftrace_graph_caller" > +#endif > + > +ftrace_regs_return: > + ldp x0, x1, [sp] > + ldp x2, x3, [sp, #16] > + ldp x4, x5, [sp, #32] > + ldp x6, x7, [sp, #48] > + ldp x8, x9, [sp, #64] > + ldp x10, x11, [sp, #80] > + ldp x12, x13, [sp, #96] > + ldp x14, x15, [sp, #112] > + ldp x16, x17, [sp, #128] > + ldp x18, x19, [sp, #144] > + ldp x20, x21, [sp, #160] > + ldp x22, x23, [sp, #176] > + ldp x24, x25, [sp, #192] > + ldp x26, x27, [sp, #208] > + ldp x28, x29, [sp, #224] > + > + ldr x9, [sp, #S_PC] > + ldr lr, [sp, #S_LR] > + add sp, sp, #S_FRAME_SIZE+16 > + > + ret x9 > + > +ENDPROC(ftrace_caller) > + > +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ > #endif /* CONFIG_DYNAMIC_FTRACE */ > > ENTRY(ftrace_stub) > @@ -197,12 +278,20 @@ ENDPROC(ftrace_stub) > * and run return_to_handler() later on its exit. > */ > ENTRY(ftrace_graph_caller) > +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_REGS > mcount_get_lr_addr x0 // pointer to function's saved lr > mcount_get_pc x1 // function's pc > mcount_get_parent_fp x2 // parent's fp > bl prepare_ftrace_return // prepare_ftrace_return(&lr, pc, fp) > > mcount_exit > +#else > + add x0, sp, #S_LR /* address of (LR pointing into caller) */ > + ldr x1, [sp, #S_PC] > + ldr x2, [sp, #232] /* caller's frame pointer */ Nit: Can we change the offset to #S_X28 + 8 ? > + bl prepare_ftrace_return > + b ftrace_regs_return > +#endif > ENDPROC(ftrace_graph_caller) > > /* > --- a/arch/arm64/kernel/ftrace.c > +++ b/arch/arm64/kernel/ftrace.c > @@ -70,7 +70,8 @@ int ftrace_update_ftrace_func(ftrace_fun > */ > int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) > { > - unsigned long pc = rec->ip; > + unsigned long pc = rec->ip+REC_IP_BRANCH_OFFSET; > + int ret; > u32 old, new; > long offset = (long)pc - (long)addr; > > @@ -127,23 +128,51 @@ int ftrace_make_call(struct dyn_ftrace * > #endif /* CONFIG_ARM64_MODULE_PLTS */ > } > > - old = aarch64_insn_gen_nop(); > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > + old = aarch64_insn_gen_nop(); Doesn't moving 'old' in the #ifdef break things when CONFIG_DYNAMIC_FTRACE_WITH_REGS is not defined? I think the last ftrace_modifyi_code() is using it uninitialized in that case. > + new = 0xaa1e03e9; /* mov x9,x30 */ I see there isn't a aarch64_insn_get_mov function, maybe it would be worth adding it. If not you could use: aarch64_insn_gen_add_sub_imm(AARCH64_INSN_REG_9, AARCH64_INSN_REG_30, 0, AARCH64_INSN_VARIANT_64BIT, AARCH64_INSN_ADSB_ADD); > + ret = ftrace_modify_code(pc-REC_IP_BRANCH_OFFSET, old, new, true); > + if (ret) > + return ret; > + smp_wmb(); /* ensure LR saver is in place before ftrace call */ > +#endif > new = aarch64_insn_gen_branch_imm(pc, addr, AARCH64_INSN_BRANCH_LINK); > > return ftrace_modify_code(pc, old, new, true); > } > > +int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, > + unsigned long addr) Is this getting called? I don't see it being referenced. > +{ > + unsigned long pc = rec->ip+REC_IP_BRANCH_OFFSET; > + u32 old, new; > + > + old = aarch64_insn_gen_branch_imm(pc, old_addr, true); > + new = aarch64_insn_gen_branch_imm(pc, addr, true); > + > + return ftrace_modify_code(pc, old, new, true); > +} > + > /* > * Turn off the call to ftrace_caller() in instrumented function > */ > int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, > unsigned long addr) > { > - unsigned long pc = rec->ip; > + unsigned long pc = rec->ip+REC_IP_BRANCH_OFFSET; > bool validate = true; > + int ret; > u32 old = 0, new; > long offset = (long)pc - (long)addr; > > +#ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY > + /* -fpatchable-function-entry= does not generate a profiling call > + * initially; the NOPs are already there. > + */ > + if (addr == MCOUNT_ADDR) > + return 0; > +#endif > + > if (offset < -SZ_128M || offset >= SZ_128M) { > #ifdef CONFIG_ARM64_MODULE_PLTS > u32 replaced; > @@ -188,7 +218,16 @@ int ftrace_make_nop(struct module *mod, > > new = aarch64_insn_gen_nop(); > > - return ftrace_modify_code(pc, old, new, validate); > + ret = ftrace_modify_code(pc, old, new, validate); > + if (ret) > + return ret; > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > + smp_wmb(); /* ftrace call must not remain without LR saver. */ > + old = 0xaa1e03e9; /* mov x9,x30 */ > + new = aarch64_insn_gen_nop(); 'new' is already a nop here. Cheers, -- Julien Thierry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] arm64: implement ftrace with regs 2018-08-13 10:54 ` Julien Thierry @ 2018-08-14 2:03 ` Steven Rostedt 2018-08-14 8:33 ` Julien Thierry 0 siblings, 1 reply; 12+ messages in thread From: Steven Rostedt @ 2018-08-14 2:03 UTC (permalink / raw) To: Julien Thierry Cc: Torsten Duwe, Will Deacon, Catalin Marinas, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro, linux-arm-kernel, linux-kernel, live-patching On Mon, 13 Aug 2018 11:54:06 +0100 Julien Thierry <julien.thierry@arm.com> wrote: > > --- a/arch/arm64/Makefile > > +++ b/arch/arm64/Makefile > > @@ -78,6 +78,15 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y) > > KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/arm64/kernel/module.lds > > endif > > > > +ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > > + CC_FLAGS_FTRACE := -fpatchable-function-entry=2 > > + KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY > > + ifeq ($(call cc-option,-fpatchable-function-entry=2),) > > + $(warning Cannot use CONFIG_DYNAMIC_FTRACE_WITH_REGS: \ > > + -fpatchable-function-entry not supported by compiler) > > Shouldn't this be an error? The option -fpatchable-function-entry has > been added to the CC_FLAGS_FTRACE, so any call to the compiler is gonna > break anyway. Or am I missing something? I'm guessing this adds a more informative message on that error. One will know why -fpatchable-function-entry was added to the CFLAGS. I'm for more informative error messages being a victim of poor error messages causing me to dig deep into the guts of the build infrastructure to figure out simple issues. -- Steve ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] arm64: implement ftrace with regs 2018-08-14 2:03 ` Steven Rostedt @ 2018-08-14 8:33 ` Julien Thierry 2018-08-14 16:04 ` Steven Rostedt 0 siblings, 1 reply; 12+ messages in thread From: Julien Thierry @ 2018-08-14 8:33 UTC (permalink / raw) To: Steven Rostedt Cc: Torsten Duwe, Will Deacon, Catalin Marinas, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro, linux-arm-kernel, linux-kernel, live-patching On 14/08/18 03:03, Steven Rostedt wrote: > On Mon, 13 Aug 2018 11:54:06 +0100 > Julien Thierry <julien.thierry@arm.com> wrote: > >>> --- a/arch/arm64/Makefile >>> +++ b/arch/arm64/Makefile >>> @@ -78,6 +78,15 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y) >>> KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/arm64/kernel/module.lds >>> endif >>> >>> +ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS >>> + CC_FLAGS_FTRACE := -fpatchable-function-entry=2 >>> + KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY >>> + ifeq ($(call cc-option,-fpatchable-function-entry=2),) >>> + $(warning Cannot use CONFIG_DYNAMIC_FTRACE_WITH_REGS: \ >>> + -fpatchable-function-entry not supported by compiler) >> >> Shouldn't this be an error? The option -fpatchable-function-entry has >> been added to the CC_FLAGS_FTRACE, so any call to the compiler is gonna >> break anyway. Or am I missing something? > > I'm guessing this adds a more informative message on that error. One > will know why -fpatchable-function-entry was added to the CFLAGS. I'm > for more informative error messages being a victim of poor error > messages causing me to dig deep into the guts of the build > infrastructure to figure out simple issues. > Yes, I agree it is better to have this message. My point was that we could have "$error" instead of "$warning" to stop the compilation right away since we know everything is gonna break (and on parallel builds this warning is gonna be drowned in compiler errors). -- Julien Thierry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] arm64: implement ftrace with regs 2018-08-14 8:33 ` Julien Thierry @ 2018-08-14 16:04 ` Steven Rostedt 2018-08-15 13:47 ` Torsten Duwe 0 siblings, 1 reply; 12+ messages in thread From: Steven Rostedt @ 2018-08-14 16:04 UTC (permalink / raw) To: Julien Thierry Cc: Torsten Duwe, Will Deacon, Catalin Marinas, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro, linux-arm-kernel, linux-kernel, live-patching On Tue, 14 Aug 2018 09:33:52 +0100 Julien Thierry <julien.thierry@arm.com> wrote: > On 14/08/18 03:03, Steven Rostedt wrote: > > On Mon, 13 Aug 2018 11:54:06 +0100 > > Julien Thierry <julien.thierry@arm.com> wrote: > > > >>> --- a/arch/arm64/Makefile > >>> +++ b/arch/arm64/Makefile > >>> @@ -78,6 +78,15 @@ ifeq ($(CONFIG_ARM64_MODULE_PLTS),y) > >>> KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/arm64/kernel/module.lds > >>> endif > >>> > >>> +ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > >>> + CC_FLAGS_FTRACE := -fpatchable-function-entry=2 > >>> + KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY > >>> + ifeq ($(call cc-option,-fpatchable-function-entry=2),) > >>> + $(warning Cannot use CONFIG_DYNAMIC_FTRACE_WITH_REGS: \ > >>> + -fpatchable-function-entry not supported by compiler) > >> > >> Shouldn't this be an error? The option -fpatchable-function-entry has > >> been added to the CC_FLAGS_FTRACE, so any call to the compiler is gonna > >> break anyway. Or am I missing something? > > > > I'm guessing this adds a more informative message on that error. One > > will know why -fpatchable-function-entry was added to the CFLAGS. I'm > > for more informative error messages being a victim of poor error > > messages causing me to dig deep into the guts of the build > > infrastructure to figure out simple issues. > > > > Yes, I agree it is better to have this message. My point was that we > could have "$error" instead of "$warning" to stop the compilation right > away since we know everything is gonna break (and on parallel builds > this warning is gonna be drowned in compiler errors). > OK, I see what you mean. If the resulting build wont boot, then yes this should be an error and not a warning. -- Steve ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] arm64: implement ftrace with regs 2018-08-14 16:04 ` Steven Rostedt @ 2018-08-15 13:47 ` Torsten Duwe 0 siblings, 0 replies; 12+ messages in thread From: Torsten Duwe @ 2018-08-15 13:47 UTC (permalink / raw) To: Steven Rostedt Cc: Julien Thierry, Will Deacon, Catalin Marinas, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro, linux-arm-kernel, linux-kernel, live-patching [working on V2 with your feedback] On Tue, Aug 14, 2018 at 12:04:33PM -0400, Steven Rostedt wrote: > On Tue, 14 Aug 2018 09:33:52 +0100 > Julien Thierry <julien.thierry@arm.com> wrote: > > >> Shouldn't this be an error? The option -fpatchable-function-entry has > > >> been added to the CC_FLAGS_FTRACE, so any call to the compiler is gonna > > >> break anyway. Or am I missing something? This should be the case. > OK, I see what you mean. If the resulting build wont boot, then yes > this should be an error and not a warning. No, there won't be a binary because the first gcc invocation with CC_FLAGS_FTRACE will error out. The alternatives are a makefile-warning followed by a gcc-error or just a makefile-error. The makefile warning or error should hint at the causing config option, that's the key point. Beyond that I don't have any preference. Torsten ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] arm64: implement live patching 2018-08-10 16:00 [PATCH 0/3] arm64 live patching Torsten Duwe 2018-08-10 16:02 ` [PATCH 1/3] arm64: implement ftrace with regs Torsten Duwe @ 2018-08-10 16:03 ` Torsten Duwe 2018-08-29 11:37 ` Miroslav Benes 2018-08-10 16:03 ` [PATCH 3/3] arm64: reliable stacktraces Torsten Duwe 2 siblings, 1 reply; 12+ messages in thread From: Torsten Duwe @ 2018-08-10 16:03 UTC (permalink / raw) To: Will Deacon, Catalin Marinas, Julien Thierry, Steven Rostedt, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro Cc: linux-arm-kernel, linux-kernel, live-patching Based on ftrace with regs, do the usual thing. Also allocate a task flag for whatever consistency handling is implemented. Watch out for interactions with the graph tracer. This code has been compile-tested, but has not yet seen any heavy livepatching. Signed-off-by: Torsten Duwe <duwe@suse.de> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 28c8c03..31df287 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -117,6 +117,7 @@ config ARM64 select HAVE_GENERIC_DMA_COHERENT select HAVE_HW_BREAKPOINT if PERF_EVENTS select HAVE_IRQ_TIME_ACCOUNTING + select HAVE_LIVEPATCH select HAVE_MEMBLOCK select HAVE_MEMBLOCK_NODE_MAP if NUMA select HAVE_NMI @@ -1343,4 +1344,6 @@ if CRYPTO source "arch/arm64/crypto/Kconfig" endif +source "kernel/livepatch/Kconfig" + source "lib/Kconfig" --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -76,6 +76,7 @@ void arch_release_task_struct(struct tas #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ +#define TIF_PATCH_PENDING 6 #define TIF_NOHZ 7 #define TIF_SYSCALL_TRACE 8 #define TIF_SYSCALL_AUDIT 9 @@ -94,6 +95,7 @@ void arch_release_task_struct(struct tas #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) #define _TIF_NOTIFY_RESUME (1 << TIF_NOTIFY_RESUME) #define _TIF_FOREIGN_FPSTATE (1 << TIF_FOREIGN_FPSTATE) +#define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING) #define _TIF_NOHZ (1 << TIF_NOHZ) #define _TIF_SYSCALL_TRACE (1 << TIF_SYSCALL_TRACE) #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT) @@ -106,7 +108,8 @@ void arch_release_task_struct(struct tas #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \ - _TIF_UPROBE | _TIF_FSCHECK) + _TIF_UPROBE | _TIF_FSCHECK | \ + _TIF_PATCH_PENDING) #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \ diff --git a/arch/arm64/include/asm/livepatch.h b/arch/arm64/include/asm/livepatch.h new file mode 100644 index 0000000..6b9a3d1 --- /dev/null +++ b/arch/arm64/include/asm/livepatch.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * livepatch.h - arm64-specific Kernel Live Patching Core + * + * Copyright (C) 2016 SUSE + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ +#ifndef _ASM_ARM64_LIVEPATCH_H +#define _ASM_ARM64_LIVEPATCH_H + +#include <linux/module.h> +#include <linux/ftrace.h> + +#ifdef CONFIG_LIVEPATCH +static inline int klp_check_compiler_support(void) +{ + return 0; +} + +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) +{ + regs->pc = ip; +} +#endif /* CONFIG_LIVEPATCH */ + +#endif /* _ASM_ARM64_LIVEPATCH_H */ diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S index cdb5866..cf640e8 100644 --- a/arch/arm64/kernel/entry-ftrace.S +++ b/arch/arm64/kernel/entry-ftrace.S @@ -195,6 +195,9 @@ ENTRY(ftrace_caller) str x9, [sp, #S_LR] /* The program counter just after the ftrace call site */ str lr, [sp, #S_PC] +#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_FUNCTION_GRAPH_TRACER) + mov x19,lr /* remember old return address */ +#endif /* The stack pointer as it was on ftrace_caller entry... */ add x29, sp, #S_FRAME_SIZE+16 /* ...is also our new FP */ str x29, [sp, #S_SP] @@ -210,6 +213,16 @@ ftrace_call: bl ftrace_stub +#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_FUNCTION_GRAPH_TRACER) + /* Is the trace function a live patcher an has messed with + * the return address? + */ + ldr x9, [sp, #S_PC] + cmp x9, x19 /* compare with the value we remembered */ + /* to not call graph tracer's "call" mechanism twice! */ + b.ne ftrace_regs_return +#endif + #ifdef CONFIG_FUNCTION_GRAPH_TRACER .global ftrace_graph_call ftrace_graph_call: // ftrace_graph_caller(); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] arm64: implement live patching 2018-08-10 16:03 ` [PATCH 2/3] arm64: implement live patching Torsten Duwe @ 2018-08-29 11:37 ` Miroslav Benes 0 siblings, 0 replies; 12+ messages in thread From: Miroslav Benes @ 2018-08-29 11:37 UTC (permalink / raw) To: Torsten Duwe Cc: Will Deacon, Catalin Marinas, Julien Thierry, Steven Rostedt, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro, linux-arm-kernel, linux-kernel, live-patching On Fri, 10 Aug 2018, Torsten Duwe wrote: > Based on ftrace with regs, do the usual thing. Also allocate a > task flag for whatever consistency handling is implemented. > Watch out for interactions with the graph tracer. > This code has been compile-tested, but has not yet seen any > heavy livepatching. > > Signed-off-by: Torsten Duwe <duwe@suse.de> There is one thing missing. Livepatch is able to automatically resolve relocations pointing to SHN_LIVEPATCH symbols. Arch-specific apply_relocate_add() is called for the purpose. It needs all arch-specific info preserved because of that. Usually it means to keep at least mod_arch_specific struct also after a module is loaded. It depends on its content. See f31e0960f395 ("module: s390: keep mod_arch_specific for livepatch modules") for example. We discussed the issue in 2016 when you submitted the arm64 support originally. It was more or less ok back then. Jessica only proposed to update count_plts(). However, a lot has changed since then and the code must be inspected again. If everything is ok, there should be at least a note in the changelog. > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 28c8c03..31df287 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -117,6 +117,7 @@ config ARM64 > select HAVE_GENERIC_DMA_COHERENT > select HAVE_HW_BREAKPOINT if PERF_EVENTS > select HAVE_IRQ_TIME_ACCOUNTING > + select HAVE_LIVEPATCH > select HAVE_MEMBLOCK > select HAVE_MEMBLOCK_NODE_MAP if NUMA > select HAVE_NMI > @@ -1343,4 +1344,6 @@ if CRYPTO > source "arch/arm64/crypto/Kconfig" > endif > > +source "kernel/livepatch/Kconfig" > + > source "lib/Kconfig" > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -76,6 +76,7 @@ void arch_release_task_struct(struct tas > #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ > #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ > +#define TIF_PATCH_PENDING 6 > #define TIF_NOHZ 7 > #define TIF_SYSCALL_TRACE 8 > #define TIF_SYSCALL_AUDIT 9 > @@ -94,6 +95,7 @@ void arch_release_task_struct(struct tas > #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) > #define _TIF_NOTIFY_RESUME (1 << TIF_NOTIFY_RESUME) > #define _TIF_FOREIGN_FPSTATE (1 << TIF_FOREIGN_FPSTATE) > +#define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING) > #define _TIF_NOHZ (1 << TIF_NOHZ) > #define _TIF_SYSCALL_TRACE (1 << TIF_SYSCALL_TRACE) > #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT) > @@ -106,7 +108,8 @@ void arch_release_task_struct(struct tas > > #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ > _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \ > - _TIF_UPROBE | _TIF_FSCHECK) > + _TIF_UPROBE | _TIF_FSCHECK | \ > + _TIF_PATCH_PENDING) We're ok if _TIF_WORK_MASK is processed in the syscall return path and in irq return to user space. It looks like it is the case, but I'd welcome a confirmation. Anyway, one piece is still missing. _TIF_PATCH_PENDING must be cleared somewhere. I think something like if (thread_flags & _TIF_PATCH_PENDING) klp_update_patch_state(current); in do_notify_resume() (arch/arm64/kernel/signal.c) before _TIF_SIGPENDING processing should do the trick. > #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ > _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \ > diff --git a/arch/arm64/include/asm/livepatch.h b/arch/arm64/include/asm/livepatch.h > new file mode 100644 > index 0000000..6b9a3d1 > --- /dev/null > +++ b/arch/arm64/include/asm/livepatch.h > @@ -0,0 +1,38 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * livepatch.h - arm64-specific Kernel Live Patching Core > + * > + * Copyright (C) 2016 SUSE > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > + */ Someone could argue that GPL boilerplate is superfluous thanks to SPDX identifier. I don't, so thanks for putting it there. > +#ifndef _ASM_ARM64_LIVEPATCH_H > +#define _ASM_ARM64_LIVEPATCH_H > + > +#include <linux/module.h> > +#include <linux/ftrace.h> > + > +#ifdef CONFIG_LIVEPATCH The guard is unnecessary, I think. The header file is included from include/linux/livepatch.h only. Right after the guard there. > +static inline int klp_check_compiler_support(void) > +{ > + return 0; > +} > + > +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) > +{ > + regs->pc = ip; > +} > +#endif /* CONFIG_LIVEPATCH */ > + > +#endif /* _ASM_ARM64_LIVEPATCH_H */ Regards, Miroslav ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] arm64: reliable stacktraces 2018-08-10 16:00 [PATCH 0/3] arm64 live patching Torsten Duwe 2018-08-10 16:02 ` [PATCH 1/3] arm64: implement ftrace with regs Torsten Duwe 2018-08-10 16:03 ` [PATCH 2/3] arm64: implement live patching Torsten Duwe @ 2018-08-10 16:03 ` Torsten Duwe 2018-08-10 20:44 ` Josh Poimboeuf 2 siblings, 1 reply; 12+ messages in thread From: Torsten Duwe @ 2018-08-10 16:03 UTC (permalink / raw) To: Will Deacon, Catalin Marinas, Julien Thierry, Steven Rostedt, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro Cc: linux-arm-kernel, linux-kernel, live-patching This is more an RFC in the original sense: is this basically the correct approach? (as I had to tweak the API a bit). In particular the code does not detect interrupts and exception frames, and does not yet check whether the code address is valid. The latter check would also have to be omitted for the latest frame on other tasks' stacks. This would require some more tweaking. unwind_frame() now reports whether we had to stop normally or due to an error condition; walk_stackframe() will pass that info. __save_stack_trace() is used for a start to check the validity of a frame; maybe save_stack_trace_tsk_reliable() will need its own callback. Any comments welcome. Signed-off-by: Torsten Duwe <duwe@suse.de> --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -127,8 +127,9 @@ config ARM64 select HAVE_PERF_EVENTS select HAVE_PERF_REGS select HAVE_PERF_USER_STACK_DUMP - select HAVE_REGS_AND_STACK_ACCESS_API select HAVE_RCU_TABLE_FREE + select HAVE_REGS_AND_STACK_ACCESS_API + select HAVE_RELIABLE_STACKTRACE select HAVE_STACKPROTECTOR select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES --- a/arch/arm64/include/asm/stacktrace.h +++ b/arch/arm64/include/asm/stacktrace.h @@ -33,7 +33,7 @@ struct stackframe { }; extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame); -extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame, +extern int walk_stackframe(struct task_struct *tsk, struct stackframe *frame, int (*fn)(struct stackframe *, void *), void *data); extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk); diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index d5718a060672..fe0dd4745ff3 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -81,23 +81,27 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) * both are NULL. */ if (!frame->fp && !frame->pc) - return -EINVAL; + return 1; return 0; } -void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame, +int notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame, int (*fn)(struct stackframe *, void *), void *data) { while (1) { int ret; - if (fn(frame, data)) - break; + ret = fn(frame, data); + if (ret) + return ret; ret = unwind_frame(tsk, frame); if (ret < 0) + return ret; + if (ret > 0) break; } + return 0; } #ifdef CONFIG_STACKTRACE @@ -145,14 +149,15 @@ void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace) trace->entries[trace->nr_entries++] = ULONG_MAX; } -static noinline void __save_stack_trace(struct task_struct *tsk, +static noinline int __save_stack_trace(struct task_struct *tsk, struct stack_trace *trace, unsigned int nosched) { struct stack_trace_data data; struct stackframe frame; + int ret; if (!try_get_task_stack(tsk)) - return; + return -EBUSY; data.trace = trace; data.skip = trace->skip; @@ -171,11 +176,12 @@ static noinline void __save_stack_trace(struct task_struct *tsk, frame.graph = tsk->curr_ret_stack; #endif - walk_stackframe(tsk, &frame, save_trace, &data); + ret = walk_stackframe(tsk, &frame, save_trace, &data); if (trace->nr_entries < trace->max_entries) trace->entries[trace->nr_entries++] = ULONG_MAX; put_task_stack(tsk); + return ret; } EXPORT_SYMBOL_GPL(save_stack_trace_tsk); @@ -190,4 +196,12 @@ void save_stack_trace(struct stack_trace *trace) } EXPORT_SYMBOL_GPL(save_stack_trace); + +int save_stack_trace_tsk_reliable(struct task_struct *tsk, + struct stack_trace *trace) +{ + return __save_stack_trace(tsk, trace, 1); +} +EXPORT_SYMBOL_GPL(save_stack_trace_tsk_reliable); + #endif ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] arm64: reliable stacktraces 2018-08-10 16:03 ` [PATCH 3/3] arm64: reliable stacktraces Torsten Duwe @ 2018-08-10 20:44 ` Josh Poimboeuf 0 siblings, 0 replies; 12+ messages in thread From: Josh Poimboeuf @ 2018-08-10 20:44 UTC (permalink / raw) To: Torsten Duwe Cc: Will Deacon, Catalin Marinas, Julien Thierry, Steven Rostedt, Ingo Molnar, Ard Biesheuvel, Arnd Bergmann, AKASHI Takahiro, linux-arm-kernel, linux-kernel, live-patching On Fri, Aug 10, 2018 at 06:03:11PM +0200, Torsten Duwe wrote: > This is more an RFC in the original sense: is this basically > the correct approach? (as I had to tweak the API a bit). > > In particular the code does not detect interrupts and exception > frames, and does not yet check whether the code address is valid. > The latter check would also have to be omitted for the latest frame > on other tasks' stacks. This would require some more tweaking. > > unwind_frame() now reports whether we had to stop normally or due to > an error condition; walk_stackframe() will pass that info. > __save_stack_trace() is used for a start to check the validity of a > frame; maybe save_stack_trace_tsk_reliable() will need its own callback. > > Any comments welcome. > > Signed-off-by: Torsten Duwe <duwe@suse.de> Before we do this we'll need the same analysis we did for ppc64le to figure out if objtool is needed. -- Josh ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-08-29 11:37 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-10 16:00 [PATCH 0/3] arm64 live patching Torsten Duwe 2018-08-10 16:02 ` [PATCH 1/3] arm64: implement ftrace with regs Torsten Duwe 2018-08-10 19:27 ` Steven Rostedt 2018-08-13 10:54 ` Julien Thierry 2018-08-14 2:03 ` Steven Rostedt 2018-08-14 8:33 ` Julien Thierry 2018-08-14 16:04 ` Steven Rostedt 2018-08-15 13:47 ` Torsten Duwe 2018-08-10 16:03 ` [PATCH 2/3] arm64: implement live patching Torsten Duwe 2018-08-29 11:37 ` Miroslav Benes 2018-08-10 16:03 ` [PATCH 3/3] arm64: reliable stacktraces Torsten Duwe 2018-08-10 20:44 ` Josh Poimboeuf
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).