Re: [PATCH v10 03/18] perf env, dso, thread: Add _endian variants for e_machine helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect host endianness fallback in `thread__e_machine_endian` for NULL thread queries.
- [High] Data race and TOCTOU vulnerability due to concurrent reads and lockless updates of thread architecture properties.
--

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

perf env, dso, thread: Add _endian variants for e_machine helpers

This commit adds _endian variants for ELF architecture helpers to support
bi-endianness and cross-architecture analysis. It introduces 
perf_arch_is_big_endian() and updates the dso and thread structures to
query and cache the absolute endianness.

> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
[ ... ]
> -uint16_t thread__e_machine(struct thread *thread, struct machine *machine, uint32_t *e_flags)
> +uint16_t thread__e_machine_endian(struct thread *thread, struct machine *machine, uint32_t *e_flags,
> +				  bool *is_big_endian)
>  {
>  	pid_t tid, pid;
> -	uint16_t e_machine = RC_CHK_ACCESS(thread)->e_machine;
> +	uint16_t e_machine;
>  	uint32_t local_e_flags = 0;
> -	struct thread__e_machine_callback_args args = {
> -		.machine = machine,
> -		.e_flags = 0,
> -		.e_machine = EM_NONE,
> -	};
> +	struct thread__e_machine_callback_args args;
> +
> +	if (!thread) {
> +		if (is_big_endian)
> +			*is_big_endian = perf_arch_is_big_endian(NULL);

[Severity: High]
Does this fallback path incorrectly use the host endianness instead of the
target architecture? By passing NULL to perf_arch_is_big_endian(), it looks
like this resolves to the host's __BYTE_ORDER__ even when performing offline
cross-architecture analysis.

Should this dynamically resolve the endianness from the machine environment,
similar to how it is handled in dso__e_machine_endian() by passing the
architecture from machine->env?

> +		return perf_env__e_machine(machine ? machine->env : NULL, e_flags);
> +	}
> +
> +	e_machine = RC_CHK_ACCESS(thread)->e_machine;
> +	args.machine = machine;
> +	args.e_flags = 0;
> +	args.e_machine = EM_NONE;
> +	args.is_big_endian = false;
>  
>  	if (e_machine != EM_NONE) {
>  		if (e_flags)
>  			*e_flags = thread__e_flags(thread);
> +		if (is_big_endian)
> +			*is_big_endian = thread__e_is_big_endian(thread);
>  		return e_machine;
>  	}
[ ... ]
>  out:
>  	if (e_machine != EM_NONE) {
>  		thread__set_e_machine(thread, e_machine);
>  		thread__set_e_flags(thread, local_e_flags);
> +		thread__set_e_is_big_endian(thread, args.is_big_endian);

[Severity: High]
Is there a potential data race here when initializing the thread architecture
properties?

Since struct thread objects can be accessed concurrently, a second thread
could observe e_machine != EM_NONE in the fast-path above immediately after
thread__set_e_machine() is called, but before thread__set_e_flags() or
thread__set_e_is_big_endian() complete.

Could this cause the second thread to read uninitialized or stale metadata?
Would it be safer to update the payload properties before setting the
e_machine indicator, possibly with memory barriers?

> +		if (is_big_endian)
> +			*is_big_endian = args.is_big_endian;
>  	} else {
>  		e_machine = EM_HOST;

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