Re: [PATCH v18 1/5] perf maps: Add maps__mutate_mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The patch fails to add the mandatory `maps__load_maps()` pre-loading call to `perf inject`, violating its own architectural contract and leaving the tool vulnerable to deadlocks.
- [High] `maps__load_first` triggers a guaranteed read-to-write self-deadlock due to the new locking rules.
- [High] Lockless reads of map boundary fields (`start`, `end`, `pgoff`) in `maps__find_symbol_by_name` and `maps__find_symbol` cause TOCTOU/inconsistent reads.
--

commit b899514520f2642fd2ff0bdc70f3cfeac8916515
Author: Ian Rogers <irogers@google.com>
Subject: perf maps: Add maps__mutate_mapping

This patch introduces maps__mutate_mapping() to safely update map address
boundaries under a write lock, fixing a race condition with concurrent map
lookups. It also introduces maps__load_maps() to pre-load kernel maps and
updates symbol parsing paths to use the new synchronized mutator.

>     lock. Callers (such as perf inject) must pre-load all kernel symbol
>     maps up front at startup using maps__load_maps(), completely bypassing
>     dynamic runtime mutations.
>     
>     Fixes: 39b12f781271 ("perf tools: Make it possible to read object code from vmlinux")
>     
>     Signed-off-by: Ian Rogers <irogers@google.com>
>     Assisted-by: Antigravity:gemini-3.1-pro

[Severity: High]
The commit message explicitly mandates that perf inject must pre-load maps
up front to avoid deadlocks. Does tools/perf/builtin-inject.c need to be
updated in this patch to include the maps__load_maps() call?

Without it, perf inject might remain exposed to the recursive read-to-write
self-deadlock during lazy symbol loading.

> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 923935ee21b66..b1b8efe421496 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
[ ... ]
> @@ -576,6 +576,49 @@ void maps__remove(struct maps *maps, struct map *map)
>  #endif
>  }
>  
> +/**
> + * maps__mutate_mapping - Apply write-protected mutations to a map.
> + * @maps: The maps collection containing the map.
> + * @map: The map to mutate.
> + * @mutate_cb: Callback function that performs the actual mutations.
> + * @data: Private data passed to the callback.
> + *
> + * This acquires the write lock on the maps semaphore to safely protect
> + * concurrent readers from seeing partially mutated or unsorted map boundaries.
> + *
> + * WARNING: Acquiring down_write() here can trigger a recursive self-deadlock if
> + * the caller already holds the read lock (e.g., during maps__for_each_map() or
> + * maps__find() iteration paths that trigger lazy symbol loading). To completely
> + * avoid this deadlock, all kernel/module maps must be pre-loaded up-front (via
> + * maps__load_maps()) under a clean, single-threaded context before entering
> + * multi-threaded event processing loops.
> + */

[Severity: High]
Does the newly introduced locking contract cause a read-to-write self-deadlock
in the existing maps__load_first() API?

If another subsystem calls maps__load_first(), it acquires the read lock:

maps__load_first()
   down_read(maps__lock(maps));
   map__load(...)

If map__load() triggers lazy symbol loading, won't it eventually call
maps__mutate_mapping() which unconditionally takes the write lock via
down_write(maps__lock(maps)), deadlocking the thread?

[ ... ]
> +struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name, struct map **mapp)
>  {
> +	struct map **maps_copy;
> +	unsigned int nr_maps;
> +	struct symbol *sym = NULL;
>  
> +	if (!maps)
> +		return NULL;
>  
> +	/*
> +	 * First, ensure all maps are loaded. We pre-load them outside of any
> +	 * read-to-write locks to avoid deadlocks. Even if some fail, we proceed.
> +	 */
> +	maps__load_maps(maps);
> +
> +	/*
> +	 * Create a local snapshot of the maps while holding the read lock.
> +	 * This prevents deadlocking if iteration triggers further map insertions.
> +	 */
> +	down_read(maps__lock(maps));
> +	nr_maps = maps__nr_maps(maps);
> +	maps_copy = calloc(nr_maps, sizeof(*maps_copy));
> +	if (maps_copy) {
> +		for (unsigned int i = 0; i < nr_maps; i++) {
> +			struct map *map = maps__maps_by_address(maps)[i];
> +
> +			maps_copy[i] = map__get(map);
> +		}
>  	}
> +	up_read(maps__lock(maps));
>  
> +	if (!maps_copy)
> +		return NULL;
>  
> +	for (unsigned int i = 0; i < nr_maps; i++) {
> +		struct map *map = maps_copy[i];
> +
> +		sym = map__find_symbol_by_name(map, name);
> +		if (sym && map__contains_symbol(map, sym)) {

[Severity: High]
Since the maps->lock read lock was dropped prior to this loop, is it safe to
call map__contains_symbol() here?

map__contains_symbol() accesses map->start and map->end. Concurrently, a
thread processing an mmap event could invoke maps__mutate_mapping(), which
mutates these exact boundary fields while holding the maps->lock write lock.
Will this cause a data race resulting in torn boundary reads?

The same lockless read pattern appears to exist in maps__find_symbol():

maps__find_symbol()
   ...
   if (map != NULL && map__load(map) >= 0)
       result = map__find_symbol(map, map__map_ip(map, addr));

where map__map_ip() accesses map->start and map->pgoff after the read
lock was dropped by maps__find().

> +			if (mapp)
> +				*mapp = map__get(map);
> +			break;
> +		}
> +		sym = NULL;
> +	}
> +
> +	for (unsigned int i = 0; i < nr_maps; i++)
> +		map__put(maps_copy[i]);
>  
> -	maps__for_each_map(maps, maps__find_symbol_by_name_cb, &args);
> -	return args.sym;
> +	free(maps_copy);
> +	return sym;
>  }

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