Re: [PATCH v5 3/5] perf symbols: Fix map removal sequence inside dso__process_kernel_symbol()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Incomplete implementation missing identical map mutation anti-patterns in the kallsyms loading path.
- [Medium] Out-of-bounds array access and crash on session teardown if `maps__insert()` fails.
- [Medium] Lockless window exposes missing kernel map to concurrent worker threads, causing dropped samples.
--

commit bf2ae4502d20517901689001acc83f0ec6ca94db
Author: Ian Rogers <irogers@google.com>

perf symbols: Fix map removal sequence inside dso__process_kernel_symbol()

This commit addresses a binary search indexing issue by ensuring a map is
removed from its cache list before mutating its address key fields. After the
in-place mutations are complete, the map is re-inserted to preserve the
sorting invariants of the underlying array.

> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 7afa8a1171396..f31d481a8627a 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1372,20 +1372,31 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
>  		 */
>  		if (*remap_kernel && dso__kernel(dso) && !kmodule) {
>  			*remap_kernel = false;
> -			map__set_start(map, shdr->sh_addr + ref_reloc(kmap));
> -			map__set_end(map, map__start(map) + shdr->sh_size);
> -			map__set_pgoff(map, shdr->sh_offset);
> -			map__set_mapping_type(map, MAPPING_TYPE__DSO);
> -			/* Ensure maps are correctly ordered */
> +			/*
> +			 * If the map is tracking inside the kmaps cache list array, we
> +			 * MUST remove it before mutating its virtual address key fields
> +			 * in place. Otherwise, downstream binary search lookups (bsearch)
> +			 * will search for mutated keys inside an array sorted under old
> +			 * invariants, causing indexing desynchronization faults.
> +			 */
>  			if (kmaps) {
>  				int err;
>  				struct map *tmp = map__get(map);
>  
>  				maps__remove(kmaps, map);

Does this introduce a window where the kernel map is missing from kmaps?

maps__remove() and maps__insert() each independently acquire and release the
kmaps write lock. During the gap between these operations, the kernel map is
missing from the collection.

For KVM guests, kernel symbols are loaded lazily when the first guest sample
is received. If concurrent perf top worker threads process samples during
this window, will their maps__find() queries fail and cause samples to be
dropped or misattributed as unknown?

> +				map__set_start(map, shdr->sh_addr + ref_reloc(kmap));
> +				map__set_end(map, map__start(map) + shdr->sh_size);
> +				map__set_pgoff(map, shdr->sh_offset);
> +				map__set_mapping_type(map, MAPPING_TYPE__DSO);
>  				err = maps__insert(kmaps, map);

If maps__insert() fails here (for example, due to an ENOMEM when reallocating
the array), is the kernel map left permanently detached from kmaps?

If it remains detached but is still referenced by machine->vmlinux_map, what
happens during session teardown? It looks like machine__destroy_kernel_maps()
unconditionally calls maps__remove().

Inside __maps__remove(), if maps__by_address_index() fails to find the map
and returns -1 (UINT_MAX), it seems the code will use this index to call
map__put(maps_by_address[UINT_MAX]) and perform a memmove(). Could this
trigger an out-of-bounds memory access and crash?

>  				map__put(tmp);
>  				if (err)
>  					return err;
> +			} else {
> +				map__set_start(map, shdr->sh_addr + ref_reloc(kmap));
> +				map__set_end(map, map__start(map) + shdr->sh_size);
> +				map__set_pgoff(map, shdr->sh_offset);
> +				map__set_mapping_type(map, MAPPING_TYPE__DSO);
>  			}
>  		}

Also, this commit fixes the in-place map boundary mutations for ELF symbols,
but does it miss similar patterns in the kallsyms loading path?

In dso__load_kernel_sym() and dso__load_guest_kernel_sym(), it appears
map__fixup_start(map) directly mutates map->start in place after the map is
already inserted into the kmaps array. Will this lockless mutation break the
sorting invariants of the maps_by_address array in the same way, causing
bsearch lookups to fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260506004546.3140141-1-irogers@google.com?part=3