Re: [PATCH v6 3/6] perf maps: Add maps__mutate_mapping

[James Clark <james.clark@linaro.org>]

On 11/05/2026 8:07 am, Namhyung Kim wrote:
> On Fri, May 08, 2026 at 01:27:23AM -0700, Ian Rogers wrote:
>> During kernel ELF symbol parsing (dso__process_kernel_symbol), proc
>> kallsyms image loading (dso__load_kernel_sym,
>> dso__load_guest_kernel_sym), and dynamic kernel memory map alignment
>> updates (machine__update_kernel_mmap), the loader directly modifies
>> live virtual address boundary keys fields on map objects.  If these
>> boundaries are mutated while the map pointer actively resides inside
>> the parent maps cache array list (kmaps) outside of any lock closure,
>> an unsafe concurrent window is exposed where parallel worker lookup
>> threads (e.g., inside perf top) can mistakenly assume the cache
>> remains sorted based on stale parameters, executing binary search
>> queries (bsearch) across an unsorted range and triggering lookups
>> failure.
>>
>> Fix this by introducing a thread-safe, atomic transactional framework
>> routine maps__mutate_mapping() that explicitly acquires the parent
>> maps write semaphore lock, executes an incoming mutation callback
>> block to perform the field updates under full lock protection, and
>> invalidates the sorted tracking flags prior to releasing the write
>> lock. This guarantees absolute atomic synchronization invariants,
>> completely closing the concurrent lookup race window. The adjacent
>> module alignment pass inside machine__create_kernel_maps() is safely
>> preserved as a high-performance lockless pass, as its invocation
>> lifecycle bounds remain strictly single-threaded by contract during
>> session initialization construction. There is a potential for self
>> deadlock if maps__mutate_mapping is called with the lock held, such as
>> with maps__for_each_map but this problem also existed with the
>> previous remove and insert approaches.
>>
>> Fixes: 39b12f781271 ("perf tools: Make it possible to read object code from vmlinux")
>> Signed-off-by: Ian Rogers <irogers@google.com>
>> ---
>>   tools/perf/util/machine.c    | 32 +++++++++++++++++-----------
>>   tools/perf/util/maps.c       | 26 +++++++++++++++++++++++
>>   tools/perf/util/maps.h       |  2 ++
>>   tools/perf/util/symbol-elf.c | 41 +++++++++++++++++++++++-------------
>>   tools/perf/util/symbol.c     | 17 +++++++++++----
>>   5 files changed, 87 insertions(+), 31 deletions(-)
>>
>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> index e76f8c86e62a..8d4452c70cb5 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1522,22 +1522,30 @@ static void machine__set_kernel_mmap(struct machine *machine,
>>   		map__set_end(machine->vmlinux_map, ~0ULL);
>>   }
>>   
>> -static int machine__update_kernel_mmap(struct machine *machine,
>> -				     u64 start, u64 end)
>> +struct kernel_mmap_mutation_ctx {
>> +	u64 start;
>> +	u64 end;
>> +};
>> +
>> +static int kernel_mmap_mutate_cb(struct map *map, void *data)
>>   {
>> -	struct map *orig, *updated;
>> -	int err;
>> +	struct kernel_mmap_mutation_ctx *ctx = data;
>>   
>> -	orig = machine->vmlinux_map;
>> -	updated = map__get(orig);
>> +	map__set_start(map, ctx->start);
>> +	map__set_end(map, ctx->end);
>> +	if (ctx->start == 0 && ctx->end == 0)
>> +		map__set_end(map, ~0ULL);
>> +	return 0;
>> +}
>>   
>> -	machine->vmlinux_map = updated;
>> -	maps__remove(machine__kernel_maps(machine), orig);
>> -	machine__set_kernel_mmap(machine, start, end);
>> -	err = maps__insert(machine__kernel_maps(machine), updated);
>> -	map__put(orig);
>> +static int machine__update_kernel_mmap(struct machine *machine,
>> +				     u64 start, u64 end)
>> +{
>> +	struct kernel_mmap_mutation_ctx ctx = { .start = start, .end = end };
>>   
>> -	return err;
>> +	return maps__mutate_mapping(machine__kernel_maps(machine),
>> +				     machine->vmlinux_map,
>> +				     kernel_mmap_mutate_cb, &ctx);
>>   }
>>   
>>   int machine__create_kernel_maps(struct machine *machine)
>> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
>> index 81a97ac34077..91345a773aa2 100644
>> --- a/tools/perf/util/maps.c
>> +++ b/tools/perf/util/maps.c
>> @@ -575,6 +575,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);
>> +
>> +	if (maps) {
>> +		RC_CHK_ACCESS(maps)->maps_by_address_sorted = false;
>> +		RC_CHK_ACCESS(maps)->maps_by_name_sorted = false;
>> +	}
>> +
>> +	if (maps)
>> +		up_write(maps__lock(maps));
>> +
>> +#ifdef HAVE_LIBDW_SUPPORT
>> +	if (maps)
>> +		libdw__invalidate_dwfl(maps, maps__libdw_addr_space_dwfl(maps));
>> +#endif
>> +
>> +	return err;
>> +}
> 
> Could be simplified by checking 'maps' once.  But I'm not sure if
> there's a case it doesn't have the maps.
> 
> Thanks,
> Namhyung
> 

Hi Ian,

The multiple maps checks after mutate_cb() still seem to be in V19.

