Re: [PATCH v6 1/3] perf env: Add perf_env__e_machine helper and use in perf_env__arch

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] perf_session__e_machine() bypasses lazy evaluation of e_machine from env->arch when env->e_machine is EM_NONE, breaking cross-architecture analysis of legacy perf.data files.
- [High] Uninitialized env->kernel_is_64_bit coerces to true, causing 32-bit architectures to be parsed as 64-bit.
- [High] write_arch() silently caches EM_HOST, permanently bypassing accurate thread-based architecture detection during perf record.
- [Medium] Unhandled OOM in process_arch() leaves env->arch as NULL, leading to a subsequent NULL pointer dereference.
--

commit a2ed4a04364ee664047eec1e47e4e540e458eff9
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 of
EM_HOST. Use the perf_env's arch to compute the e_machine if
available. Use a binary search for some efficiency in this, but handle
somewhat complex duplicate rules.

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 1e54e2c863605..339d62ca37bb8 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
[ ... ]
> @@ -588,51 +590,160 @@ void cpu_cache_level__free(struct cpu_cache_level *cache)
[ ... ]
> +uint16_t perf_env__e_machine(struct perf_env *env, uint32_t *e_flags)
> +{
> +	if (!env) {
> +		if (e_flags)
> +			*e_flags = EF_HOST;
> +
> +		return EM_HOST;
> +	}
> +	if (env->e_machine == EM_NONE) {
> +		env->e_machine = perf_arch_to_e_machine(env->arch, env->kernel_is_64_bit);

Is env->kernel_is_64_bit guaranteed to be a boolean here?

If env->kernel_is_64_bit is uninitialized, it defaults to -1 in
perf_env__init(). In C, -1 evaluates as true, which would cause 32-bit
architectures to be parsed as 64-bit (e.g., returning EM_X86_64 instead
of EM_386).

Should this use the lazy evaluator perf_env__kernel_is_64_bit(env) instead?

[ ... ]
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index c6efddb70aee2..9bb4a271b4f8a 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -370,21 +370,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));

When write_arch() is called during perf record, it invokes perf_env__arch().
Since env->arch is NULL and env->e_machine is EM_NONE, this falls back to
perf_arch_to_e_machine(NULL, ...), locking env->e_machine to EM_HOST.

Later, when write_e_machine() calls perf_session__e_machine(), the check for
env->e_machine != EM_NONE will evaluate to true.

Will this permanently bypass the accurate thread-based architecture detection
(machines__for_each_thread) designed to handle mixed 32/64-bit threads?

[ ... ]
> @@ -2675,10 +2679,30 @@ static int process_##__feat(struct feat_fd *ff, void *data __maybe_unused) \
[ ... ]
> +static int process_arch(struct feat_fd *ff, void *data __maybe_unused)
> +{
> +	uint16_t saved_e_machine = ff->ph->env.e_machine;
> +
> +	free(ff->ph->env.arch);
> +	ff->ph->env.arch = do_read_string(ff);
> +	if (!ff->ph->env.arch)
> +		return -ENOMEM;
> +	/*
> +	 * Make the arch string canonical by computing the e_machine from it,
> +	 * then turning the e_machine back into an arch string.
> +	 */
> +	ff->ph->env.e_machine = EM_NONE;
> +	if (perf_env__e_machine(&ff->ph->env, /*e_flags=*/NULL) != EM_NONE) {
> +		zfree(&ff->ph->env.arch);
> +		perf_env__arch(&ff->ph->env);

If the strdup() inside perf_env__arch() fails due to memory constraints,
it returns NULL and env->arch remains NULL.

Since process_arch() unconditionally returns 0 here, could this lead to a
NULL pointer dereference later if strncmp() is called on the architecture
string?

> +	}
> +	ff->ph->env.e_machine = saved_e_machine;
> +	return 0;
> +}
> +
>  static int process_e_machine(struct feat_fd *ff, void *data __maybe_unused)
>  {
>  	int ret;
[ ... ]
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index fe0de2a0277f0..726568b888039 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -3023,14 +3023,16 @@ uint16_t perf_session__e_machine(struct perf_session *session, uint32_t *e_flags
>  		return EM_HOST;
>  	}
>  
> +	/* Is the env caching an e_machine? */
>  	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);

For legacy perf.data files without HEADER_E_MACHINE, process_arch() restores
env->e_machine to EM_NONE.

Since the condition here explicitly requires env->e_machine != EM_NONE, won't
this completely bypass the new lazy evaluator and fall back to EM_HOST,
potentially breaking cross-architecture analysis for legacy files?

>  
> +	/*
> +	 * Compute from threads, note this is more accurate than
> +	 * perf_env__e_machine that falls back on EM_HOST and doesn't consider
> +	 * mixed 32-bit and 64-bit threads.
> +	 */
>  	machines__for_each_thread(&session->machines,
>  				  perf_session__e_machine_cb,
>  				  &args);

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