From: Ingo Molnar <mingo@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Andrii Nakryiko <andrii.nakryiko@gmail.com>,
Masami Hiramatsu <mhiramat@kernel.org>,
Jiri Olsa <jolsa@kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@chromium.org>,
Peter Zijlstra <peterz@infradead.org>,
x86@kernel.org
Subject: Re: [PATCH v2] ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak function
Date: Thu, 26 May 2022 04:50:17 +0200 [thread overview]
Message-ID: <Yo7q6dwphFexGuRA@gmail.com> (raw)
In-Reply-To: <20220525180553.419eac77@gandalf.local.home>
* 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.
Yeah, so I actually have a (prototype...) objtool based kallsyms
implementation in the (way too large) fast-headers tree that is both faster
& allows such details in principle:
431bca135cf8 kbuild/linker: Gather all the __kallsyms sections into a single table
6bc7af02e402 objtool/kallsyms: Copy the symbol name and offset to the new __kallsyms ELF section
e1e85b2fab9e objtool/kallsyms: Increase section size dynamically
3528d607641b objtool/kallsyms: Use zero entry size for section
2555dd62348a objtool/kallsyms: Output variable length strings
c114b71f8547 objtool/kallsyms: Add kallsyms_offsets[] table
cfcfce6cb51f objtool/kallsyms: Change name to __kallsyms_strs
134160bb2de1 objtool/kallsyms: Split out process_kallsyms_symbols()
33347a4b46e0 objtool/kallsyms: Add relocations
86626e9e6603 objtool/kallsyms: Skip discarded sections
7dd9fef6fbb0 kallsyms/objtool: Print out the new symbol table on the kernel side
c82d94b33a1f objtool/kallsyms: Use struct kallsyms_entry
a66ee5034008 objtool/kallsyms, kallsyms/objtool: Share 'struct kallsyms_entry' definition
e496159e5282 kallsyms/objtool: Process entries
47fc63ef3fa8 objtool/kallsyms: Switch to 64-bit absolute relocations
c54fcc03cd64 objtool/kallsyms: Fix initial offset
eac3c107aa6e kallsyms/objtool: Emit System.map-alike symbol list
ebfb7e14b8ca objtool/kallsyms: Skip undefined symbols
25b69ef5666b objtool/kallsyms: Update symbol filter
3b26a82c733f objtool/kallsyms: Add the CONFIG_KALLSYMS_FAST Kconfig option & its related Kconfig switches
9350c25942f8 kallsyms/objtool: Make the kallsyms work even if the generic tables are not there
4a0a120bde05 objtool/kallsyms, x86/relocs: Detect & ignore __kallsyms_offset section relocations
87c5244f1fa8 kallsyms/objtool: Introduce linear table of symbol structures: kallsyms_syms[]
51dafdefc61f kallsyms/objtool: Split fast vs. generic functions
e4123a40125f kallsyms/objtool: Sort symbols by address and deduplicate them
2d738c69965a kallsyms/objtool: Utilize the kallsyms_syms[] table in kallsyms_expand_symbol() and kallsyms_sym_address()
997ffe217a34 kallsyms/objtool: Port kallsyms_relative_base functionality to the kallsyms_syms[] offsets
> 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>
LGTM.
I suppose you'd like to merge this via the tracing tree? If so:
Acked-by: Ingo Molnar <mingo@kernel.org>
Thanks,
Ingo
next prev parent reply other threads:[~2022-05-26 2:50 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-25 22:05 [PATCH v2] ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak function Steven Rostedt
2022-05-26 2:50 ` Ingo Molnar [this message]
2022-05-26 13:11 ` Steven Rostedt
2022-05-27 10:04 ` Ingo Molnar
2022-05-27 20:36 ` Steven Rostedt
2022-05-26 7:39 ` Peter Zijlstra
2022-05-26 13:14 ` Steven Rostedt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yo7q6dwphFexGuRA@gmail.com \
--to=mingo@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kafai@fb.com \
--cc=kpsingh@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=songliubraving@fb.com \
--cc=x86@kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.