Re: [PATCH v10 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] Data race and logical corruption by modifying map sorting flags under a read lock on the memory allocation error path in `maps__load_maps()`.
- [High] A single module load failure in `maps__load_maps()` causes `maps__find_symbol_by_name()` to abort the entire symbol lookup.
- [High] The lockless map pre-loading exposes a TOCTOU race that still leads to the read-to-write recursive self-deadlock.
--

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

perf maps: Add maps__mutate_mapping

This patch introduces maps__mutate_mapping() to apply map boundary mutations
under a write lock to fix a concurrent lookup race. It also introduces
maps__load_maps() to pre-load modules and bypass dynamic runtime mutations
to prevent read-to-write self-deadlocks.

[ ... ]

> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 923935ee21b66..be77dee16041b 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c

[ ... ]

> @@ -626,6 +669,41 @@ 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);
> +	if (nr_maps == 0) {
> +		up_read(maps__lock(maps));
> +		return 0;
> +	}
> +	maps_copy = calloc(nr_maps, sizeof(*maps_copy));
> +	if (!maps_copy) {
> +		RC_CHK_ACCESS(maps)->maps_by_address_sorted = false;
> +		RC_CHK_ACCESS(maps)->maps_by_name_sorted = false;

[Severity: High]
Does this code create a data race by modifying shared state under a read
lock?

We only hold the down_read() semaphore here, but we are writing to the
shared boolean flags.

Also, since the memory allocation failed and no maps were actually mutated,
is it necessary to invalidate the sorted state? This seems like it will
unnecessarily force future readers to acquire a write lock to re-sort an
already sorted array.

> +		up_read(maps__lock(maps));
> +		return -ENOMEM;
> +	}
> +	for (unsigned int i = 0; i < nr_maps; i++)
> +		maps_copy[i] = map__get(maps__maps_by_address(maps)[i]);
> +	up_read(maps__lock(maps));
> +
> +	for (unsigned int i = 0; i < nr_maps; i++) {
> +		if (map__load(maps_copy[i]) < 0)
> +			err = -1;
> +		map__put(maps_copy[i]);
> +	}
> +	free(maps_copy);
> +	return err;
> +}

[ ... ]

> @@ -700,6 +778,8 @@ struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name, st
>  		.sym = NULL,
>  	};
>  
> +	if (maps__load_maps(maps) < 0)
> +		return NULL;

[Severity: High]
Will a single module load failure cause this to abort the entire symbol lookup?

If maps__load_maps() encounters a single failure (for example, due to a missing
kernel module debuginfo), it propagates an error of -1.

By returning NULL immediately here, don't we bypass searching all the
successfully loaded modules? Previously, individual map load failures
were silently skipped, allowing the search to gracefully continue.


[Severity: High]
Can a concurrent insertion defeat this lockless pre-loading and still trigger
the recursive self-deadlock?

maps__load_maps() drops the read lock after copying the map pointers. What
happens if a concurrent thread inserts a new, unloaded kernel map right
after maps__load_maps() returns, but before maps__for_each_map() executes?

It looks like maps__for_each_map() would then iterate over the newly inserted
map, invoke map__load() while holding the down_read() lock, and trigger
maps__mutate_mapping().

Won't maps__mutate_mapping() then unconditionally attempt to acquire the
down_write() lock, triggering the exact read-to-write self-deadlock this
patch intends to fix?

>  	maps__for_each_map(maps, maps__find_symbol_by_name_cb, &args);
>  	return args.sym;
>  }

[ ... ]

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