Re: [PATCH v2] perf: Extract 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:
- [High] Asymmetric mapping symbol filtering leads to map/symbol deletion during ksymbol unregistration.
- [Medium] Incomplete refactoring leaves ELF mapping symbol logic fragile and inconsistent.
--

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

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

This patch introduces an is_mapping_symbol() inline helper to consolidate
logic for filtering out ELF mapping symbols across architectures. It also
adds the helper to machine__process_ksymbol_register() and
map__process_kallsym_symbol().

> The perf tool currently has ad-hoc logic to filter out ELF mapping
> symbols scattered across multiple files and architectures.  ARM,
> AArch64 and RISC-V each have their own inline checks in
> dso__load_sym_internal(), and kallsym processing in map__process_kallsym_symbol()
> 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*').

This commit message claims to convert dso__load_sym_internal() to use the
new helper. However, this patch does not modify dso__load_sym_internal()
in tools/perf/util/symbol-elf.c at all, leaving the scattered inline checks
untouched.

Because x86 local symbols (.L*) were added to the new helper, does this
omission introduce a new inconsistency where kallsyms parsing will correctly
filter x86 .L* symbols, but kernel module parsing via ELF (.ko) will not?

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index e76f8c86e62a6..e0dcf8bfb896e 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -729,9 +729,15 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  {
>  	struct symbol *sym;
>  	struct dso *dso = NULL;
> -	struct map *map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
> +	struct map *map;
>  	int err = 0;
>  
> +	/* Ignore mapping symbols in ksymbol events - check early before any state mutation */
> +	if (is_mapping_symbol(event->ksymbol.name))
> +		return 0;

If a PERF_RECORD_KSYMBOL event arrives with the UNREGISTER flag,
machine__process_ksymbol() routes it to machine__process_ksymbol_unregister().

Because machine__process_ksymbol_unregister() does not have the same
is_mapping_symbol() filter, could maps__find() return the enclosing map
(such as the BPF JIT map or a loaded kernel module's map) that covers the
mapping symbol's address?

tools/perf/util/machine.c:machine__process_ksymbol_unregister() {
...
	map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
	if (!map)
		return 0;

	if (!RC_CHK_EQUAL(map, machine->vmlinux_map))
		maps__remove(machine__kernel_maps(machine), map);
...
}

If so, will the unregister path incorrectly call maps__remove() on that
parent map, prematurely destroying legitimate tracking state and breaking
symbol resolution for subsequent samples in that memory region?

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