| Message ID | 20200625200235.GQ4781@hirez.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] objtool,x86_64: Replace recordmcount with objtool | expand |
On Thu, Jun 25, 2020 at 1:02 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Jun 25, 2020 at 09:15:03AM -0700, Sami Tolvanen wrote: > > On Thu, Jun 25, 2020 at 09:45:30AM +0200, Peter Zijlstra wrote: > > > > At least for x86_64 I can do a really quick take for a recordmcount pass > > > in objtool, but I suppose you also need this for ARM64 ? > > > > Sure, sounds good. arm64 uses -fpatchable-function-entry with clang, so we > > don't need recordmcount there. > > This is on top of my local pile: > > git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git master > > which notably includes the static_call series. > > Not boot tested, but it generates the required sections and they look > more or less as expected, ymmv. > > --- > arch/x86/Kconfig | 1 - > scripts/Makefile.build | 3 ++ > scripts/link-vmlinux.sh | 2 +- > tools/objtool/builtin-check.c | 9 ++--- > tools/objtool/builtin.h | 2 +- > tools/objtool/check.c | 81 +++++++++++++++++++++++++++++++++++++++++++ > tools/objtool/check.h | 1 + > tools/objtool/objtool.h | 1 + > 8 files changed, 91 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index a291823f3f26..189575c12434 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -174,7 +174,6 @@ config X86 > select HAVE_EXIT_THREAD > select HAVE_FAST_GUP > select HAVE_FENTRY if X86_64 || DYNAMIC_FTRACE > - select HAVE_FTRACE_MCOUNT_RECORD > select HAVE_FUNCTION_GRAPH_TRACER > select HAVE_FUNCTION_TRACER > select HAVE_GCC_PLUGINS > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 2e8810b7e5ed..c774befc57da 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -227,6 +227,9 @@ endif > ifdef CONFIG_X86_SMAP > objtool_args += --uaccess > endif > +ifdef CONFIG_DYNAMIC_FTRACE > + objtool_args += --mcount > +endif > > # 'OBJECT_FILES_NON_STANDARD := y': skip objtool checking for a directory > # 'OBJECT_FILES_NON_STANDARD_foo.o := 'y': skip objtool checking for a file > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh > index 92dd745906f4..00c6e4f28a1a 100755 > --- a/scripts/link-vmlinux.sh > +++ b/scripts/link-vmlinux.sh > @@ -60,7 +60,7 @@ objtool_link() > local objtoolopt; > > if [ -n "${CONFIG_VMLINUX_VALIDATION}" ]; then > - objtoolopt="check" > + objtoolopt="check --vmlinux" > if [ -z "${CONFIG_FRAME_POINTER}" ]; then > objtoolopt="${objtoolopt} --no-fp" > fi > diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c > index 4896c5a89702..a6c3a3fba67d 100644 > --- a/tools/objtool/builtin-check.c > +++ b/tools/objtool/builtin-check.c > @@ -18,7 +18,7 @@ > #include "builtin.h" > #include "objtool.h" > > -bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, fpu; > +bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, fpu, mcount; > > static const char * const check_usage[] = { > "objtool check [<options>] file.o", > @@ -36,12 +36,13 @@ const struct option check_options[] = { > OPT_BOOLEAN('d', "duplicate", &validate_dup, "duplicate validation for vmlinux.o"), > OPT_BOOLEAN('l', "vmlinux", &vmlinux, "vmlinux.o validation"), > OPT_BOOLEAN('F', "fpu", &fpu, "validate FPU context"), > + OPT_BOOLEAN('M', "mcount", &mcount, "generate __mcount_loc"), > OPT_END(), > }; > > int cmd_check(int argc, const char **argv) > { > - const char *objname, *s; > + const char *objname; > > argc = parse_options(argc, argv, check_options, check_usage, 0); > > @@ -50,9 +51,5 @@ int cmd_check(int argc, const char **argv) > > objname = argv[0]; > > - s = strstr(objname, "vmlinux.o"); > - if (s && !s[9]) > - vmlinux = true; > - > return check(objname, false); > } > diff --git a/tools/objtool/builtin.h b/tools/objtool/builtin.h > index 7158e09d4cc9..b51d883ec245 100644 > --- a/tools/objtool/builtin.h > +++ b/tools/objtool/builtin.h > @@ -8,7 +8,7 @@ > #include <subcmd/parse-options.h> > > extern const struct option check_options[]; > -extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, fpu; > +extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, fpu, mcount; > > extern int cmd_check(int argc, const char **argv); > extern int cmd_orc(int argc, const char **argv); > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 6647a8d1545b..ee99566bdae9 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -533,6 +533,65 @@ static int create_static_call_sections(struct objtool_file *file) > return 0; > } > > +static int create_mcount_loc_sections(struct objtool_file *file) > +{ > + struct section *sec, *reloc_sec; > + struct reloc *reloc; > + unsigned long *loc; > + struct instruction *insn; > + int idx; > + > + sec = find_section_by_name(file->elf, "__mcount_loc"); > + if (sec) { > + INIT_LIST_HEAD(&file->mcount_loc_list); > + WARN("file already has __mcount_loc section, skipping"); > + return 0; > + } > + > + if (list_empty(&file->mcount_loc_list)) > + return 0; > + > + idx = 0; > + list_for_each_entry(insn, &file->mcount_loc_list, mcount_loc_node) > + idx++; > + > + sec = elf_create_section(file->elf, "__mcount_loc", 0, sizeof(unsigned long), idx); > + if (!sec) > + return -1; > + > + reloc_sec = elf_create_reloc_section(file->elf, sec, SHT_RELA); > + if (!reloc_sec) > + return -1; > + > + idx = 0; > + list_for_each_entry(insn, &file->mcount_loc_list, mcount_loc_node) { > + > + loc = (unsigned long *)sec->data->d_buf + idx; > + memset(loc, 0, sizeof(unsigned long)); > + > + reloc = malloc(sizeof(*reloc)); > + if (!reloc) { > + perror("malloc"); > + return -1; > + } > + memset(reloc, 0, sizeof(*reloc)); calloc(1, sizeof(*reloc))? > + > + reloc->sym = insn->sec->sym; > + reloc->addend = insn->offset; > + reloc->type = R_X86_64_64; > + reloc->offset = idx * sizeof(unsigned long); > + reloc->sec = reloc_sec; > + elf_add_reloc(file->elf, reloc); > + > + idx++; > + } > + > + if (elf_rebuild_reloc_section(file->elf, reloc_sec)) > + return -1; > + > + return 0; > +} > + > /* > * Warnings shouldn't be reported for ignored functions. > */ > @@ -892,6 +951,22 @@ static int add_call_destinations(struct objtool_file *file) > insn->type = INSN_NOP; > } > > + if (mcount && !strcmp(insn->call_dest->name, "__fentry__")) { > + if (reloc) { > + reloc->type = R_NONE; > + elf_write_reloc(file->elf, reloc); > + } > + > + elf_write_insn(file->elf, insn->sec, > + insn->offset, insn->len, > + arch_nop_insn(insn->len)); > + > + insn->type = INSN_NOP; > + > + list_add_tail(&insn->mcount_loc_node, > + &file->mcount_loc_list); > + } > + > /* > * Whatever stack impact regular CALLs have, should be undone > * by the RETURN of the called function. > @@ -3004,6 +3079,7 @@ int check(const char *_objname, bool orc) > INIT_LIST_HEAD(&file.insn_list); > hash_init(file.insn_hash); > INIT_LIST_HEAD(&file.static_call_list); > + INIT_LIST_HEAD(&file.mcount_loc_list); > file.c_file = !vmlinux && find_section_by_name(file.elf, ".comment"); > file.ignore_unreachables = no_unreachable; > file.hints = false; > @@ -3056,6 +3132,11 @@ int check(const char *_objname, bool orc) > goto out; > warnings += ret; > > + ret = create_mcount_loc_sections(&file); > + if (ret < 0) > + goto out; > + warnings += ret; > + > if (orc) { > ret = create_orc(&file); > if (ret < 0) > diff --git a/tools/objtool/check.h b/tools/objtool/check.h > index cd95fca0d237..01f11b5da5dd 100644 > --- a/tools/objtool/check.h > +++ b/tools/objtool/check.h > @@ -24,6 +24,7 @@ struct instruction { > struct list_head list; > struct hlist_node hash; > struct list_head static_call_node; > + struct list_head mcount_loc_node; > struct section *sec; > unsigned long offset; > unsigned int len; > diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h > index 9a7cd0b88bd8..f604b22d22cc 100644 > --- a/tools/objtool/objtool.h > +++ b/tools/objtool/objtool.h > @@ -17,6 +17,7 @@ struct objtool_file { > struct list_head insn_list; > DECLARE_HASHTABLE(insn_hash, 20); > struct list_head static_call_list; > + struct list_head mcount_loc_list; > bool ignore_unreachables, c_file, hints, rodata; > }; >
On Thu, Jun 25, 2020 at 10:02:35PM +0200, Peter Zijlstra wrote: > On Thu, Jun 25, 2020 at 09:15:03AM -0700, Sami Tolvanen wrote: > > On Thu, Jun 25, 2020 at 09:45:30AM +0200, Peter Zijlstra wrote: > > > > At least for x86_64 I can do a really quick take for a recordmcount pass > > > in objtool, but I suppose you also need this for ARM64 ? > > > > Sure, sounds good. arm64 uses -fpatchable-function-entry with clang, so we > > don't need recordmcount there. > > This is on top of my local pile: > > git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git master > > which notably includes the static_call series. > > Not boot tested, but it generates the required sections and they look > more or less as expected, ymmv. > > --- > arch/x86/Kconfig | 1 - > scripts/Makefile.build | 3 ++ > scripts/link-vmlinux.sh | 2 +- > tools/objtool/builtin-check.c | 9 ++--- > tools/objtool/builtin.h | 2 +- > tools/objtool/check.c | 81 +++++++++++++++++++++++++++++++++++++++++++ > tools/objtool/check.h | 1 + > tools/objtool/objtool.h | 1 + > 8 files changed, 91 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index a291823f3f26..189575c12434 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -174,7 +174,6 @@ config X86 > select HAVE_EXIT_THREAD > select HAVE_FAST_GUP > select HAVE_FENTRY if X86_64 || DYNAMIC_FTRACE > - select HAVE_FTRACE_MCOUNT_RECORD > select HAVE_FUNCTION_GRAPH_TRACER > select HAVE_FUNCTION_TRACER > select HAVE_GCC_PLUGINS This breaks DYNAMIC_FTRACE according to kernel/trace/ftrace.c: #ifndef CONFIG_FTRACE_MCOUNT_RECORD # error Dynamic ftrace depends on MCOUNT_RECORD #endif And the build errors after that seem to confirm this. It looks like we might need another flag to skip recordmcount. Anyway, since objtool is run before recordmcount, I just left this unchanged for testing and ignored the recordmcount warnings about __mcount_loc already existing. Something is a bit off still though, I see this at boot: ------------[ ftrace bug ]------------ ftrace failed to modify [<ffffffff81000660>] __tracepoint_iter_initcall_level+0x0/0x40 actual: 0f:1f:44:00:00 Initializing ftrace call sites ftrace record flags: 0 (0) expected tramp: ffffffff81056500 ------------[ cut here ]------------ Otherwise, this looks pretty good. Sami
On Thu, Jun 25, 2020 at 03:40:42PM -0700, Sami Tolvanen wrote: > > Not boot tested, but it generates the required sections and they look > > more or less as expected, ymmv. > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > index a291823f3f26..189575c12434 100644 > > --- a/arch/x86/Kconfig > > +++ b/arch/x86/Kconfig > > @@ -174,7 +174,6 @@ config X86 > > select HAVE_EXIT_THREAD > > select HAVE_FAST_GUP > > select HAVE_FENTRY if X86_64 || DYNAMIC_FTRACE > > - select HAVE_FTRACE_MCOUNT_RECORD > > select HAVE_FUNCTION_GRAPH_TRACER > > select HAVE_FUNCTION_TRACER > > select HAVE_GCC_PLUGINS > > This breaks DYNAMIC_FTRACE according to kernel/trace/ftrace.c: > > #ifndef CONFIG_FTRACE_MCOUNT_RECORD > # error Dynamic ftrace depends on MCOUNT_RECORD > #endif > > And the build errors after that seem to confirm this. It looks like we might > need another flag to skip recordmcount. Hurm, Steve, how you want to do that? > Anyway, since objtool is run before recordmcount, I just left this unchanged > for testing and ignored the recordmcount warnings about __mcount_loc already > existing. Something is a bit off still though, I see this at boot: > > ------------[ ftrace bug ]------------ > ftrace failed to modify > [<ffffffff81000660>] __tracepoint_iter_initcall_level+0x0/0x40 > actual: 0f:1f:44:00:00 > Initializing ftrace call sites > ftrace record flags: 0 > (0) > expected tramp: ffffffff81056500 > ------------[ cut here ]------------ > > Otherwise, this looks pretty good. Ha! it is trying to convert the "CALL __fentry__" into a NOP and not finding the CALL -- because objtool already made it a NOP... Weird, I thought recordmcount would also write NOPs, it certainly has code for that. I suppose we can use CC_USING_NOP_MCOUNT to avoid those, but I'd rather Steve explain this before I wreck things further.
On Fri, Jun 26, 2020 at 01:29:31PM +0200, Peter Zijlstra wrote: > On Thu, Jun 25, 2020 at 03:40:42PM -0700, Sami Tolvanen wrote: > > Anyway, since objtool is run before recordmcount, I just left this unchanged > > for testing and ignored the recordmcount warnings about __mcount_loc already > > existing. Something is a bit off still though, I see this at boot: > > > > ------------[ ftrace bug ]------------ > > ftrace failed to modify > > [<ffffffff81000660>] __tracepoint_iter_initcall_level+0x0/0x40 > > actual: 0f:1f:44:00:00 > > Initializing ftrace call sites > > ftrace record flags: 0 > > (0) > > expected tramp: ffffffff81056500 > > ------------[ cut here ]------------ > > > > Otherwise, this looks pretty good. > > Ha! it is trying to convert the "CALL __fentry__" into a NOP and not > finding the CALL -- because objtool already made it a NOP... > > Weird, I thought recordmcount would also write NOPs, it certainly has > code for that. I suppose we can use CC_USING_NOP_MCOUNT to avoid those, > but I'd rather Steve explain this before I wreck things further. Something like so would ignore whatever text is there and rewrite it with ideal_nop. --- diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index c84d28e90a58..98a6a93d7615 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -109,9 +109,11 @@ static int __ref ftrace_modify_code_direct(unsigned long ip, const char *old_code, const char *new_code) { - int ret = ftrace_verify_code(ip, old_code); - if (ret) - return ret; + if (old_code) { + int ret = ftrace_verify_code(ip, old_code); + if (ret) + return ret; + } /* replace the text with the new text */ if (ftrace_poke_late) @@ -124,9 +126,8 @@ ftrace_modify_code_direct(unsigned long ip, const char *old_code, int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec, unsigned long addr) { unsigned long ip = rec->ip; - const char *new, *old; + const char *new; - old = ftrace_call_replace(ip, addr); new = ftrace_nop_replace(); /* @@ -138,7 +139,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_code_direct(ip, NULL, new); /* * x86 overrides ftrace_replace_code -- this function will never be used
On Fri, Jun 26, 2020 at 4:29 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Jun 25, 2020 at 03:40:42PM -0700, Sami Tolvanen wrote: > > > > Not boot tested, but it generates the required sections and they look > > > more or less as expected, ymmv. > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > > index a291823f3f26..189575c12434 100644 > > > --- a/arch/x86/Kconfig > > > +++ b/arch/x86/Kconfig > > > @@ -174,7 +174,6 @@ config X86 > > > select HAVE_EXIT_THREAD > > > select HAVE_FAST_GUP > > > select HAVE_FENTRY if X86_64 || DYNAMIC_FTRACE > > > - select HAVE_FTRACE_MCOUNT_RECORD > > > select HAVE_FUNCTION_GRAPH_TRACER > > > select HAVE_FUNCTION_TRACER > > > select HAVE_GCC_PLUGINS > > > > This breaks DYNAMIC_FTRACE according to kernel/trace/ftrace.c: > > > > #ifndef CONFIG_FTRACE_MCOUNT_RECORD > > # error Dynamic ftrace depends on MCOUNT_RECORD > > #endif > > > > And the build errors after that seem to confirm this. It looks like we might > > need another flag to skip recordmcount. > > Hurm, Steve, how you want to do that? Steven, did you have any thoughts about this? Moving recordmcount to an objtool pass that knows about call sites feels like a much cleaner solution than annotating kernel code to avoid unwanted relocations. Sami
On Fri, 17 Jul 2020 10:28:13 -0700 Sami Tolvanen <samitolvanen@google.com> wrote: > On Fri, Jun 26, 2020 at 4:29 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Thu, Jun 25, 2020 at 03:40:42PM -0700, Sami Tolvanen wrote: > > > > > > Not boot tested, but it generates the required sections and they look > > > > more or less as expected, ymmv. > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > > > index a291823f3f26..189575c12434 100644 > > > > --- a/arch/x86/Kconfig > > > > +++ b/arch/x86/Kconfig > > > > @@ -174,7 +174,6 @@ config X86 > > > > select HAVE_EXIT_THREAD > > > > select HAVE_FAST_GUP > > > > select HAVE_FENTRY if X86_64 || DYNAMIC_FTRACE > > > > - select HAVE_FTRACE_MCOUNT_RECORD > > > > select HAVE_FUNCTION_GRAPH_TRACER > > > > select HAVE_FUNCTION_TRACER > > > > select HAVE_GCC_PLUGINS > > > > > > This breaks DYNAMIC_FTRACE according to kernel/trace/ftrace.c: > > > > > > #ifndef CONFIG_FTRACE_MCOUNT_RECORD > > > # error Dynamic ftrace depends on MCOUNT_RECORD > > > #endif > > > > > > And the build errors after that seem to confirm this. It looks like we might > > > need another flag to skip recordmcount. > > > > Hurm, Steve, how you want to do that? > > Steven, did you have any thoughts about this? Moving recordmcount to > an objtool pass that knows about call sites feels like a much cleaner > solution than annotating kernel code to avoid unwanted relocations. > Bah, I started to reply to this then went to look for details, got distracted, forgot about it, my laptop crashed (due to a zoom call), and I lost the email I was writing (haven't looked in the drafts folder, but my idea about this has changed since anyway). So the problem is that we process mcount references in other areas and that confuses the ftrace modification portion? Someone just submitted a patch for arm64 for this: https://lore.kernel.org/r/20200717143338.19302-1-gregory.herrero@oracle.com Is that what you want? -- Steve
On Fri, Jul 17, 2020 at 10:36 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 17 Jul 2020 10:28:13 -0700 > Sami Tolvanen <samitolvanen@google.com> wrote: > > > On Fri, Jun 26, 2020 at 4:29 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > On Thu, Jun 25, 2020 at 03:40:42PM -0700, Sami Tolvanen wrote: > > > > > > > > Not boot tested, but it generates the required sections and they look > > > > > more or less as expected, ymmv. > > > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > > > > index a291823f3f26..189575c12434 100644 > > > > > --- a/arch/x86/Kconfig > > > > > +++ b/arch/x86/Kconfig > > > > > @@ -174,7 +174,6 @@ config X86 > > > > > select HAVE_EXIT_THREAD > > > > > select HAVE_FAST_GUP > > > > > select HAVE_FENTRY if X86_64 || DYNAMIC_FTRACE > > > > > - select HAVE_FTRACE_MCOUNT_RECORD > > > > > select HAVE_FUNCTION_GRAPH_TRACER > > > > > select HAVE_FUNCTION_TRACER > > > > > select HAVE_GCC_PLUGINS > > > > > > > > This breaks DYNAMIC_FTRACE according to kernel/trace/ftrace.c: > > > > > > > > #ifndef CONFIG_FTRACE_MCOUNT_RECORD > > > > # error Dynamic ftrace depends on MCOUNT_RECORD > > > > #endif > > > > > > > > And the build errors after that seem to confirm this. It looks like we might > > > > need another flag to skip recordmcount. > > > > > > Hurm, Steve, how you want to do that? > > > > Steven, did you have any thoughts about this? Moving recordmcount to > > an objtool pass that knows about call sites feels like a much cleaner > > solution than annotating kernel code to avoid unwanted relocations. > > > > Bah, I started to reply to this then went to look for details, got > distracted, forgot about it, my laptop crashed (due to a zoom call), > and I lost the email I was writing (haven't looked in the drafts > folder, but my idea about this has changed since anyway). > > So the problem is that we process mcount references in other areas and > that confuses the ftrace modification portion? Correct. > Someone just submitted a patch for arm64 for this: > > https://lore.kernel.org/r/20200717143338.19302-1-gregory.herrero@oracle.com > > Is that what you want? That looks like the same issue, but we need to fix this on x86 instead. Sami
On Fri, 17 Jul 2020 10:47:51 -0700 Sami Tolvanen <samitolvanen@google.com> wrote: > > Someone just submitted a patch for arm64 for this: > > > > https://lore.kernel.org/r/20200717143338.19302-1-gregory.herrero@oracle.com > > > > Is that what you want? > > That looks like the same issue, but we need to fix this on x86 instead. Does x86 have a way to differentiate between the two that record mcount can check? -- Steve
On Fri, Jul 17, 2020 at 11:05 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 17 Jul 2020 10:47:51 -0700 > Sami Tolvanen <samitolvanen@google.com> wrote: > > > > Someone just submitted a patch for arm64 for this: > > > > > > https://lore.kernel.org/r/20200717143338.19302-1-gregory.herrero@oracle.com > > > > > > Is that what you want? > > > > That looks like the same issue, but we need to fix this on x86 instead. > > Does x86 have a way to differentiate between the two that record mcount > can check? I'm not sure if looking at the relocation alone is sufficient on x86, we might also have to decode the instruction, which is what objtool does. Did you have any thoughts on Peter's patch, or my initial suggestion, which adds a __nomcount attribute to affected functions? Sami
On Fri, 26 Jun 2020 13:29:31 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, Jun 25, 2020 at 03:40:42PM -0700, Sami Tolvanen wrote: > > > > Not boot tested, but it generates the required sections and they look > > > more or less as expected, ymmv. > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > > index a291823f3f26..189575c12434 100644 > > > --- a/arch/x86/Kconfig > > > +++ b/arch/x86/Kconfig > > > @@ -174,7 +174,6 @@ config X86 > > > select HAVE_EXIT_THREAD > > > select HAVE_FAST_GUP > > > select HAVE_FENTRY if X86_64 || DYNAMIC_FTRACE > > > - select HAVE_FTRACE_MCOUNT_RECORD > > > select HAVE_FUNCTION_GRAPH_TRACER > > > select HAVE_FUNCTION_TRACER > > > select HAVE_GCC_PLUGINS > > > > This breaks DYNAMIC_FTRACE according to kernel/trace/ftrace.c: > > > > #ifndef CONFIG_FTRACE_MCOUNT_RECORD > > # error Dynamic ftrace depends on MCOUNT_RECORD > > #endif > > > > And the build errors after that seem to confirm this. It looks like we might > > need another flag to skip recordmcount. > > Hurm, Steve, how you want to do that? That was added when we removed that dangerous daemon that did the updates, and was added to make sure it didn't come back. We can probably just get rid of it. > > > Anyway, since objtool is run before recordmcount, I just left this unchanged > > for testing and ignored the recordmcount warnings about __mcount_loc already > > existing. Something is a bit off still though, I see this at boot: > > > > ------------[ ftrace bug ]------------ > > ftrace failed to modify > > [<ffffffff81000660>] __tracepoint_iter_initcall_level+0x0/0x40 > > actual: 0f:1f:44:00:00 > > Initializing ftrace call sites > > ftrace record flags: 0 > > (0) > > expected tramp: ffffffff81056500 > > ------------[ cut here ]------------ > > > > Otherwise, this looks pretty good. > > Ha! it is trying to convert the "CALL __fentry__" into a NOP and not > finding the CALL -- because objtool already made it a NOP... > > Weird, I thought recordmcount would also write NOPs, it certainly has > code for that. I suppose we can use CC_USING_NOP_MCOUNT to avoid those, > but I'd rather Steve explain this before I wreck things further. The reason for not having recordmcount insert all the nops, is because x86 has more than one optimal nop which is determined by the machine it runs on, and not at compile time. So we figured just updated it then. We can change it to be a nop on boot, and just modify it if it's not the optimal nop already. That said, Andi Kleen added an option to gcc called -mnop-mcount which will have gcc do both create the mcount section and convert the calls into nops. When doing so, it defines CC_USING_NOP_MCOUNT which will tell ftrace to expect the calls to already be converted. -- Steve
On Mon, 20 Jul 2020 09:52:37 -0700 Sami Tolvanen <samitolvanen@google.com> wrote: > > Does x86 have a way to differentiate between the two that record mcount > > can check? > > I'm not sure if looking at the relocation alone is sufficient on x86, > we might also have to decode the instruction, which is what objtool > does. Did you have any thoughts on Peter's patch, or my initial > suggestion, which adds a __nomcount attribute to affected functions? There's a lot of code in this thread. Can you give me the message-id of Peter's patch in question. Thanks, -- Steve
On Wed, Jul 22, 2020 at 10:58 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Mon, 20 Jul 2020 09:52:37 -0700 > Sami Tolvanen <samitolvanen@google.com> wrote: > > > > Does x86 have a way to differentiate between the two that record mcount > > > can check? > > > > I'm not sure if looking at the relocation alone is sufficient on x86, > > we might also have to decode the instruction, which is what objtool > > does. Did you have any thoughts on Peter's patch, or my initial > > suggestion, which adds a __nomcount attribute to affected functions? > > There's a lot of code in this thread. Can you give me the message-id of > Peter's patch in question. Sure, I was referring to the objtool patch in this message: https://lore.kernel.org/lkml/20200625200235.GQ4781@hirez.programming.kicks-ass.net/ Sami
On Wed, Jul 22, 2020 at 01:55:42PM -0400, Steven Rostedt wrote: > > Ha! it is trying to convert the "CALL __fentry__" into a NOP and not > > finding the CALL -- because objtool already made it a NOP... > > > > Weird, I thought recordmcount would also write NOPs, it certainly has > > code for that. I suppose we can use CC_USING_NOP_MCOUNT to avoid those, > > but I'd rather Steve explain this before I wreck things further. > > The reason for not having recordmcount insert all the nops, is because > x86 has more than one optimal nop which is determined by the machine it > runs on, and not at compile time. So we figured just updated it then. > > We can change it to be a nop on boot, and just modify it if it's not > the optimal nop already. Right, I throught that's what we'd be doing already, anyway: > That said, Andi Kleen added an option to gcc called -mnop-mcount which > will have gcc do both create the mcount section and convert the calls > into nops. When doing so, it defines CC_USING_NOP_MCOUNT which will > tell ftrace to expect the calls to already be converted. That seems like the much easier solution, then we can forget about recordmcount / objtool entirely for this.
On Wed, 22 Jul 2020 20:41:37 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > > That said, Andi Kleen added an option to gcc called -mnop-mcount which > > will have gcc do both create the mcount section and convert the calls > > into nops. When doing so, it defines CC_USING_NOP_MCOUNT which will > > tell ftrace to expect the calls to already be converted. > > That seems like the much easier solution, then we can forget about > recordmcount / objtool entirely for this. Of course that was only for some gcc compilers, and I'm not sure if clang can do this. Or do you just see all compilers doing this in the future, and not worrying about record-mcount at all, and bothering with objtool? -- Steve
On Wed, Jul 22, 2020 at 12:09 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 22 Jul 2020 20:41:37 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > > That said, Andi Kleen added an option to gcc called -mnop-mcount which > > > will have gcc do both create the mcount section and convert the calls > > > into nops. When doing so, it defines CC_USING_NOP_MCOUNT which will > > > tell ftrace to expect the calls to already be converted. > > > > That seems like the much easier solution, then we can forget about > > recordmcount / objtool entirely for this. > > Of course that was only for some gcc compilers, and I'm not sure if > clang can do this. > > Or do you just see all compilers doing this in the future, and not > worrying about record-mcount at all, and bothering with objtool? Clang appears to only support -mrecord-mcount and -mnop-mcount for s390, so we still need recordmcount / objtool for x86. Sami
On Wed, Jul 22, 2020 at 03:09:43PM -0400, Steven Rostedt wrote: > On Wed, 22 Jul 2020 20:41:37 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > > That said, Andi Kleen added an option to gcc called -mnop-mcount which > > > will have gcc do both create the mcount section and convert the calls > > > into nops. When doing so, it defines CC_USING_NOP_MCOUNT which will > > > tell ftrace to expect the calls to already be converted. > > > > That seems like the much easier solution, then we can forget about > > recordmcount / objtool entirely for this. > > Of course that was only for some gcc compilers, and I'm not sure if > clang can do this. > > Or do you just see all compilers doing this in the future, and not > worrying about record-mcount at all, and bothering with objtool? I got the GCC version wrong :/ Both -mnop-mcount and -mrecord-mcount landed in GCC-5, where our minimum GCC is now at 4.9. Anyway, what do you prefer, I suppose I can make objtool whatever we need, that patch is trivial. Simply recording the sites and not rewriting them should be simple enough.
On Thu, 23 Jul 2020 01:56:20 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > Anyway, what do you prefer, I suppose I can make objtool whatever we > need, that patch is trivial. Simply recording the sites and not > rewriting them should be simple enough. Either way. If objtool turns it into nops, just make it where we can enable -DCC_USING_NOP_MCOUNT set, and the kernel will be unaware. Or if you just add the locations, then that would work too. -- Steve
On Wed, Jul 22, 2020 at 08:06:08PM -0400, Steven Rostedt wrote: > On Thu, 23 Jul 2020 01:56:20 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > Anyway, what do you prefer, I suppose I can make objtool whatever we > > need, that patch is trivial. Simply recording the sites and not > > rewriting them should be simple enough. > > Either way. If objtool turns it into nops, just make it where we can > enable -DCC_USING_NOP_MCOUNT set, and the kernel will be unaware. > > Or if you just add the locations, then that would work too. I took Peter's earlier patch, rebased it on top of the current mainline tree for easier testing, and tweaked the makefiles to only use objtool --mcount when CONFIG_STACK_VALIDATION is enabled and the compiler supports -mfentry. This works for me with both gcc and clang. Thoughts? Sami --- Makefile | 38 ++++++++++++---- arch/x86/Kconfig | 1 + kernel/trace/Kconfig | 5 +++ scripts/Makefile.build | 9 ++-- tools/objtool/builtin-check.c | 3 +- tools/objtool/builtin.h | 2 +- tools/objtool/check.c | 83 +++++++++++++++++++++++++++++++++++ tools/objtool/check.h | 1 + tools/objtool/objtool.h | 1 + 9 files changed, 129 insertions(+), 14 deletions(-) diff --git a/Makefile b/Makefile index 5cfc3481207f..2d23b6b6c4c9 100644 --- a/Makefile +++ b/Makefile @@ -864,17 +864,34 @@ ifdef CONFIG_HAVE_FENTRY ifeq ($(call cc-option-yn, -mfentry),y) CC_FLAGS_FTRACE += -mfentry CC_FLAGS_USING += -DCC_USING_FENTRY + export CC_USING_FENTRY := 1 endif endif export CC_FLAGS_FTRACE -KBUILD_CFLAGS += $(CC_FLAGS_FTRACE) $(CC_FLAGS_USING) -KBUILD_AFLAGS += $(CC_FLAGS_USING) ifdef CONFIG_DYNAMIC_FTRACE - ifdef CONFIG_HAVE_C_RECORDMCOUNT - BUILD_C_RECORDMCOUNT := y - export BUILD_C_RECORDMCOUNT - endif + ifndef CC_USING_RECORD_MCOUNT + ifndef CC_USING_PATCHABLE_FUNCTION_ENTRY + # use objtool or recordmcount to generate mcount tables + ifdef CONFIG_HAVE_OBJTOOL_MCOUNT + ifdef CC_USING_FENTRY + USE_OBJTOOL_MCOUNT := y + CC_FLAGS_USING += -DCC_USING_NOP_MCOUNT + export USE_OBJTOOL_MCOUNT + endif + endif + ifndef USE_OBJTOOL_MCOUNT + USE_RECORDMCOUNT := y + export USE_RECORDMCOUNT + ifdef CONFIG_HAVE_C_RECORDMCOUNT + BUILD_C_RECORDMCOUNT := y + export BUILD_C_RECORDMCOUNT + endif + endif + endif + endif endif +KBUILD_CFLAGS += $(CC_FLAGS_FTRACE) $(CC_FLAGS_USING) +KBUILD_AFLAGS += $(CC_FLAGS_USING) endif # We trigger additional mismatches with less inlining @@ -1211,11 +1228,16 @@ uapi-asm-generic: PHONY += prepare-objtool prepare-resolve_btfids prepare-objtool: $(objtool_target) ifeq ($(SKIP_STACK_VALIDATION),1) +objtool-lib-prompt := "please install libelf-dev, libelf-devel or elfutils-libelf-devel" +ifdef USE_OBJTOOL_MCOUNT + @echo "error: Cannot generate __mcount_loc for CONFIG_DYNAMIC_FTRACE=y, $(objtool-lib-prompt)" >&2 + @false +endif ifdef CONFIG_UNWINDER_ORC - @echo "error: Cannot generate ORC metadata for CONFIG_UNWINDER_ORC=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel" >&2 + @echo "error: Cannot generate ORC metadata for CONFIG_UNWINDER_ORC=y, $(objtool-lib-prompt)" >&2 @false else - @echo "warning: Cannot use CONFIG_STACK_VALIDATION=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel" >&2 + @echo "warning: Cannot use CONFIG_STACK_VALIDATION=y, $(objtool-lib-prompt)" >&2 endif endif diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 9a2849527dd7..149c94a44cf0 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -163,6 +163,7 @@ config X86 select HAVE_CMPXCHG_LOCAL select HAVE_CONTEXT_TRACKING if X86_64 select HAVE_C_RECORDMCOUNT + select HAVE_OBJTOOL_MCOUNT if STACK_VALIDATION select HAVE_DEBUG_KMEMLEAK select HAVE_DMA_CONTIGUOUS select HAVE_DYNAMIC_FTRACE diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig index a4020c0b4508..b510af5b216c 100644 --- a/kernel/trace/Kconfig +++ b/kernel/trace/Kconfig @@ -56,6 +56,11 @@ config HAVE_C_RECORDMCOUNT help C version of recordmcount available? +config HAVE_OBJTOOL_MCOUNT + bool + help + Arch supports objtool --mcount + config TRACER_MAX_TRACE bool diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 2e8810b7e5ed..f66f8c0ef294 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -175,8 +175,7 @@ cmd_modversions_c = \ fi endif -ifdef CONFIG_FTRACE_MCOUNT_RECORD -ifndef CC_USING_RECORD_MCOUNT +ifdef USE_RECORDMCOUNT # compiler will not generate __mcount_loc use recordmcount or recordmcount.pl ifdef BUILD_C_RECORDMCOUNT ifeq ("$(origin RECORDMCOUNT_WARN)", "command line") @@ -203,8 +202,7 @@ recordmcount_source := $(srctree)/scripts/recordmcount.pl endif # BUILD_C_RECORDMCOUNT cmd_record_mcount = $(if $(findstring $(strip $(CC_FLAGS_FTRACE)),$(_c_flags)), \ $(sub_cmd_record_mcount)) -endif # CC_USING_RECORD_MCOUNT -endif # CONFIG_FTRACE_MCOUNT_RECORD +endif # USE_RECORDMCOUNT ifdef CONFIG_STACK_VALIDATION ifneq ($(SKIP_STACK_VALIDATION),1) @@ -227,6 +225,9 @@ endif ifdef CONFIG_X86_SMAP objtool_args += --uaccess endif +ifdef USE_OBJTOOL_MCOUNT + objtool_args += --mcount +endif # 'OBJECT_FILES_NON_STANDARD := y': skip objtool checking for a directory # 'OBJECT_FILES_NON_STANDARD_foo.o := 'y': skip objtool checking for a file diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c index 7a44174967b5..71595cf4946d 100644 --- a/tools/objtool/builtin-check.c +++ b/tools/objtool/builtin-check.c @@ -18,7 +18,7 @@ #include "builtin.h" #include "objtool.h" -bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux; +bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, mcount; static const char * const check_usage[] = { "objtool check [<options>] file.o", @@ -35,6 +35,7 @@ const struct option check_options[] = { OPT_BOOLEAN('s', "stats", &stats, "print statistics"), OPT_BOOLEAN('d', "duplicate", &validate_dup, "duplicate validation for vmlinux.o"), OPT_BOOLEAN('l', "vmlinux", &vmlinux, "vmlinux.o validation"), + OPT_BOOLEAN('M', "mcount", &mcount, "generate __mcount_loc"), OPT_END(), }; diff --git a/tools/objtool/builtin.h b/tools/objtool/builtin.h index 85c979caa367..94565a72b701 100644 --- a/tools/objtool/builtin.h +++ b/tools/objtool/builtin.h @@ -8,7 +8,7 @@ #include <subcmd/parse-options.h> extern const struct option check_options[]; -extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux; +extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, mcount; extern int cmd_check(int argc, const char **argv); extern int cmd_orc(int argc, const char **argv); diff --git a/tools/objtool/check.c b/tools/objtool/check.c index e034a8f24f46..6e0b478dc065 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -433,6 +433,65 @@ static int add_dead_ends(struct objtool_file *file) return 0; } +static int create_mcount_loc_sections(struct objtool_file *file) +{ + struct section *sec, *reloc_sec; + struct reloc *reloc; + unsigned long *loc; + struct instruction *insn; + int idx; + + sec = find_section_by_name(file->elf, "__mcount_loc"); + if (sec) { + INIT_LIST_HEAD(&file->mcount_loc_list); + WARN("file already has __mcount_loc section, skipping"); + return 0; + } + + if (list_empty(&file->mcount_loc_list)) + return 0; + + idx = 0; + list_for_each_entry(insn, &file->mcount_loc_list, mcount_loc_node) + idx++; + + sec = elf_create_section(file->elf, "__mcount_loc", sizeof(unsigned long), idx); + if (!sec) + return -1; + + reloc_sec = elf_create_reloc_section(file->elf, sec, SHT_RELA); + if (!reloc_sec) + return -1; + + idx = 0; + list_for_each_entry(insn, &file->mcount_loc_list, mcount_loc_node) { + + loc = (unsigned long *)sec->data->d_buf + idx; + memset(loc, 0, sizeof(unsigned long)); + + reloc = malloc(sizeof(*reloc)); + if (!reloc) { + perror("malloc"); + return -1; + } + memset(reloc, 0, sizeof(*reloc)); + + reloc->sym = insn->sec->sym; + reloc->addend = insn->offset; + reloc->type = R_X86_64_64; + reloc->offset = idx * sizeof(unsigned long); + reloc->sec = reloc_sec; + elf_add_reloc(file->elf, reloc); + + idx++; + } + + if (elf_rebuild_reloc_section(file->elf, reloc_sec)) + return -1; + + return 0; +} + /* * Warnings shouldn't be reported for ignored functions. */ @@ -784,6 +843,22 @@ static int add_call_destinations(struct objtool_file *file) insn->type = INSN_NOP; } + if (mcount && !strcmp(insn->call_dest->name, "__fentry__")) { + if (reloc) { + reloc->type = R_NONE; + elf_write_reloc(file->elf, reloc); + } + + elf_write_insn(file->elf, insn->sec, + insn->offset, insn->len, + arch_nop_insn(insn->len)); + + insn->type = INSN_NOP; + + list_add_tail(&insn->mcount_loc_node, + &file->mcount_loc_list); + } + /* * Whatever stack impact regular CALLs have, should be undone * by the RETURN of the called function. @@ -2791,6 +2866,7 @@ int check(const char *_objname, bool orc) INIT_LIST_HEAD(&file.insn_list); hash_init(file.insn_hash); + INIT_LIST_HEAD(&file.mcount_loc_list); file.c_file = !vmlinux && find_section_by_name(file.elf, ".comment"); file.ignore_unreachables = no_unreachable; file.hints = false; @@ -2838,6 +2914,13 @@ int check(const char *_objname, bool orc) warnings += ret; } + if (mcount) { + ret = create_mcount_loc_sections(&file); + if (ret < 0) + goto out; + warnings += ret; + } + if (orc) { ret = create_orc(&file); if (ret < 0) diff --git a/tools/objtool/check.h b/tools/objtool/check.h index 061aa96e15d3..b62afd3d970b 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -22,6 +22,7 @@ struct insn_state { struct instruction { struct list_head list; struct hlist_node hash; + struct list_head mcount_loc_node; struct section *sec; unsigned long offset; unsigned int len; diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h index 528028a66816..427806079540 100644 --- a/tools/objtool/objtool.h +++ b/tools/objtool/objtool.h @@ -16,6 +16,7 @@ struct objtool_file { struct elf *elf; struct list_head insn_list; DECLARE_HASHTABLE(insn_hash, 20); + struct list_head mcount_loc_list; bool ignore_unreachables, c_file, hints, rodata; };
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index a291823f3f26..189575c12434 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -174,7 +174,6 @@ config X86 select HAVE_EXIT_THREAD select HAVE_FAST_GUP select HAVE_FENTRY if X86_64 || DYNAMIC_FTRACE - select HAVE_FTRACE_MCOUNT_RECORD select HAVE_FUNCTION_GRAPH_TRACER select HAVE_FUNCTION_TRACER select HAVE_GCC_PLUGINS diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 2e8810b7e5ed..c774befc57da 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -227,6 +227,9 @@ endif ifdef CONFIG_X86_SMAP objtool_args += --uaccess endif +ifdef CONFIG_DYNAMIC_FTRACE + objtool_args += --mcount +endif # 'OBJECT_FILES_NON_STANDARD := y': skip objtool checking for a directory # 'OBJECT_FILES_NON_STANDARD_foo.o := 'y': skip objtool checking for a file diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh index 92dd745906f4..00c6e4f28a1a 100755 --- a/scripts/link-vmlinux.sh +++ b/scripts/link-vmlinux.sh @@ -60,7 +60,7 @@ objtool_link() local objtoolopt; if [ -n "${CONFIG_VMLINUX_VALIDATION}" ]; then - objtoolopt="check" + objtoolopt="check --vmlinux" if [ -z "${CONFIG_FRAME_POINTER}" ]; then objtoolopt="${objtoolopt} --no-fp" fi diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c index 4896c5a89702..a6c3a3fba67d 100644 --- a/tools/objtool/builtin-check.c +++ b/tools/objtool/builtin-check.c @@ -18,7 +18,7 @@ #include "builtin.h" #include "objtool.h" -bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, fpu; +bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, fpu, mcount; static const char * const check_usage[] = { "objtool check [<options>] file.o", @@ -36,12 +36,13 @@ const struct option check_options[] = { OPT_BOOLEAN('d', "duplicate", &validate_dup, "duplicate validation for vmlinux.o"), OPT_BOOLEAN('l', "vmlinux", &vmlinux, "vmlinux.o validation"), OPT_BOOLEAN('F', "fpu", &fpu, "validate FPU context"), + OPT_BOOLEAN('M', "mcount", &mcount, "generate __mcount_loc"), OPT_END(), }; int cmd_check(int argc, const char **argv) { - const char *objname, *s; + const char *objname; argc = parse_options(argc, argv, check_options, check_usage, 0); @@ -50,9 +51,5 @@ int cmd_check(int argc, const char **argv) objname = argv[0]; - s = strstr(objname, "vmlinux.o"); - if (s && !s[9]) - vmlinux = true; - return check(objname, false); } diff --git a/tools/objtool/builtin.h b/tools/objtool/builtin.h index 7158e09d4cc9..b51d883ec245 100644 --- a/tools/objtool/builtin.h +++ b/tools/objtool/builtin.h @@ -8,7 +8,7 @@ #include <subcmd/parse-options.h> extern const struct option check_options[]; -extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, fpu; +extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux, fpu, mcount; extern int cmd_check(int argc, const char **argv); extern int cmd_orc(int argc, const char **argv); diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 6647a8d1545b..ee99566bdae9 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -533,6 +533,65 @@ static int create_static_call_sections(struct objtool_file *file) return 0; } +static int create_mcount_loc_sections(struct objtool_file *file) +{ + struct section *sec, *reloc_sec; + struct reloc *reloc; + unsigned long *loc; + struct instruction *insn; + int idx; + + sec = find_section_by_name(file->elf, "__mcount_loc"); + if (sec) { + INIT_LIST_HEAD(&file->mcount_loc_list); + WARN("file already has __mcount_loc section, skipping"); + return 0; + } + + if (list_empty(&file->mcount_loc_list)) + return 0; + + idx = 0; + list_for_each_entry(insn, &file->mcount_loc_list, mcount_loc_node) + idx++; + + sec = elf_create_section(file->elf, "__mcount_loc", 0, sizeof(unsigned long), idx); + if (!sec) + return -1; + + reloc_sec = elf_create_reloc_section(file->elf, sec, SHT_RELA); + if (!reloc_sec) + return -1; + + idx = 0; + list_for_each_entry(insn, &file->mcount_loc_list, mcount_loc_node) { + + loc = (unsigned long *)sec->data->d_buf + idx; + memset(loc, 0, sizeof(unsigned long)); + + reloc = malloc(sizeof(*reloc)); + if (!reloc) { + perror("malloc"); + return -1; + } + memset(reloc, 0, sizeof(*reloc)); + + reloc->sym = insn->sec->sym; + reloc->addend = insn->offset; + reloc->type = R_X86_64_64; + reloc->offset = idx * sizeof(unsigned long); + reloc->sec = reloc_sec; + elf_add_reloc(file->elf, reloc); + + idx++; + } + + if (elf_rebuild_reloc_section(file->elf, reloc_sec)) + return -1; + + return 0; +} + /* * Warnings shouldn't be reported for ignored functions. */ @@ -892,6 +951,22 @@ static int add_call_destinations(struct objtool_file *file) insn->type = INSN_NOP; } + if (mcount && !strcmp(insn->call_dest->name, "__fentry__")) { + if (reloc) { + reloc->type = R_NONE; + elf_write_reloc(file->elf, reloc); + } + + elf_write_insn(file->elf, insn->sec, + insn->offset, insn->len, + arch_nop_insn(insn->len)); + + insn->type = INSN_NOP; + + list_add_tail(&insn->mcount_loc_node, + &file->mcount_loc_list); + } + /* * Whatever stack impact regular CALLs have, should be undone * by the RETURN of the called function. @@ -3004,6 +3079,7 @@ int check(const char *_objname, bool orc) INIT_LIST_HEAD(&file.insn_list); hash_init(file.insn_hash); INIT_LIST_HEAD(&file.static_call_list); + INIT_LIST_HEAD(&file.mcount_loc_list); file.c_file = !vmlinux && find_section_by_name(file.elf, ".comment"); file.ignore_unreachables = no_unreachable; file.hints = false; @@ -3056,6 +3132,11 @@ int check(const char *_objname, bool orc) goto out; warnings += ret; + ret = create_mcount_loc_sections(&file); + if (ret < 0) + goto out; + warnings += ret; + if (orc) { ret = create_orc(&file); if (ret < 0) diff --git a/tools/objtool/check.h b/tools/objtool/check.h index cd95fca0d237..01f11b5da5dd 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -24,6 +24,7 @@ struct instruction { struct list_head list; struct hlist_node hash; struct list_head static_call_node; + struct list_head mcount_loc_node; struct section *sec; unsigned long offset; unsigned int len; diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h index 9a7cd0b88bd8..f604b22d22cc 100644 --- a/tools/objtool/objtool.h +++ b/tools/objtool/objtool.h @@ -17,6 +17,7 @@ struct objtool_file { struct list_head insn_list; DECLARE_HASHTABLE(insn_hash, 20); struct list_head static_call_list; + struct list_head mcount_loc_list; bool ignore_unreachables, c_file, hints, rodata; };