Re: [PATCH v2 2/2] perf maps: Fix fixup_overlap_and_insert that can break sorted by name order

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] The maps__copy_from function corrupts the maps_by_name array by populating it in address order but flagging it as sorted by name.

--

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

perf maps: Fix fixup_overlap_and_insert that can break sorted by name order

When an entry in the address array is replaced, the corresponding name
entry is replaced. The entries names may sort differently and so it is
important that the sorted by name property be cleared on the maps.

> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 7dd6da9d1e4f0..b44bc41f51f3b 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
[ ... ]
> @@ -955,6 +955,7 @@ static int __maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
>  			if (maps_by_name) {
>  				map__put(maps_by_name[ni]);
>  				maps_by_name[ni] = map__get(new);
> +				maps__set_maps_by_name_sorted(maps, false);
>  			}
[ ... ]

This isn't a bug in the newly added lines, but while reviewing the maps
sorting logic, I noticed a regression in maps__copy_from().

In maps__copy_from(), when copying maps from a parent to a destination
(e.g., during thread fork), the dest_maps_by_name array is populated
sequentially inside a loop iterating over parent_maps_by_address:

tools/perf/util/maps.c:maps__copy_from() {
[ ... ]
		for (unsigned int i = 0; !err && i < n; i++) {
			struct map *pos = parent_maps_by_address[i];
			struct map *new = map__clone(pos);
[ ... ]
				if (!err) {
					dest_maps_by_address[i] = new;
					map__set_kmap_maps(new, dest);
					if (dest_maps_by_name)
						dest_maps_by_name[i] = map__get(new);
[ ... ]
}

Because it clones and copies the elements in address order,
dest_maps_by_name is strictly populated in address order.

However, at the end of the fast-path copy block, the code executes:

tools/perf/util/maps.c:maps__copy_from() {
[ ... ]
		if (!err) {
			RC_CHK_ACCESS(dest)->last_search_by_name_idx =
				RC_CHK_ACCESS(parent)->last_search_by_name_idx;
			maps__set_maps_by_name_sorted(dest,
						dest_maps_by_name &&
						maps__maps_by_name_sorted(parent));
		}
[ ... ]
}

If the parent's name array was sorted, does this explicitly flag the new
destination map's name array as sorted by name, even though it is physically
sorted by address?

Could this cause any subsequent lookup by name in the copied map
(such as maps__find_by_name during child thread symbol resolution)
to execute a binary search on an address-sorted array?

Would this cause the binary search to take incorrect paths and fail to find
legitimate mappings, leading to missing symbols or missing maps for
child threads?

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