Re: [PATCH v10 01/18] perf env: Add perf_env__e_machine helper and use in perf_env__arch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] x86_64 is severely misclassified as 32-bit locally and in newly recorded `perf.data` files due to normalization overwriting the architecture string.
- [High] Removing `normalize_arch` breaks the parsing of older `perf.data` files by returning unnormalized architecture strings.
- [High] Kernel 64-bit detection fails for architectures with endianness or variant suffixes due to a strict `str_ends_with` check.
- [High] Unsynchronized lazy initialization of `perf_env` fields (`arch`, `e_machine`, `e_flags`) causes data races and memory leaks.
--

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

perf env: Add perf_env__e_machine helper and use in perf_env__arch

Add a helper that lazily computes the e_machine and falls back to EM_HOST.
Use the perf_env's arch to compute the e_machine if available, using a
binary search for efficiency while handling duplicate rules.

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 20953ef7b9d8f..23dcd7a108a10 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
[ ... ]
> @@ -309,15 +311,27 @@ void perf_env__init(struct perf_env *env)
>  
>  static void perf_env__init_kernel_mode(struct perf_env *env)
>  {
> -	const char *arch = perf_env__raw_arch(env);
> +	const char *arch = env->arch;
[ ... ]
> +	if (arch) {
> +		if (str_ends_with(arch, "64") || !strncmp(arch, "s390x", 5))

[Severity: High]
Does this strict str_ends_with(arch, "64") check reject valid 64-bit
architectures that possess trailing variant suffixes?

For example, architectures like "aarch64_be", "mips64el", or "riscv64gc" do
not end exactly in "64". Will they fail this check and be incorrectly classified
as 32-bit (env->kernel_is_64_bit = 0), which could break unwinding and BPF
parsing on those platforms?

[Severity: Critical]
Additionally, could this str_ends_with() check cause standard x86_64 systems to
be misclassified as 32-bit locally and in newly recorded perf.data files?

When recording a new perf.data file on x86_64, perf_env__arch() maps the host
machine to the normalized string "x86" and caches it in env->arch. Later, when
perf_env__init_kernel_mode() evaluates the cached "x86" string here,
str_ends_with("x86", "64") will evaluate to false, erroneously setting
env->kernel_is_64_bit to 0.

> +			env->kernel_is_64_bit = 1;
> +		else
> +			env->kernel_is_64_bit = 0;
> +		return;
> +	}
[ ... ]
> @@ -588,51 +602,249 @@ void cpu_cache_level__free(struct cpu_cache_level *cache)
[ ... ]
>  const char *perf_env__arch(struct perf_env *env)
>  {
> -	char *arch_name;
> +	uint16_t e_machine;
> +	const char *arch;
>  
> -	if (!env || !env->arch) { /* Assume local operation */
> +	if (!env) {
[ ... ]
> +	if (env->arch)
> +		return env->arch;

[Severity: High]
By returning env->arch directly and removing the normalize_arch() call, does
this break the parsing of older perf.data files?

When reading from older files, env->arch contains the raw unnormalized string
(e.g., "x86_64") from the HEADER_ARCH section. Returning this unnormalized
string directly bypasses standard perf architecture aliases, which could break
callers expecting canonical names, such as arch_syscalls__strerrno_function().

>  
> -	return normalize_arch(arch_name);
[ ... ]
> +	/*
> +	 * Only cache the normalized arch string in env->arch if the
> +	 * e_machine was successfully resolved (i.e. not the unpopulated
> +	 * fallback to EM_HOST), allowing threads to dynamically indicate
> +	 * a better ELF machine type later.
> +	 */
> +	if (env && env->e_machine != EM_NONE)
> +		env->arch = strdup(arch);

[Severity: High]
Since perf_env can be shared across multiple threads during concurrent event
processing (e.g., in perf report), could this unsynchronized lazy caching of
env->arch via strdup() lead to data races and memory leaks?

Multiple threads observing env->arch as NULL could concurrently allocate and
overwrite the same pointer. Additionally, without memory barriers, concurrent
readers might observe a non-NULL env->arch pointer but read uninitialized
memory. Similar unsynchronized lazy writes seem to happen for env->e_machine
and env->e_flags in perf_env__e_machine().

> +	/*
> +	 * Avoid potential crashes on the arch string if memory allocation in
> +	 * strdup fails and NULL were to be returned.
> +	 */
> +	return env->arch ?: arch;
>  }

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