Re: [PATCH v11 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 2 potential issue(s) to consider:
- [High] Writing the normalized architecture string instead of the raw host string to HEADER_ARCH breaks 64-bit kernel detection.
- [High] Lockless reads and writes of `env->e_machine` and `env->e_flags` can cause data tearing and incorrect flag values.
--

commit c84846b53351380843f7a9f4a25aa8273e1fb3a5
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..0cd23b5fc6515 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
[ ... ]
> @@ -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]
Can concurrent lockless updates to env->e_machine and env->e_flags cause a
race condition?

If Thread A sets env->e_machine and is about to set env->e_flags, and Thread B
concurrently observes env->e_machine != EM_NONE in this function, Thread B
might read a stale or zero value for env->e_flags before Thread A finishes
writing it.

A similar unprotected update happens in perf_session__e_machine().

This appears to be fixed later in the series by the commit "perf env: Add mutex
to protect lazy environment initialization", but could this intermediate state
cause problems with per-thread event parsing?

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 5b1fa1653d2a4..220e7720fbdbc 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -441,21 +441,25 @@ static int write_osrelease(struct feat_fd *ff,
>  	return do_write_string(ff, uts.release);
>  }
>  
> -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: High]
Does this code inadvertently break 64-bit kernel detection?

Since write_arch() now uses perf_env__arch() which returns a normalized arch
string (e.g., "x86" or "s390"), it writes that normalized string to HEADER_ARCH
in the perf.data file.

Later, process_arch() reads this back into env->arch. Then,
perf_env__init_kernel_mode() checks if env->arch contains "64" or "s390x":

tools/perf/util/env.c:perf_env__init_kernel_mode() {
...
	if (arch) {
		if (strstr(arch, "64") || strstr(arch, "s390x"))
			env->kernel_is_64_bit = 1;
...

Because "x86" and "s390" lack these substrings, won't this misclassify 64-bit
kernels as 32-bit and break things like VDSO unwinding or BPF contexts?

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