Re: [PATCH v9 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 4 potential issue(s) to consider:
- [High] thread__e_machine_endian() fast-path returns successfully without initializing the is_big_endian output parameter.
- [High] dso__e_machine_endian() bypasses lazy evaluation of env->arch, silently falling back to the host's endianness.
- [High] perf_arch_is_big_endian() falsely categorizes standard big-endian architectures (e.g., MIPS, m68k) as little-endian.
- [High] read_proc_e_machine_for_pid() uses a blocking open() on user-controlled /proc paths, risking an indefinite hang.
--

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

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

Add perf_arch_is_big_endian(), dso__read_e_machine_endian(),
dso__e_machine_endian(), and thread__e_machine_endian() to support
bi-endianness and cross-architecture analysis without breaking the
existing API.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index b791e1b6b2cf..6439b2a3c898 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
[ ... ]
> @@ -1308,9 +1313,11 @@ uint16_t dso__e_machine(struct dso *dso, struct machine *machine, uint32_t *e_fl
>  	case DSO_BINARY_TYPE__BPF_IMAGE:
>  	case DSO_BINARY_TYPE__OOL:
>  	case DSO_BINARY_TYPE__JAVA_JIT:
> -		if (e_flags)
> -			*e_flags = EF_HOST;
> -		return EM_HOST;
> +		if (is_big_endian) {
> +			*is_big_endian = perf_arch_is_big_endian(
> +				machine && machine->env ? machine->env->arch : NULL);

Could this direct access to machine->env->arch read a NULL pointer if the
architecture hasn't been lazily computed yet?

The arch string inside struct perf_env is lazily populated via
perf_env__arch(env). If NULL is passed to perf_arch_is_big_endian() here, it
will default to the host's byte order.

> +		}
> +		return perf_env__e_machine(machine ? machine->env : NULL, e_flags);
>  	case DSO_BINARY_TYPE__DEBUGLINK:
[ ... ]
> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 0edc67a468ab..1a4db133262b 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
[ ... ]
> @@ -339,6 +339,18 @@ int perf_env__kernel_is_64_bit(struct perf_env *env)
>  	return env->kernel_is_64_bit;
>  }
>  
> +bool perf_arch_is_big_endian(const char *arch)
> +{
> +	if (!arch)
> +		return __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__;
> +
> +	if (str_ends_with(arch, "_be") || !strcmp(arch, "sparc") || !strcmp(arch, "sparc64") ||
> +	    !strcmp(arch, "s390") || !strcmp(arch, "s390x") || !strcmp(arch, "powerpc"))
> +		return true;
> +
> +	return false;
> +}

Does this check omit standard big-endian architectures like mips, m68k, and
parisc?

Because they aren't in the allowlist, cross-architecture analysis on a MIPS
kernel might return false, causing downstream consumers to be incorrectly
configured in little-endian mode.

> +
>  int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[])
>  {
[ ... ]
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 22be77225bb0..8611293deca9 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
[ ... ]
> @@ -449,7 +449,7 @@ void thread__find_cpumode_addr_location(struct thread *thread, u64 addr,
>  	}
>  }
>  
> -static uint16_t read_proc_e_machine_for_pid(pid_t pid, uint32_t *e_flags)
> +static uint16_t read_proc_e_machine_for_pid(pid_t pid, uint32_t *e_flags, bool *is_big_endian)
>  {
>  	char path[6 /* "/proc/" */ + 11 /* max length of pid */ + 5 /* "/exe\0" */];
>  	int fd;
> @@ -458,7 +458,8 @@ static uint16_t read_proc_e_machine_for_pid(pid_t pid, uint32_t *e_flags)
>  	snprintf(path, sizeof(path), "/proc/%d/exe", pid);
>  	fd = open(path, O_RDONLY);

Is it possible for this open() to hang indefinitely if the /proc/%d/exe path
points to a blocking FUSE filesystem or an unresponsive network mount?

Opening it without O_NONBLOCK risks hanging the perf daemon.

>  	if (fd >= 0) {
> -		e_machine = dso__read_e_machine(/*optional_dso=*/NULL, fd, e_flags);
> +		e_machine = dso__read_e_machine_endian(/*optional_dso=*/NULL, fd, e_flags,
> +						       is_big_endian);
>  		close(fd);
>  	}
[ ... ]
> @@ -491,6 +495,7 @@ uint16_t thread__e_machine(struct thread *thread, struct machine *machine, uint3
>  		.machine = machine,
>  		.e_flags = 0,
>  		.e_machine = EM_NONE,
> +		.is_big_endian = false,
>  	};
>  
>  	if (e_machine != EM_NONE) {

Does this fast-path return successfully without initializing the is_big_endian
output parameter?

Because struct thread does not cache the endianness, any caller relying on
is_big_endian might read uninitialized memory or a default value from their
stack.

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