Re: [PATCH v8 1/4] perf maps: Add maps__mutate_mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch introduces a guaranteed recursive read-to-write self-deadlock during lazy symbol loading by acquiring a write lock inside `maps__mutate_mapping` while the thread already holds a read lock. The provided mitigation function, `maps__load_maps()`, is effectively dead code for most tools as it is never actually invoked for them.
--

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

perf maps: Add maps__mutate_mapping

This commit introduces maps__mutate_mapping() to safely mutate map boundaries
under a write lock, resolving concurrent lookup race windows. It also adds
maps__load_maps() to pre-load kernel symbol maps upfront in order to bypass
dynamic runtime mutations during lazy symbol loading.

Fixes: 39b12f781271 ("perf tools: Make it possible to read object code from vmlinux")
Signed-off-by: Ian Rogers <irogers@google.com>

> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 923935ee21b66..7dce07e4d9b49 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.
> + */
> +int maps__mutate_mapping(struct maps *maps, struct map *map,
> +			 int (*mutate_cb)(struct map *map, void *data), void *data)
> +{
> +	int err = 0;
> +
> +	if (maps)
> +		down_write(maps__lock(maps));

[Severity: High]
Can this down_write() trigger a recursive self-deadlock for existing tools
that rely on lazy kernel symbol loading?

If tools like perf lock or perf probe have not been updated to call the new
maps__load_maps() up front, they may still trigger lazy loading while iterating
over maps.

In that scenario, a read lock is already held:

maps__for_each_map()
  down_read(maps__lock(maps));
  ...
  maps__find_symbol_by_name_cb()
    map__load()
      dso__load()
        dso__load_kernel_sym()
          maps__mutate_mapping()
            down_write(maps__lock(maps));

Since the read lock is held by the same thread across the loop, will this
down_write() cause a deadlock when lazy loading occurs?

> +
> +	err = mutate_cb(map, data);
> +
> +	if (maps) {
> +		RC_CHK_ACCESS(maps)->maps_by_address_sorted = false;
> +		RC_CHK_ACCESS(maps)->maps_by_name_sorted = false;
> +	}

[ ... ]

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