Re: [PATCH v3] perf: Add is_mapping_symbol() helper for kernel mapping symbol filtering

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The commit message claims to replace ad-hoc mapping symbol filtering in `dso__load_sym_internal()` with a single helper, but the existing architecture-specific checks were not removed, resulting in redundant code and a code/commit message mismatch.
- [Medium] The new mapping symbol filter in `dso__load_sym_internal()` is strictly gated behind `kmodule`, which incorrectly allows mapping symbols from the main kernel image (`vmlinux`) to bypass the filter.
--

commit b0c39fbcfd41cb96be112e1c6d3979a91d527b75
Author: Rui Qi <qirui.001@bytedance.com>

perf: Add is_mapping_symbol() helper for kernel mapping symbol filtering

This commit introduces a unified is_mapping_symbol() inline helper to
consolidate and replace ad-hoc mapping symbol filtering for ARM, AArch64,
and RISC-V across the perf tool, while adding x86 local symbol filtering.

> perf: Add is_mapping_symbol() helper for kernel mapping symbol filtering
>
> The perf tool currently has ad-hoc logic to filter out ELF mapping
> symbols scattered across multiple files. ARM, AArch64 and RISC-V each
> have their own inline checks in dso__load_sym_internal(), and kallsym
> processing has yet another check for ARM module symbols.
> 
> This is fragile: adding support for a new architecture or adjusting
> which prefixes are considered mapping symbols requires touching
> multiple places, and it is easy for the checks to diverge. It also
> does not match the kernel's own is_mapping_symbol() logic, which
> additionally covers x86 local symbols (".L*" and "L0*").
> 
> Introduce a single is_mapping_symbol() inline helper in symbol.h and
> convert all kernel symbol handling to use it.

The commit message states we are replacing the inline checks in
dso__load_sym_internal() and converting all kernel symbol handling to use
the new helper.

However, looking at the resulting code in dso__load_sym_internal(), the old
checks are still present:

		if (ehdr.e_machine == EM_ARM || ehdr.e_machine == EM_AARCH64) {
			if (elf_name[0] == '$' && strchr("adtx", elf_name[1])
			    && (elf_name[2] == '\0' || elf_name[2] == '.'))
				continue;
		}

		if (ehdr.e_machine == EM_RISCV) {
			if (elf_name[0] == '$' && strchr("dx", elf_name[1]))
				continue;
		}

Should these existing ARM, AArch64, and RISC-V inline checks have been
removed to avoid redundant filtering, or was the intention to only apply
the new helper to kernel modules while leaving the old checks for user-space
binaries?

> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 7afa8a1171396..6b12508ea58dc 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
[ ... ]
> @@ -1607,6 +1607,14 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>  				continue;
>  		}
>  
> +		/*
> +		 * For kernel modules, also reject x86 local symbols (.L* and L0*)
> +		 * to match the kernel's is_mapping_symbol() logic and kallsyms
> +		 * parsing behavior.
> +		 */
> +		if (kmodule && is_mapping_symbol(elf_name))
> +			continue;
> +

When perf loads the main kernel image via dso__load_vmlinux(), it calls
dso__load_sym() with kmodule = 0.

Does gating this check behind kmodule inadvertently bypass the new mapping
symbol logic for vmlinux?

If so, x86 local symbols might leak into the symbol tables for the main
kernel image. Would using dso__kernel(dso) instead of kmodule correctly
cover both vmlinux and kernel modules?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507071103.2772577-1-qirui.001@bytedance.com?part=1