| Message ID | 20220503150410.2d9e88aa@rorschach.local.home (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | : [PATCH] ftrace/x86: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak functions | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on z15 + selftests |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest + selftests |
| netdev/tree_selection | success | Not a local patch |
On Tue, May 03, 2022 at 03:04:10PM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > If an unused weak function was traced, it's call to fentry will still > exist, which gets added into the __mcount_loc table. Ftrace will use > kallsyms to retrieve the name for each location in __mcount_loc to display > it in the available_filter_functions and used to enable functions via the > name matching in set_ftrace_filter/notrace. Enabling these functions do > nothing but enable an unused call to ftrace_caller. If a traced weak > function is overridden, the symbol of the function would be used for it, > which will either created duplicate names, or if the previous function was > not traced, it would be incorrectly listed in available_filter_functions > as a function that can be traced. > > This became an issue with BPF[1] as there are tooling that enables the > direct callers via ftrace but then checks to see if the functions were > actually enabled. The case of one function that was marked notrace, but > was followed by an unused weak function that was traced. The unused > function's call to fentry was added to the __mcount_loc section, and > kallsyms retrieved the untraced function's symbol as the weak function was > overridden. Since the untraced function would not get traced, the BPF > check would detect this and fail. > > The real fix would be to fix kallsyms to not show address of weak > functions as the function before it. But that would require adding code in > the build to add function size to kallsyms so that it can know when the > function ends instead of just using the start of the next known symbol. > > In the mean time, this is a work around. Add a FTRACE_MCOUNT_MAX_OFFSET > macro that if defined, ftrace will ignore any function that has its call > to fentry/mcount that has an offset from the symbol that is greater than > FTRACE_MCOUNT_MAX_OFFSET. So for x86-64... objtool knows about these holes and *could* squash these entries if you want (at the cost of requiring link time objtool run). Also see commit: 4adb23686795 ("objtool: Ignore extra-symbol code") But yeah, ensuring fentry is close ought to work. > If CONFIG_HAVE_FENTRY is defined for x86, define FTRACE_MCOUNT_MAX_OFFSET > to zero, which will have ftrace ignore all locations that are not at the > start of the function. You forgot about IBT again? __fentry__ no longer lives at +0 on x86 anymore.
On Tue, May 3, 2022 at 12:04 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > If an unused weak function was traced, it's call to fentry will still > exist, which gets added into the __mcount_loc table. Ftrace will use > kallsyms to retrieve the name for each location in __mcount_loc to display > it in the available_filter_functions and used to enable functions via the > name matching in set_ftrace_filter/notrace. Enabling these functions do > nothing but enable an unused call to ftrace_caller. If a traced weak > function is overridden, the symbol of the function would be used for it, > which will either created duplicate names, or if the previous function was > not traced, it would be incorrectly listed in available_filter_functions > as a function that can be traced. > > This became an issue with BPF[1] as there are tooling that enables the > direct callers via ftrace but then checks to see if the functions were > actually enabled. The case of one function that was marked notrace, but > was followed by an unused weak function that was traced. The unused > function's call to fentry was added to the __mcount_loc section, and > kallsyms retrieved the untraced function's symbol as the weak function was > overridden. Since the untraced function would not get traced, the BPF > check would detect this and fail. > > The real fix would be to fix kallsyms to not show address of weak > functions as the function before it. But that would require adding code in > the build to add function size to kallsyms so that it can know when the > function ends instead of just using the start of the next known symbol. > > In the mean time, this is a work around. Add a FTRACE_MCOUNT_MAX_OFFSET > macro that if defined, ftrace will ignore any function that has its call > to fentry/mcount that has an offset from the symbol that is greater than > FTRACE_MCOUNT_MAX_OFFSET. > > If CONFIG_HAVE_FENTRY is defined for x86, define FTRACE_MCOUNT_MAX_OFFSET > to zero, which will have ftrace ignore all locations that are not at the > start of the function. > > [1] https://lore.kernel.org/all/20220412094923.0abe90955e5db486b7bca279@kernel.org/ > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > arch/x86/include/asm/ftrace.h | 5 ++++ > kernel/trace/ftrace.c | 50 +++++++++++++++++++++++++++++++++-- > 2 files changed, 53 insertions(+), 2 deletions(-) > Thanks for investigating and fixing this! I guess we'll need ENDBR handling, but otherwise it looks good to me!
On Fri, 6 May 2022 21:38:29 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > You forgot about IBT again? __fentry__ no longer lives at +0 on x86 > anymore. I didn't forget. It was that the kernel I wrote this for didn't have it ;-) I wasn't sure if it was accepted into mainline yet. -- Steve
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h index 024d9797646e..70c88d49bf45 100644 --- a/arch/x86/include/asm/ftrace.h +++ b/arch/x86/include/asm/ftrace.h @@ -9,6 +9,11 @@ # define MCOUNT_ADDR ((unsigned long)(__fentry__)) #define MCOUNT_INSN_SIZE 5 /* sizeof mcount call */ +/* Ignore unused weak functions which will have non zero offsets */ +#ifdef CONFIG_HAVE_FENTRY +# define FTRACE_MCOUNT_MAX_OFFSET 0 +#endif + #ifdef CONFIG_DYNAMIC_FTRACE #define ARCH_SUPPORTS_FTRACE_OPS 1 #endif diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 5c465e70d146..3529c44ab9db 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -3665,6 +3665,31 @@ static void add_trampoline_func(struct seq_file *m, struct ftrace_ops *ops, seq_printf(m, " ->%pS", ptr); } +#ifdef FTRACE_MCOUNT_MAX_OFFSET +static int print_rec(struct seq_file *m, unsigned long ip) +{ + unsigned long offset; + char str[KSYM_SYMBOL_LEN]; + char *modname; + int ret; + + ret = kallsyms_lookup(ip, NULL, &offset, &modname, str); + if (!ret || offset > FTRACE_MCOUNT_MAX_OFFSET) + return -1; + + seq_puts(m, str); + if (modname) + seq_printf(m, " [%s]", modname); + return 0; +} +#else +static int print_rec(struct seq_file *m, unsigned long ip) +{ + seq_printf(m, "%ps", (void *)ip); + return 0; +} +#endif + static int t_show(struct seq_file *m, void *v) { struct ftrace_iterator *iter = m->private; @@ -3689,7 +3714,9 @@ static int t_show(struct seq_file *m, void *v) if (!rec) return 0; - seq_printf(m, "%ps", (void *)rec->ip); + if (print_rec(m, rec->ip)) + return 0; + if (iter->flags & FTRACE_ITER_ENABLED) { struct ftrace_ops *ops; @@ -4007,6 +4034,24 @@ add_rec_by_index(struct ftrace_hash *hash, struct ftrace_glob *func_g, return 0; } +#ifdef FTRACE_MCOUNT_MAX_OFFSET +static int lookup_ip(unsigned long ip, char **modname, char *str) +{ + unsigned long offset; + + kallsyms_lookup(ip, NULL, &offset, modname, str); + if (offset > FTRACE_MCOUNT_MAX_OFFSET) + return -1; + return 0; +} +#else +static int lookup_ip(unsigned long ip, char **modname, char *str) +{ + kallsyms_lookup(ip, NULL, NULL, modname, str); + return 0; +} +#endif + static int ftrace_match_record(struct dyn_ftrace *rec, struct ftrace_glob *func_g, struct ftrace_glob *mod_g, int exclude_mod) @@ -4014,7 +4059,8 @@ ftrace_match_record(struct dyn_ftrace *rec, struct ftrace_glob *func_g, char str[KSYM_SYMBOL_LEN]; char *modname; - kallsyms_lookup(rec->ip, NULL, NULL, &modname, str); + if (lookup_ip(rec->ip, &modname, str)) + return 0; if (mod_g) { int mod_matches = (modname) ? ftrace_match(modname, mod_g) : 0;