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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Recursive read-to-write self-deadlock on maps rw_semaphore during lazy symbol loading
- [Medium] Spurious -ENOMEM failure in maps__load_maps() for empty map collections
--

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

perf maps: Add maps__mutate_mapping

This commit introduces maps__mutate_mapping() to provide thread-safe
modifications to map boundaries using a write lock. It also adds
maps__load_maps() to allow pre-loading of kernel maps without triggering
runtime mutations.

Fixes: 39b12f781271 ("perf tools: Make it possible to read object code from vmlinux")

> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
[ ... ]
> @@ -576,6 +576,32 @@ void maps__remove(struct maps *maps, struct map *map)
>  #endif
>  }
>  
> +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));
> +
> +	err = mutate_cb(map, data);

Does this unconditional down_write() create a recursive read-to-write
self-deadlock during lazy symbol loading?

Looking at the existing codebase, map__load() is often called while holding
the read lock. For example, in maps__for_each_map() or maps__load_first(),
a read lock is taken before iterating:

maps__for_each_map() {
    ...
    down_read(maps__lock(maps));
    ...
        cb(map, data);
    ...
}

The callback can trigger map__load() which calls dso__load_kernel_sym().
Since dso__load_kernel_sym() and dso__process_kernel_symbol() are updated
in this patch to call maps__mutate_mapping(), which acquires the write lock
on the same rw_semaphore, won't this cause a guaranteed deadlock for existing
paths that haven't been updated to use the new maps__load_maps() pre-loading
API?

[ ... ]
> @@ -626,6 +652,35 @@ int maps__for_each_map(struct maps *maps, int (*cb)(struct map *map, void *data)
>  	return ret;
>  }
>  
> +int maps__load_maps(struct maps *maps)
> +{
> +	struct map **maps_copy;
> +	unsigned int nr_maps;
> +	int err = 0;
> +
> +	if (!maps)
> +		return 0;
> +
> +	down_read(maps__lock(maps));
> +	nr_maps = maps__nr_maps(maps);
> +	maps_copy = calloc(nr_maps, sizeof(*maps_copy));
> +	if (!maps_copy) {
> +		up_read(maps__lock(maps));
> +		return -ENOMEM;
> +	}

Can this return a spurious -ENOMEM if the maps list is empty?

If nr_maps is 0, calloc(0, size) is implementation-defined and permitted to
return NULL. If it does, this error path will be taken instead of safely
returning 0 for an empty list.

Should this code explicitly check for nr_maps == 0 before attempting the
allocation?

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