Re: [PATCH v11 06/19] perf symbol: Avoid use of machine__is

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Cross-platform analysis regression due to machine_or_dso_e_machine falling back to the host architecture when perf_env is missing.
--

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

perf symbol: Avoid use of machine__is

This commit switches from string comparisons against the perf_env arch
to using the ELF machine value from the dso or running machine.

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 714b6e6048fa2..2ce512f08a1d6 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c

[ ... ]

> @@ -851,6 +851,23 @@ static int maps__split_kallsyms_for_kcore(struct maps *kmaps, struct dso *dso)
>  	return count;
>  }
>  
> +static uint16_t machine_or_dso_e_machine(struct machine *machine, struct dso *dso)
> +{
> +	uint16_t e_machine = EM_NONE;
> +	/* DSO should be most accurate */
> +	if (dso)
> +		e_machine = dso__e_machine(dso, machine, /*e_flags=*/NULL);
> +
> +	if (e_machine != EM_NONE)
> +		return e_machine;
> +
> +	/* Check the global environment next. */
> +	if (machine && machine->env && machine->env->e_machine != EM_NONE)
> +		return machine->env->e_machine;
> +
> +	return perf_env__e_machine(machine ? machine->env : NULL, /*e_flags=*/NULL);

[Severity: High]
When machine->env is NULL (such as in pipe mode before perf_env is fully
initialized), perf_env__e_machine() falls back to evaluating the host
architecture via uname().

If we are on an x86_64 host analyzing a non-x86_64 perf.data file without
an environment, will this silently return EM_X86_64 and break cross-platform
analysis?

Previously, machine__is() safely returned false if the environment was NULL.

> +}
> +
>  /*
>   * Split the symbols into maps, making sure there are no overlaps, i.e. the
>   * kernel range is broken in several maps, named [kernel].N, as we don't have

[ ... ]

> @@ -925,7 +941,7 @@ static int maps__split_kallsyms(struct maps *kmaps, struct dso *dso, u64 delta,
>  			 */
>  			pos->start = map__map_ip(curr_map, pos->start);
>  			pos->end   = map__map_ip(curr_map, pos->end);
> -		} else if (x86_64 && is_entry_trampoline(pos->name)) {
> +		} else if (e_machine == EM_X86_64 && is_entry_trampoline(pos->name)) {

[Severity: High]
Could this erroneously discard symbols resembling entry trampolines on
foreign architecture data when parsed on an x86_64 host, due to the
host fallback behavior in machine_or_dso_e_machine() described above?

>  			/*
>  			 * These symbols are not needed anymore since the
>  			 * trampoline maps refer to the text section and it's

[ ... ]

> @@ -1428,7 +1444,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  		free(new_node);
>  	}
>  
> -	if (machine__is(machine, "x86_64")) {
> +	if (machine_or_dso_e_machine(machine, dso) == EM_X86_64) {

[Severity: High]
Does this similarly risk executing x86_64-specific trampoline logic on
non-x86_64 data if run on an x86_64 host without a perf_env?

>  		u64 addr;
>  
>  		/*

[ ... ]

> @@ -1716,7 +1732,7 @@ int dso__load(struct dso *dso, struct map *map)
>  			ret = dso__load_guest_kernel_sym(dso, map);
>  
>  		machine = maps__machine(map__kmaps(map));
> -		if (machine__is(machine, "x86_64"))
> +		if (machine && machine_or_dso_e_machine(machine, dso) == EM_X86_64)

[Severity: High]
Does this incorrectly map x86_64 entry trampolines for foreign architecture
data if the machine environment is missing and we fall back to the host arch?

>  			machine__map_x86_64_entry_trampolines(machine, dso);
>  		goto out;
>  	}

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