Re: [PATCH v9 18/19] perf maps: Fix lock inversion in maps__load_first() and bounds check in __maps__remove()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf maps: infinite loop in maps__remove_maps() on lookup failure
--

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

perf maps: Fix lock inversion in maps__load_first() and bounds check in __maps__remove()

This commit fixes a lock inversion in maps__load_first() by taking a reference
to the first map and releasing the read lock before calling map__load(). It
also adds bounds checks and reorders reference releases in __maps__remove().

> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index f808df2fe77b4..e947506c60a54 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <errno.h>
>  #include <stdlib.h>
> +#include <string.h>
>  #include <linux/zalloc.h>
>  #include "debug.h"
>  #include "dso.h"
> @@ -548,20 +549,27 @@ static void __maps__remove(struct maps *maps, struct map *map)
>  
>  	/* Slide later mappings over the one to remove */
>  	address_idx = maps__by_address_index(maps, map);
> -	map__put(maps_by_address[address_idx]);
> -	memmove(&maps_by_address[address_idx],
> -		&maps_by_address[address_idx + 1],
> -		(nr_maps - address_idx - 1) * sizeof(*maps_by_address));
> +	if (address_idx >= nr_maps)
> +		return;

[Severity: Medium]
Can returning early here cause an infinite loop in maps__remove_maps() while
holding the maps write lock?

In tools/perf/util/maps.c, maps__remove_maps() iterates over maps_by_address
and relies on __maps__remove() to shift subsequent entries down and decrement
nr_maps instead of incrementing i:

tools/perf/util/maps.c:maps__remove_maps() {
	...
	for (unsigned int i = 0; i < maps__nr_maps(maps);) {
		if (cb(maps_by_address[i], data)) {
			__maps__remove(maps, maps_by_address[i]);
			removed = true;
		} else {
			i++;
		}
	}
	...
}

However, in maps__by_address_index(), if maps_by_address_sorted is true, it
only checks bsearch() with map__start_cmp() and does not fall back to the
linear pointer scan if bsearch() returns NULL:

tools/perf/util/maps.c:maps__by_address_index() {
	...
	if (maps__maps_by_address_sorted(maps)) {
		struct map **mapp =
			bsearch(&map, maps__maps_by_address(maps), maps__nr_maps(maps),
				sizeof(*mapp), map__start_cmp);

		if (mapp)
			return mapp - maps_by_address;
	} else {
		for (unsigned int i = 0; i < nr_maps; i++) {
			if (RC_CHK_EQUAL(maps_by_address[i], map))
				return i;
		}
	}
	pr_err("Map missing from maps");
	return -1;
}

When a map's start or end address is updated in place without clearing
maps_by_address_sorted (for instance in machine__create_kernel_maps() where
maps__find_next_entry() sorts kmaps while vmlinux_map has end == ~0ULL before
machine__set_kernel_mmap() and maps__fixup_end() adjust map end addresses, or
in machine__create_module() and machine__process_kernel_mmap_event()),
bsearch() can fail to find maps_by_address[i] and maps__by_address_index()
returns -1 (UINT_MAX).

When dso__load_kcore() then calls maps__remove_maps(kmaps, remove_old_maps,
map), __maps__remove() will return early on address_idx >= nr_maps without
removing maps_by_address[i] or decrementing nr_maps, causing
maps__remove_maps() to repeat the same iteration i indefinitely.

Should maps__by_address_index() (and maps__by_name_index()) fall back to the
linear scan when bsearch() fails?

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