I noticed because I still get the -Wthread-safety-analysis error on x86 
clang 15. Not sure why the compiler isn't able to see that maps doesn't 
change, and no amount of re-arranging made it go away. The only way to 
fix it is to have the lock and unlock in the same block, but I don't 
think it looks bad:

	if (maps) {
		down_write(maps__lock(maps));

		err = mutate_cb(map, data);

		RC_CHK_ACCESS(maps)->maps_by_address_sorted = false;
		RC_CHK_ACCESS(maps)->maps_by_name_sorted = false;

		up_write(maps__lock(maps));
	} else {
		err = mutate_cb(map, data);
	}

Thanks
James


>> +
>>   bool maps__empty(struct maps *maps)
>>   {
>>   	bool res;
>> diff --git a/tools/perf/util/maps.h b/tools/perf/util/maps.h
>> index 20c52084ba9e..de74ccbb8a12 100644
>> --- a/tools/perf/util/maps.h
>> +++ b/tools/perf/util/maps.h
>> @@ -61,6 +61,8 @@ size_t maps__fprintf(struct maps *maps, FILE *fp);
>>   
>>   int maps__insert(struct maps *maps, struct map *map);
>>   void maps__remove(struct maps *maps, struct map *map);
>> +int maps__mutate_mapping(struct maps *maps, struct map *map,
>> +			 int (*mutate_cb)(struct map *map, void *data), void *data);
>>   
>>   struct map *maps__find(struct maps *maps, u64 addr);
>>   struct symbol *maps__find_symbol(struct maps *maps, u64 addr, struct map **mapp);
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 7afa8a117139..dc4ab58857b3 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -1341,6 +1341,24 @@ static u64 ref_reloc(struct kmap *kmap)
>>   void __weak arch__sym_update(struct symbol *s __maybe_unused,
>>   		GElf_Sym *sym __maybe_unused) { }
>>   
>> +struct remap_kernel_ctx {
>> +	u64 sh_addr;
>> +	u64 sh_size;
>> +	u64 sh_offset;
>> +	struct kmap *kmap;
>> +};
>> +
>> +static int remap_kernel_cb(struct map *map, void *data)
>> +{
>> +	struct remap_kernel_ctx *ctx = data;
>> +
>> +	map__set_start(map, ctx->sh_addr + ref_reloc(ctx->kmap));
>> +	map__set_end(map, map__start(map) + ctx->sh_size);
>> +	map__set_pgoff(map, ctx->sh_offset);
>> +	map__set_mapping_type(map, MAPPING_TYPE__DSO);
>> +	return 0;
>> +}
>> +
>>   static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
>>   				      GElf_Sym *sym, GElf_Shdr *shdr,
>>   				      struct maps *kmaps, struct kmap *kmap,
>> @@ -1371,22 +1389,15 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
>>   		 * map to the kernel dso.
>>   		 */
>>   		if (*remap_kernel && dso__kernel(dso) && !kmodule) {
>> +			struct remap_kernel_ctx ctx = {
>> +				.sh_addr = shdr->sh_addr,
>> +				.sh_size = shdr->sh_size,
>> +				.sh_offset = shdr->sh_offset,
>> +				.kmap = kmap
>> +			};
>> +
>>   			*remap_kernel = false;
>> -			map__set_start(map, shdr->sh_addr + ref_reloc(kmap));
>> -			map__set_end(map, map__start(map) + shdr->sh_size);
>> -			map__set_pgoff(map, shdr->sh_offset);
>> -			map__set_mapping_type(map, MAPPING_TYPE__DSO);
>> -			/* Ensure maps are correctly ordered */
>> -			if (kmaps) {
>> -				int err;
>> -				struct map *tmp = map__get(map);
>> -
>> -				maps__remove(kmaps, map);
>> -				err = maps__insert(kmaps, map);
>> -				map__put(tmp);
>> -				if (err)
>> -					return err;
>> -			}
>> +			maps__mutate_mapping(kmaps, map, remap_kernel_cb, &ctx);
>>   		}
>>   
>>   		/*
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index fcaeeddbbb6b..09b93e844887 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -48,6 +48,13 @@
>>   #include <symbol/kallsyms.h>
>>   #include <sys/utsname.h>
>>   
>> +static int map_fixup_cb(struct map *map, void *data __maybe_unused)
>> +{
>> +	map__fixup_start(map);
>> +	map__fixup_end(map);
>> +	return 0;
>> +}
>> +
>>   static int dso__load_kernel_sym(struct dso *dso, struct map *map);
>>   static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map);
>>   static bool symbol__is_idle(const char *name);
>> @@ -2121,10 +2128,11 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
>>   	free(kallsyms_allocated_filename);
>>   
>>   	if (err > 0 && !dso__is_kcore(dso)) {
>> +		struct maps *kmaps = map__kmaps(map);
>> +
>>   		dso__set_binary_type(dso, DSO_BINARY_TYPE__KALLSYMS);
>>   		dso__set_long_name(dso, DSO__NAME_KALLSYMS, false);
>> -		map__fixup_start(map);
>> -		map__fixup_end(map);
>> +		maps__mutate_mapping(kmaps, map, map_fixup_cb, NULL);
>>   	}
>>   
>>   	return err;
>> @@ -2164,10 +2172,11 @@ static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map)
>>   	if (err > 0)
>>   		pr_debug("Using %s for symbols\n", kallsyms_filename);
>>   	if (err > 0 && !dso__is_kcore(dso)) {
>> +		struct maps *kmaps = map__kmaps(map);
>> +
>>   		dso__set_binary_type(dso, DSO_BINARY_TYPE__GUEST_KALLSYMS);
>>   		dso__set_long_name(dso, machine->mmap_name, false);
>> -		map__fixup_start(map);
>> -		map__fixup_end(map);
>> +		maps__mutate_mapping(kmaps, map, map_fixup_cb, NULL);
>>   	}
>>   
>>   	return err;
>> -- 
>> 2.54.0.563.g4f69b47b94-goog
>>