* [PATCH RFC 21/43] x86/ftrace: Adapt assembly for PIE support [not found] <cover.1682673542.git.houwenlong.hwl@antgroup.com> @ 2023-04-28 9:51 ` Hou Wenlong 2023-04-28 13:37 ` Steven Rostedt 2023-04-28 9:51 ` [PATCH RFC 22/43] x86/ftrace: Adapt ftrace nop patching " Hou Wenlong 1 sibling, 1 reply; 6+ messages in thread From: Hou Wenlong @ 2023-04-28 9:51 UTC (permalink / raw) To: linux-kernel Cc: Thomas Garnier, Lai Jiangshan, Kees Cook, Hou Wenlong, Steven Rostedt, Masami Hiramatsu, Mark Rutland, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-trace-kernel Change the assembly code to use only relative references of symbols for the kernel to be PIE compatible. Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com> Cc: Thomas Garnier <thgarnie@chromium.org> Cc: Lai Jiangshan <jiangshan.ljs@antgroup.com> Cc: Kees Cook <keescook@chromium.org> --- arch/x86/kernel/ftrace_64.S | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index eddb4fabc16f..411fa4148e18 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -315,7 +315,14 @@ STACK_FRAME_NON_STANDARD_FP(ftrace_regs_caller) SYM_FUNC_START(__fentry__) CALL_DEPTH_ACCOUNT +#ifdef CONFIG_X86_PIE + pushq %r8 + leaq ftrace_stub(%rip), %r8 + cmpq %r8, ftrace_trace_function(%rip) + popq %r8 +#else cmpq $ftrace_stub, ftrace_trace_function +#endif jnz trace RET @@ -329,7 +336,7 @@ trace: * ip and parent ip are used and the list function is called when * function tracing is enabled. */ - movq ftrace_trace_function, %r8 + movq ftrace_trace_function(%rip), %r8 CALL_NOSPEC r8 restore_mcount_regs -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 21/43] x86/ftrace: Adapt assembly for PIE support 2023-04-28 9:51 ` [PATCH RFC 21/43] x86/ftrace: Adapt assembly for PIE support Hou Wenlong @ 2023-04-28 13:37 ` Steven Rostedt 2023-04-29 3:43 ` Hou Wenlong 0 siblings, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2023-04-28 13:37 UTC (permalink / raw) To: Hou Wenlong Cc: linux-kernel, Thomas Garnier, Lai Jiangshan, Kees Cook, Masami Hiramatsu, Mark Rutland, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-trace-kernel On Fri, 28 Apr 2023 17:51:01 +0800 "Hou Wenlong" <houwenlong.hwl@antgroup.com> wrote: > Change the assembly code to use only relative references of symbols for > the kernel to be PIE compatible. > > Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com> > Cc: Thomas Garnier <thgarnie@chromium.org> > Cc: Lai Jiangshan <jiangshan.ljs@antgroup.com> > Cc: Kees Cook <keescook@chromium.org> > --- > arch/x86/kernel/ftrace_64.S | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S > index eddb4fabc16f..411fa4148e18 100644 > --- a/arch/x86/kernel/ftrace_64.S > +++ b/arch/x86/kernel/ftrace_64.S > @@ -315,7 +315,14 @@ STACK_FRAME_NON_STANDARD_FP(ftrace_regs_caller) > SYM_FUNC_START(__fentry__) > CALL_DEPTH_ACCOUNT > > +#ifdef CONFIG_X86_PIE > + pushq %r8 > + leaq ftrace_stub(%rip), %r8 > + cmpq %r8, ftrace_trace_function(%rip) > + popq %r8 > +#else > cmpq $ftrace_stub, ftrace_trace_function > +#endif > jnz trace > RET > > @@ -329,7 +336,7 @@ trace: > * ip and parent ip are used and the list function is called when > * function tracing is enabled. > */ > - movq ftrace_trace_function, %r8 > + movq ftrace_trace_function(%rip), %r8 > CALL_NOSPEC r8 > restore_mcount_regs > I really don't want to add more updates to !DYNAMIC_FTRACE. This code only exists to make sure I don't break it for other architectures. How about diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 442eccc00960..ee4d0713139d 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -37,7 +37,7 @@ config X86_64 config FORCE_DYNAMIC_FTRACE def_bool y - depends on X86_32 + depends on X86_32 || X86_PIE depends on FUNCTION_TRACER select DYNAMIC_FTRACE help ? -- Steve ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 21/43] x86/ftrace: Adapt assembly for PIE support 2023-04-28 13:37 ` Steven Rostedt @ 2023-04-29 3:43 ` Hou Wenlong 0 siblings, 0 replies; 6+ messages in thread From: Hou Wenlong @ 2023-04-29 3:43 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Thomas Garnier, Lai Jiangshan, Kees Cook, Masami Hiramatsu, Mark Rutland, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-trace-kernel On Fri, Apr 28, 2023 at 09:37:19PM +0800, Steven Rostedt wrote: > On Fri, 28 Apr 2023 17:51:01 +0800 > "Hou Wenlong" <houwenlong.hwl@antgroup.com> wrote: > > > Change the assembly code to use only relative references of symbols for > > the kernel to be PIE compatible. > > > > Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com> > > Cc: Thomas Garnier <thgarnie@chromium.org> > > Cc: Lai Jiangshan <jiangshan.ljs@antgroup.com> > > Cc: Kees Cook <keescook@chromium.org> > > --- > > arch/x86/kernel/ftrace_64.S | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S > > index eddb4fabc16f..411fa4148e18 100644 > > --- a/arch/x86/kernel/ftrace_64.S > > +++ b/arch/x86/kernel/ftrace_64.S > > @@ -315,7 +315,14 @@ STACK_FRAME_NON_STANDARD_FP(ftrace_regs_caller) > > SYM_FUNC_START(__fentry__) > > CALL_DEPTH_ACCOUNT > > > > +#ifdef CONFIG_X86_PIE > > + pushq %r8 > > + leaq ftrace_stub(%rip), %r8 > > + cmpq %r8, ftrace_trace_function(%rip) > > + popq %r8 > > +#else > > cmpq $ftrace_stub, ftrace_trace_function > > +#endif > > jnz trace > > RET > > > > @@ -329,7 +336,7 @@ trace: > > * ip and parent ip are used and the list function is called when > > * function tracing is enabled. > > */ > > - movq ftrace_trace_function, %r8 > > + movq ftrace_trace_function(%rip), %r8 > > CALL_NOSPEC r8 > > restore_mcount_regs > > > > I really don't want to add more updates to !DYNAMIC_FTRACE. This code only > exists to make sure I don't break it for other architectures. > > How about > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 442eccc00960..ee4d0713139d 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -37,7 +37,7 @@ config X86_64 > > config FORCE_DYNAMIC_FTRACE > def_bool y > - depends on X86_32 > + depends on X86_32 || X86_PIE > depends on FUNCTION_TRACER > select DYNAMIC_FTRACE > help > > > ? > OK, I'll drop it. Actually, I select DYNAMIC_FTRACE when CONFIG_RETPOLINE is enabled for PIE due to the indirect call for __fentry__() in patch 34. Thanks. > -- Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH RFC 22/43] x86/ftrace: Adapt ftrace nop patching for PIE support [not found] <cover.1682673542.git.houwenlong.hwl@antgroup.com> 2023-04-28 9:51 ` [PATCH RFC 21/43] x86/ftrace: Adapt assembly for PIE support Hou Wenlong @ 2023-04-28 9:51 ` Hou Wenlong 2023-04-28 13:44 ` Steven Rostedt 1 sibling, 1 reply; 6+ messages in thread From: Hou Wenlong @ 2023-04-28 9:51 UTC (permalink / raw) To: linux-kernel Cc: Thomas Garnier, Lai Jiangshan, Kees Cook, Hou Wenlong, Steven Rostedt, Masami Hiramatsu, Mark Rutland, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Huacai Chen, Qing Zhang, linux-trace-kernel From: Thomas Garnier <thgarnie@chromium.org> From: Thomas Garnier <thgarnie@chromium.org> When using PIE with function tracing, the compiler generates a call through the GOT (call *__fentry__@GOTPCREL). This instruction takes 6-bytes instead of 5-bytes with a relative call. And -mnop-mcount option is not implemented for -fPIE now. If PIE is enabled, replace the 6th byte of the GOT call by a 1-byte nop so ftrace can handle the previous 5-bytes as before. [Hou Wenlong: Adapt code change and fix wrong offset calculation in make_nop_x86()] Signed-off-by: Thomas Garnier <thgarnie@chromium.org> Co-developed-by: Hou Wenlong <houwenlong.hwl@antgroup.com> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com> Cc: Lai Jiangshan <jiangshan.ljs@antgroup.com> Cc: Kees Cook <keescook@chromium.org> --- arch/x86/kernel/ftrace.c | 46 ++++++++++++++++++++++- scripts/recordmcount.c | 81 ++++++++++++++++++++++++++-------------- 2 files changed, 98 insertions(+), 29 deletions(-) diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index 5e7ead52cfdb..b795f9dde561 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -124,6 +124,50 @@ ftrace_modify_code_direct(unsigned long ip, const char *old_code, return 0; } +/* Bytes before call GOT offset */ +static const unsigned char got_call_preinsn[] = { 0xff, 0x15 }; + +static int __ref +ftrace_modify_initial_code(unsigned long ip, unsigned const char *old_code, + unsigned const char *new_code) +{ + unsigned char replaced[MCOUNT_INSN_SIZE + 1]; + + /* + * If PIE is not enabled default to the original approach to code + * modification. + */ + if (!IS_ENABLED(CONFIG_X86_PIE)) + return ftrace_modify_code_direct(ip, old_code, new_code); + + ftrace_expected = old_code; + + /* Ensure the instructions point to a call to the GOT */ + if (copy_from_kernel_nofault(replaced, (void *)ip, sizeof(replaced))) { + WARN_ONCE(1, "invalid function"); + return -EFAULT; + } + + if (memcmp(replaced, got_call_preinsn, sizeof(got_call_preinsn))) { + WARN_ONCE(1, "invalid function call"); + return -EINVAL; + } + + /* + * Build a nop slide with a 5-byte nop and 1-byte nop to keep the ftrace + * hooking algorithm working with the expected 5 bytes instruction. + */ + memset(replaced, x86_nops[1][0], sizeof(replaced)); + memcpy(replaced, new_code, MCOUNT_INSN_SIZE); + + /* replace the text with the new text */ + if (ftrace_poke_late) + text_poke_queue((void *)ip, replaced, MCOUNT_INSN_SIZE + 1, NULL); + else + text_poke_early((void *)ip, replaced, MCOUNT_INSN_SIZE + 1); + return 0; +} + int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long addr) { unsigned long ip = rec->ip; @@ -141,7 +185,7 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long ad * just modify the code directly. */ if (addr == MCOUNT_ADDR) - return ftrace_modify_code_direct(ip, old, new); + return ftrace_modify_initial_code(ip, old, new); /* * x86 overrides ftrace_replace_code -- this function will never be used diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c index e30216525325..02783a29d428 100644 --- a/scripts/recordmcount.c +++ b/scripts/recordmcount.c @@ -218,36 +218,10 @@ static void *mmap_file(char const *fname) return file_map; } - -static unsigned char ideal_nop5_x86_64[5] = { 0x0f, 0x1f, 0x44, 0x00, 0x00 }; -static unsigned char ideal_nop5_x86_32[5] = { 0x3e, 0x8d, 0x74, 0x26, 0x00 }; -static unsigned char *ideal_nop; - static char rel_type_nop; - static int (*make_nop)(void *map, size_t const offset); -static int make_nop_x86(void *map, size_t const offset) -{ - uint32_t *ptr; - unsigned char *op; - - /* Confirm we have 0xe8 0x0 0x0 0x0 0x0 */ - ptr = map + offset; - if (*ptr != 0) - return -1; - - op = map + offset - 1; - if (*op != 0xe8) - return -1; - - /* convert to nop */ - if (ulseek(offset - 1, SEEK_SET) < 0) - return -1; - if (uwrite(ideal_nop, 5) < 0) - return -1; - return 0; -} +static unsigned char *ideal_nop; static unsigned char ideal_nop4_arm_le[4] = { 0x00, 0x00, 0xa0, 0xe1 }; /* mov r0, r0 */ static unsigned char ideal_nop4_arm_be[4] = { 0xe1, 0xa0, 0x00, 0x00 }; /* mov r0, r0 */ @@ -504,6 +478,50 @@ static void MIPS64_r_info(Elf64_Rel *const rp, unsigned sym, unsigned type) }).r_info; } +static unsigned char ideal_nop5_x86_64[5] = { 0x0f, 0x1f, 0x44, 0x00, 0x00 }; +static unsigned char ideal_nop6_x86_64[6] = { 0x66, 0x0f, 0x1f, 0x44, 0x00, 0x00 }; +static unsigned char ideal_nop5_x86_32[5] = { 0x3e, 0x8d, 0x74, 0x26, 0x00 }; +static size_t ideal_nop_x86_size; + +static unsigned char stub_default_x86[2] = { 0xe8, 0x00 }; /* call relative */ +static unsigned char stub_got_x86[3] = { 0xff, 0x15, 0x00 }; /* call .got */ +static unsigned char *stub_x86; +static size_t stub_x86_size; + +static int make_nop_x86(void *map, size_t const offset) +{ + uint32_t *ptr; + size_t stub_offset = offset + 1 - stub_x86_size; + + /* Confirm we have the expected stub */ + ptr = map + stub_offset; + if (memcmp(ptr, stub_x86, stub_x86_size)) + return -1; + + /* convert to nop */ + if (ulseek(stub_offset, SEEK_SET) < 0) + return -1; + if (uwrite(ideal_nop, ideal_nop_x86_size) < 0) + return -1; + return 0; +} + +/* Swap the stub and nop for a got call if the binary is built with PIE */ +static int is_fake_mcount_x86_x64(Elf64_Rel const *rp) +{ + if (ELF64_R_TYPE(rp->r_info) == R_X86_64_GOTPCREL) { + ideal_nop = ideal_nop6_x86_64; + ideal_nop_x86_size = sizeof(ideal_nop6_x86_64); + stub_x86 = stub_got_x86; + stub_x86_size = sizeof(stub_got_x86); + mcount_adjust_64 = 1 - stub_x86_size; + } + + /* Once the relocation was checked, rollback to default */ + is_fake_mcount64 = fn_is_fake_mcount64; + return is_fake_mcount64(rp); +} + static int do_file(char const *const fname) { unsigned int reltype = 0; @@ -568,6 +586,9 @@ static int do_file(char const *const fname) rel_type_nop = R_386_NONE; make_nop = make_nop_x86; ideal_nop = ideal_nop5_x86_32; + ideal_nop_x86_size = sizeof(ideal_nop5_x86_32); + stub_x86 = stub_default_x86; + stub_x86_size = sizeof(stub_default_x86); mcount_adjust_32 = -1; gpfx = 0; break; @@ -597,9 +618,13 @@ static int do_file(char const *const fname) case EM_X86_64: make_nop = make_nop_x86; ideal_nop = ideal_nop5_x86_64; + ideal_nop_x86_size = sizeof(ideal_nop5_x86_64); + stub_x86 = stub_default_x86; + stub_x86_size = sizeof(stub_default_x86); reltype = R_X86_64_64; rel_type_nop = R_X86_64_NONE; - mcount_adjust_64 = -1; + is_fake_mcount64 = is_fake_mcount_x86_x64; + mcount_adjust_64 = 1 - stub_x86_size; gpfx = 0; break; } /* end switch */ -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 22/43] x86/ftrace: Adapt ftrace nop patching for PIE support 2023-04-28 9:51 ` [PATCH RFC 22/43] x86/ftrace: Adapt ftrace nop patching " Hou Wenlong @ 2023-04-28 13:44 ` Steven Rostedt 2023-04-29 3:38 ` Hou Wenlong 0 siblings, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2023-04-28 13:44 UTC (permalink / raw) To: Hou Wenlong Cc: linux-kernel, Thomas Garnier, Lai Jiangshan, Kees Cook, Masami Hiramatsu, Mark Rutland, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Huacai Chen, Qing Zhang, linux-trace-kernel On Fri, 28 Apr 2023 17:51:02 +0800 "Hou Wenlong" <houwenlong.hwl@antgroup.com> wrote: > From: Thomas Garnier <thgarnie@chromium.org> > > From: Thomas Garnier <thgarnie@chromium.org> > > When using PIE with function tracing, the compiler generates a > call through the GOT (call *__fentry__@GOTPCREL). This instruction > takes 6-bytes instead of 5-bytes with a relative call. And -mnop-mcount > option is not implemented for -fPIE now. > > If PIE is enabled, replace the 6th byte of the GOT call by a 1-byte nop > so ftrace can handle the previous 5-bytes as before. Wait! This won't work! You can't just append another nop to fill in the blanks here. We must either have a single 6 byte nop, or we need to refactor the entire logic to something that other archs have. The two nops means that the CPU can take it as two separate commands. There's nothing stopping the computer from preempting a task between the two. If that happens, and you modify the 1byte nop and 5byte nop with a single 6 byte command, when the task get's rescheduled, it will execute the last 5 bytes of that 6 byte command and take a general protection fault, and likely crash the machine. NACK on this. It needs a better solution. -- Steve > > [Hou Wenlong: Adapt code change and fix wrong offset calculation in > make_nop_x86()] > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 22/43] x86/ftrace: Adapt ftrace nop patching for PIE support 2023-04-28 13:44 ` Steven Rostedt @ 2023-04-29 3:38 ` Hou Wenlong 0 siblings, 0 replies; 6+ messages in thread From: Hou Wenlong @ 2023-04-29 3:38 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Thomas Garnier, Lai Jiangshan, Kees Cook, Masami Hiramatsu, Mark Rutland, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Huacai Chen, Qing Zhang, linux-trace-kernel On Fri, Apr 28, 2023 at 09:44:54PM +0800, Steven Rostedt wrote: > On Fri, 28 Apr 2023 17:51:02 +0800 > "Hou Wenlong" <houwenlong.hwl@antgroup.com> wrote: > > > From: Thomas Garnier <thgarnie@chromium.org> > > > > From: Thomas Garnier <thgarnie@chromium.org> > > > > When using PIE with function tracing, the compiler generates a > > call through the GOT (call *__fentry__@GOTPCREL). This instruction > > takes 6-bytes instead of 5-bytes with a relative call. And -mnop-mcount > > option is not implemented for -fPIE now. > > > > If PIE is enabled, replace the 6th byte of the GOT call by a 1-byte nop > > so ftrace can handle the previous 5-bytes as before. > > Wait! This won't work! > > You can't just append another nop to fill in the blanks here. We must > either have a single 6 byte nop, or we need to refactor the entire logic to > something that other archs have. > > The two nops means that the CPU can take it as two separate commands. > There's nothing stopping the computer from preempting a task between the > two. If that happens, and you modify the 1byte nop and 5byte nop with a > single 6 byte command, when the task get's rescheduled, it will execute the > last 5 bytes of that 6 byte command and take a general protection fault, and > likely crash the machine. > > NACK on this. It needs a better solution. > > -- Steve > > Hi Steve, Sorry for not providing the original patch link: https://lore.kernel.org/all/20190131192533.34130-22-thgarnie@chromium.org/ I drop the Reviewed-by tag due to the change described in commit message. This nop patching is only used for the first time (addr = MCOUNT) before SMP or executing code in module. And ftrace_make_call() is not modified, then we would use 5 byte direct call to replace the first 5 byte nop when tracepoint is enabled like before, it's still one instruction. So, the logic is same like before, patch the first 5 byte when tracepoint is enabled or disabled during running. > > > > [Hou Wenlong: Adapt code change and fix wrong offset calculation in > > make_nop_x86()] > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-29 3:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1682673542.git.houwenlong.hwl@antgroup.com>
2023-04-28 9:51 ` [PATCH RFC 21/43] x86/ftrace: Adapt assembly for PIE support Hou Wenlong
2023-04-28 13:37 ` Steven Rostedt
2023-04-29 3:43 ` Hou Wenlong
2023-04-28 9:51 ` [PATCH RFC 22/43] x86/ftrace: Adapt ftrace nop patching " Hou Wenlong
2023-04-28 13:44 ` Steven Rostedt
2023-04-29 3:38 ` Hou Wenlong
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).