linux-kernel - Re: [PATCH 14/14] scripts/sorttable: ftrace: Do not add weak functions to available_filter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250103114140.GF22934@noisy.programming.kicks-ass.net>
Date: Fri, 3 Jan 2025 12:41:40 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Jiri Olsa <olsajiri@...il.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, linux-kernel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org,
	bpf <bpf@...r.kernel.org>, Masami Hiramatsu <mhiramat@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Masahiro Yamada <masahiroy@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nicolas Schier <nicolas@...sle.eu>,
	Zheng Yejian <zhengyejian1@...wei.com>,
	Martin Kelly <martin.kelly@...wdstrike.com>,
	Christophe Leroy <christophe.leroy@...roup.eu>,
	Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH 14/14] scripts/sorttable: ftrace: Do not add weak
 functions to available_filter_functions

On Fri, Jan 03, 2025 at 12:10:08PM +0100, Jiri Olsa wrote:
> On Thu, Jan 02, 2025 at 01:58:59PM -0500, Steven Rostedt wrote:
> > From: Steven Rostedt <rostedt@...dmis.org>
> > 
> > When a function is annotated as "weak" and is overridden, the code is not
> > removed. If it is traced, the fentry/mcount location in the weak function
> > will be referenced by the "__mcount_loc" section. This will then be added
> > to the available_filter_functions list. Since only the address of the
> > functions are listed, to find the name to show, a search of kallsyms is
> > used.
> > 
> > Since kallsyms will return the function by simply finding the function
> > that the address is after but before the next function, an address of a
> > weak function will show up as the function before it. This is because
> > kallsyms does not save names of weak functions. This has caused issues in
> > the past, as now the traced weak function will be listed in
> > available_filter_functions with the name of the function before it.
> > 
> > At best, this will cause the previous function's name to be listed twice.
> > At worse, if the previous function was marked notrace, it will now show up
> > as a function that can be traced. Note that it only shows up that it can
> > be traced but will not be if enabled, which causes confusion.
> > 
> >  https://lore.kernel.org/all/20220412094923.0abe90955e5db486b7bca279@kernel.org/
> > 
> > The commit b39181f7c6907 ("ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid
> > adding weak function") was a workaround to this by checking the function
> > address before printing its name. If the address was too far from the
> > function given by the name then instead of printing the name it would
> > print: __ftrace_invalid_address___<invalid-offset>
> > 
> > The real issue is that these invalid addresses are listed in the ftrace
> > table look up which available_filter_functions is derived from. A place
> > holder must be listed in that file because set_ftrace_filter may take a
> > series of indexes into that file instead of names to be able to do O(1)
> > lookups to enable filtering (many tools use this method).
> > 
> > Even if kallsyms saved the size of the function, it does not remove the
> > need of having these place holders. The real solution is to not add a weak
> > function into the ftrace table in the first place.
> > 
> > To solve this, the sorttable.c code that sorts the mcount regions during
> > the build is modified to take a "nm -S vmlinux" input, sort it, and any
> > function listed in the mcount_loc section that is not within a boundary of
> > the function list given by nm is considered a weak function and is zeroed
> > out. Note, this does not mean they will remain zero when booting as KASLR
> > will still shift those addresses.
> 
> hi,
> fyi this seems to remove several functions from available_filter_functions,
> that bpf relay on.. like update_socket_protocol or bpf_rstat_flush:
> 
> 	__bpf_hook_start();
> 
> 	__weak noinline int update_socket_protocol(int family, int type, int protocol)
> 	{
> 		return protocol;
> 	}
> 
> 	__bpf_hook_end();
> 
> 
> 	[root@...u-1 tracing]# cat available_filter_functions | grep update_socket_protocol
> 	[root@...u-1 tracing]# cat /proc/kallsyms | grep update_socket_protocol
> 	ffffffff821d58b0 W __pfx_update_socket_protocol
> 	ffffffff821d58c0 W update_socket_protocol
> 
> not sure why that fits the condition above for removal

Check your build, if update_socket_protocol() is no longer in the symbol
table for your vmlinux.o then the linker deleted the symbol and things
work as advertised.

If its still there, these patches have a wobbly.