Re: [PATCH v6 10/10] perf annotate: Be robust to annotating without a thread

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional call to `map__kmaps()` on user-space maps triggers internal error spam.
- [High] Unbalanced `map__put()` on the selected `map` when zooming out via ESC/LEFT leads to Use-After-Free.
- [Medium] `map_symbol__get_arch` returns negative POSIX error code, breaking its caller's error formatting.
--

--- Patch [10]: [PATCH v6 10/10] perf annotate: Be robust to annotating without a thread ---
commit 6b7b7896027a6d84296eafef08984d82adf8db72
Author: Ian Rogers <irogers@google.com>

perf annotate: Be robust to annotating without a thread

If a thread isn't given to map_symbol__get_arch try harder to determine
the arch for disassembly. Do this by using fallback paths such as
reading the e_machine from a map's dso.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index e0ef756d95367..aee3d2c4e1867 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
[ ... ]
> @@ -3347,8 +3347,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  				 */
>  				do_zoom_dso(browser, actions);
>  			} else if (top == &browser->hists->thread_filter) {
> -				actions->thread = thread;
> -				do_zoom_thread(browser, actions);
> +				memset(&hotkey_act, 0, sizeof(hotkey_act));
> +				hotkey_act.ms.thread = thread__get(thread);
> +				do_zoom_thread(browser, &hotkey_act);
> +				map_symbol__exit(&hotkey_act.ms);

[Severity: High]
Does the migration to use map_symbol in popup_action elements introduce a
use-after-free when zooming out?

In evsel__hists_browse(), when handling K_ESC or K_LEFT to zoom out, the
code assigns the map pointer directly:

evsel__hists_browse() {
    ...
    case K_ESC:
    case K_LEFT: {
        ...
        actions->ms.map = map;
        top = pstack__peek(browser->pstack);
        ...
        continue;
}

This assigns the raw map pointer without claiming a reference via map__get().

When the loop continues, the top of the loop unconditionally executes
free_popup_actions(actions, MAX_OPTIONS) to clean up the array. Since the
struct now embeds a map_symbol, this calls map_symbol__exit() which will
execute map__zput() on the un-refcounted map.

Could this unbalanced put prematurely free the map while it is still
referenced elsewhere?

>  			} else if (top == &browser->hists->socket_filter) {
>  				do_zoom_socket(browser, actions);
>  			}
[ ... ]
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 02505222d8c2d..deb92c27f7abf 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
[ ... ]
> @@ -982,24 +982,40 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel)
>  	annotation__calc_percent(notes, evsel, symbol__size(sym));
>  }
>  
> -int thread__get_arch(struct thread *thread, const struct arch **parch)
> +
> +
> +int map_symbol__get_arch(struct map_symbol *ms, const struct arch **parch)
>  {
>  	const struct arch *arch;
> -	struct machine *machine;
> -	uint32_t e_flags;
> -	uint16_t e_machine;
> +	struct machine *machine = NULL;
> +	struct map *map = ms->map;
> +	struct dso *dso = map ? map__dso(map) : NULL;
> +	uint32_t e_flags = 0;
> +	uint16_t e_machine = EM_NONE;
>  
> -	if (!thread) {
> -		*parch = NULL;
> -		return -1;
> +	const char *cpuid = NULL;
> +
> +	if (ms->thread) {
> +		machine = maps__machine(thread__maps(ms->thread));
> +		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
> +		if (machine && machine->env)
> +			cpuid = machine->env->cpuid;
> +	} else if (dso) {
> +		struct maps *kmaps = map ? map__kmaps(map) : NULL;

[Severity: High]
Does this call to map__kmaps() verify if the map is actually a kernel map
first?

If map__kmaps() is invoked on a user-space map, it forcefully prints an
internal error and returns NULL. Since perf annotate might process many
user-space symbols, could this result in continuous console spam that
obscures real output and breaks the TUI layout?

Should this check dso__kernel(dso) before attempting to extract the kernel
maps structure?

> +		struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> +		e_machine = dso__e_machine(dso, kmap_machine, &e_flags);
> +		if (kmap_machine && kmap_machine->env)
> +			cpuid = kmap_machine->env->cpuid;
>  	}
>  
> -	machine = maps__machine(thread__maps(thread));
> -	e_machine = thread__e_machine(thread, machine, &e_flags);
> -	arch = arch__find(e_machine, e_flags, machine->env ? machine->env->cpuid : NULL);
> +	if (e_machine == EM_NONE)
> +		e_machine = thread__e_machine(NULL, NULL, &e_flags);
> +
> +	arch = arch__find(e_machine, e_flags, cpuid);
>  	if (arch == NULL) {
>  		pr_err("%s: unsupported arch %d\n", __func__, e_machine);
> -		return errno;
> +		return errno ? errno : -ENOTSUP;

[Severity: Medium]
Is returning a negative POSIX error code (-ENOTSUP) expected by the caller?

The caller symbol__strerror_disassemble() seems to expect POSIX system error
codes to be passed as positive integers to correctly translate them with
str_error_r(). It uses negative integers exclusively for custom
SYMBOL_ANNOTATE_ERRNO__* constants.

If we return -95 here, does the switch statement in the error formatter
default to printing an invalid error code message instead of the real
unsupported architecture error?

>  	}
>  	if (parch)
>  		*parch = arch;
[ ... ]
> diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
> index 5ad537fea4360..b4903f351bac0 100644
> --- a/tools/perf/util/capstone.c
> +++ b/tools/perf/util/capstone.c
[ ... ]
> @@ -411,9 +411,22 @@ int symbol__disassemble_capstone(const char *filename, struct symbol *sym,
>  	    !strcmp(args->options->disassembler_style, "att"))
>  		disassembler_style = true;
>  
> -	e_machine = thread__e_machine_endian(args->ms->thread,
> -					     /*machine=*/NULL,
> -					     /*e_flags=*/NULL, &is_big_endian);
> +	if (args->ms->thread) {
> +		e_machine = thread__e_machine_endian(args->ms->thread,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);
> +	} else if (dso) {
> +		struct maps *kmaps = map ? map__kmaps(map) : NULL;

[Severity: High]
Similarly to map_symbol__get_arch(), does this unconditionally invoke
map__kmaps() on what could be a user-space map, triggering the same console
spam issue?

> +		struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> +		e_machine = dso__e_machine_endian(dso, kmap_machine, /*e_flags=*/NULL,
> +						  &is_big_endian);
> +	}
[ ... ]
> @@ -526,9 +539,22 @@ int symbol__disassemble_capstone_powerpc(const char *filename __maybe_unused,
>  	    !strcmp(args->options->disassembler_style, "att"))
>  		disassembler_style = true;
>  
> -	e_machine = thread__e_machine_endian(args->ms->thread,
> -					     /*machine=*/NULL,
> -					     /*e_flags=*/NULL, &is_big_endian);
> +	if (args->ms->thread) {
> +		e_machine = thread__e_machine_endian(args->ms->thread,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);
> +	} else if (dso) {
> +		struct maps *kmaps = map ? map__kmaps(map) : NULL;

[Severity: High]
Is the same dso__kernel(dso) check missing here as well?

> +		struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> +		e_machine = dso__e_machine_endian(dso, kmap_machine, /*e_flags=*/NULL,
> +						  &is_big_endian);
> +	}

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