Re: [PATCH v2] perf: Extract is_mapping_symbol() helper for kernel mapping symbol filtering

On 5/6/26 6:43 PM, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?
> 

I believe this is not a bug. The is_mapping_symbol() helper is
explicitly
documented as kernel-only:



  /*

   * Ignore kernel mapping symbols, matching kernel is_mapping_symbol()
logic.
   * ...

   * Only use this for kernel symbols (kallsyms, ksymbol events).

   */



The dso__load_sym_internal() function handles both kernel and userspace

symbols. Applying is_mapping_symbol() there would incorrectly filter out

userspace .L* and L0* symbols.



However, I noticed that kernel modules could also benefit from filtering
x86 .L* symbols. I've added the following in dso__load_sym_internal() to
handle this case:



  /*

   * 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;



This ensures:



1. Only kernel modules are filtered (kmodule == true)

2. Userspace symbols are not affected

3. The existing is_mapping_symbol() helper is reused


>> 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?
> 

You are absolutely right. The is_mapping_symbol() check is missing in

machine__process_ksymbol_unregister(). This could indeed cause the
unregister path to incorrectly remove a parent map when a mapping symbol
address falls within an existing map range.



I will fix this by adding the same filter to
machine__process_ksymbol_unregister():



static int machine__process_ksymbol_unregister(struct machine *machine,
union perf_event *event,struct perf_sample *sample __maybe_unused)
{

      struct symbol *sym;

      struct map *map;


      /* Ignore mapping symbols in ksymbol events */
      if (is_mapping_symbol(event->ksymbol.name))
          return 0;

      map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
      ...
}


I will send a v3 with these fixes.



Thanks,

Rui Qi