Re: [PATCH v10 18/18] perf symbol: Lazily compute idle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The fallback mechanism in `perf_env__os_release()` defeats the unpopulated guest trace safeguard, poisoning the guest environment with host data and causing false negatives for s390 `psw_idle`.
- [Medium] The `e_machine` prioritization bypasses the accurate DSO architecture, breaking cross-platform analysis for unpopulated traces.
--

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

perf symbol: Lazily compute idle

This patch switches from an idle boolean to a helper symbol__is_idle function.
It lazily computes whether a symbol is an idle function by taking the kernel
version and architecture of the machine into consideration.

Link: https://lore.kernel.org/lkml/20260219113850.354271-1-tmricht@linux.ibm.com/

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
[ ... ]
> +	e_machine = env ? perf_env__e_machine(env, NULL) :
> +		(dso ? dso__e_machine((struct dso *)dso, NULL, NULL) : EM_NONE);

[Severity: Medium]
When analyzing an unpopulated guest trace where the env is present but its
arch field is empty, does perf_env__e_machine() fall back to uname() and
return the host architecture?

Because the ternary operator prioritizes the environment, this short-circuits
and bypasses the more accurate dso__e_machine() check which would read the
actual target ELF binary. Could this break cross-platform analysis by
evaluating a guest trace against the host's idle routines?

> +
> +	if (e_machine == EM_386 || e_machine == EM_X86_64) {
[ ... ]
> +	if (e_machine == EM_S390 && strstarts(name, "psw_idle")) {
> +		int major = 0, minor = 0;
> +		const char *release = env ? perf_env__os_release(env) : NULL;
> +
> +		/*
> +		 * If we can't determine the release (e.g. unpopulated guest traces),
> +		 * default to idle.
> +		 */
> +		if (!release) {

[Severity: Medium]
If the env os_release is unpopulated, does perf_env__os_release() fall back to
calling uname() to get the host's OS release and cache it in the guest's
environment?

If so, it appears the !release check becomes dead code for unpopulated guest
traces. An old s390 guest trace analyzed on a modern host might incorrectly
use the host's version, fail the version check below, and misclassify psw_idle
as not idle. Can we prevent the host release from poisoning the guest
environment here?

> +			symbol__set_idle(sym, /*idle=*/true);
> +			return true;
> +		}
> +
> +		/* Before v6.10, s390 used psw_idle. */
> +		if (sscanf(release, "%d.%d", &major, &minor) == 2 &&
> +		    (major < 6 || (major == 6 && minor < 10))) {
> +			symbol__set_idle(sym, /*idle=*/true);
> +			return true;
> +		}
> +	}

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