Re: [PATCH v7 2/4] perf env: Add helper to lazily compute the os_release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `perf_env__os_release()` helper corrupts offline trace metadata by incorrectly assuming an uninitialized `os_release` implies 'live mode', lazily populating it with the analysis host's kernel version.
- [Medium] Using `perf_version_string` as a fallback for the OS release is semantically incorrect and pollutes trace headers.
--

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

perf env: Add helper to lazily compute the os_release

In live mode the os_release isn't being initialized, make a lazy
initialization helper that assumes when the os_release isn't
initialized this is live mode.

> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 1671769d44411..c3e464c6de2f1 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c

[ ... ]

> @@ -330,6 +330,27 @@ int perf_env__kernel_is_64_bit(struct perf_env *env)
>  	return env->kernel_is_64_bit;
>  }
>  
> +const char *perf_env__os_release(struct perf_env *env)
> +{
> +	struct utsname uts;
> +	int ret;
> +
> +	if (!env)
> +		return perf_version_string;
> +
> +	if (env->os_release)
> +		return env->os_release;
> +
> +	/*
> +	 * The os_release is being accessed but wasn't initialized from a data
> +	 * file, assume this is 'live' mode and use the release from uname. If
> +	 * uname or strdup fails then use the current perf tool version.
> +	 */
> +	ret = uname(&uts);

If we are doing offline analysis of a perf.data file that lacks the OS
release feature header, env->os_release will be NULL.

Does this helper incorrectly assume we are in live mode, and lazily
populate env->os_release with the analysis host's kernel version?

If so, it appears this causes tools to use the wrong vmlinux path for the
target profile in vmlinux_path__init() and potentially break symbol
resolution.

It also looks like this causes ctf_writer__setup_env() to export the host
kernel version into the CTF trace instead of safely skipping the missing
field as it did previously.

> +	env->os_release = strdup(ret < 0 ? perf_version_string : uts.release);
> +	return env->os_release ?: perf_version_string;
> +}

Is it appropriate to use the perf tool version (perf_version_string) as a
fallback for the OS release if uname() or strdup() fails?

This string could end up permanently written into the perf.data file's OS
release section, or cause bogus filesystem lookups in vmlinux_path__init()
since the tool version format differs from the kernel release.

Would it be safer to return NULL or a generic unknown string here instead?

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