Re: [PATCH v12 01/19] 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 3 potential issue(s) to consider:
- [Critical] write_arch() writes a normalized architecture string missing bitness information, causing 64-bit architectures to be misclassified as 32-bit.
- [High] Premature caching of arch-derived `e_machine` disables dynamic thread-based `e_machine` scanning for older `perf.data` files.
- [High] Unsynchronized access to static struct utsname buffer causes data race and potential misclassification of host architecture.
--

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

perf env: Add perf_env__e_machine helper and use in perf_env__arch

This commit adds a helper to lazily compute the e_machine and fallback to
EM_HOST. It updates perf_env__arch to use this helper and modifies the
session header logic to record e_machine without forcing premature thread
scanning.

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> --- 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 (!strncmp(arch, "x86_64", 6) || !strncmp(arch, "aarch64", 7) ||
> -	    !strncmp(arch, "arm64", 5) || !strncmp(arch, "mips64", 6) ||
> -	    !strncmp(arch, "parisc64", 8) || !strncmp(arch, "riscv64", 7) ||
> -	    !strncmp(arch, "s390x", 5) || !strncmp(arch, "sparc64", 7))
> -		env->kernel_is_64_bit = 1;
> -	else
> -		env->kernel_is_64_bit = 0;
> +	if (!arch) {
> +		static struct utsname uts = { .machine[0] = '\0', };
> +
> +		if (uts.machine[0] == '\0')
> +			uname(&uts);
> +		if (uts.machine[0] != '\0')
> +			arch = uts.machine;
> +	}

[Severity: High]
Is it safe to use a lockless check for this static buffer here, as well as in
perf_env__e_machine_nocache() and perf_env__arch()?

If multiple threads concurrently trigger uname(&uts), could one thread observe
uts.machine[0] != '\0' immediately after the kernel writes the first byte?

Since uname copies data sequentially, might a thread read a partially written
string (e.g., 'x8' instead of 'x86_64'), leading to a misclassified host
architecture during multi-threaded profiling?

It appears that while a subsequent commit ("perf env: Add mutex to protect
lazy environment initialization") adds env->lock to perf_env for os_release,
it might not protect these local static buffers.

[ ... ]
> @@ -588,51 +602,237 @@ void cpu_cache_level__free(struct cpu_cache_level *cache)
[ ... ]
> +uint16_t perf_env__e_machine(struct perf_env *env, uint32_t *e_flags)
> +{
> +	uint16_t e_machine;
> +	uint32_t local_e_flags = 0;
> +
> +	if (env && env->e_machine != EM_NONE) {
> +		if (e_flags)
> +			*e_flags = env->e_flags;
> +
> +		return env->e_machine;
> +	}
> +	e_machine = perf_env__e_machine_nocache(env, &local_e_flags);
> +	/*
> +	 * Only cache the e_machine in perf_env if env->arch is not NULL.
> +	 * If env->arch is NULL, the e_machine is just a fallback to EM_HOST.
> +	 * Caching it permanently would prevent dynamic, more accurate
> +	 * thread-based session e_machine scanning later in
> +	 * perf_session__e_machine().
> +	 */
> +	if (env && env->arch) {
> +		env->e_machine = e_machine;
> +		env->e_flags = local_e_flags;
> +	}

[Severity: High]
Could this cache the arch-derived value into env->e_machine simply because
env->arch is present?

If this caches the value prematurely, might older perf.data files that lack
HEADER_E_MACHINE have env->e_machine populated before the threads are scanned?

When functions like thread__insert_map() invoke thread__e_machine_endian(),
they fall back to perf_env__e_machine() and perf_env__arch() if a DSO is not
yet loaded. Does this lock in the arch-derived host value prematurely?

> +	if (e_flags)
> +		*e_flags = local_e_flags;
> +
> +	return e_machine;
>  }
[ ... ]
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -441,21 +441,25 @@ static int write_osrelease(struct feat_fd *ff,
[ ... ]
> -static int write_arch(struct feat_fd *ff,
> -		      struct evlist *evlist __maybe_unused)
> +static int write_arch(struct feat_fd *ff, struct evlist *evlist)
>  {
>  	struct utsname uts;
> -	int ret;
> +	const char *arch = NULL;
>  
> -	ret = uname(&uts);
> -	if (ret < 0)
> -		return -1;
> +	if (evlist->session)
> +		arch = perf_env__arch(perf_session__env(evlist->session));
> +
> +	if (!arch) {
> +		int ret = uname(&uts);
>  
> -	return do_write_string(ff, uts.machine);
> +		if (ret < 0)
> +			return -1;
> +		arch = uts.machine;
> +	}
> +	return do_write_string(ff, arch);
>  }

[Severity: Critical]
Does this code write the normalized architecture string lacking bitness
information to the perf.data header instead of the full uts.machine string?

If perf_env__arch() normalizes the architecture name (e.g., 'x86_64' to 'x86'),
the resulting string will miss the '64' substring.

Later in perf_env__init_kernel_mode():

	if (arch) {
		if (strstr(arch, "64") || strstr(arch, "s390x"))
			env->kernel_is_64_bit = 1;
		else
			env->kernel_is_64_bit = 0;
		return;
	}

Could this cause kernel_is_64_bit to incorrectly evaluate to 0 for 64-bit
architectures like x86, powerpc, and sparc, leading to broken kernel symbol
resolution, broken dynamic architecture fallbacks, and loss of backwards
compatibility for older perf versions reading newly recorded files?

[ ... ]
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -4080,14 +4080,19 @@ uint16_t perf_session__e_machine(struct perf_session *session, uint32_t *e_flags
[ ... ]
> +	/*
> +	 * Is the env caching an e_machine? If not we want to compute from the
> +	 * more accurate threads.
> +	 */
>  	env = perf_session__env(session);
> -	if (env && env->e_machine != EM_NONE) {
> -		if (e_flags)
> -			*e_flags = env->e_flags;
> -
> -		return env->e_machine;
> -	}
> +	if (env && env->e_machine != EM_NONE)
> +		return perf_env__e_machine(env, e_flags);

[Severity: High]
Does this early return consume the prematurely cached value from
perf_env__e_machine()?

If env->e_machine was populated early because env->arch was present, would
this bypass the machines__for_each_thread() scan entirely, preventing the
dynamic detection of 32-bit compat tasks on a 64-bit host for older perf.data
files?

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