Re: [RFC PATCH v2 2/9] perf pmus: Add perf_pmus__read_from_env()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] perf pmus: stale core PMU cache and host probing in perf_pmus__read_from_env()
- [Medium] perf pmu: out-of-bounds container_of() read on from_env drm and hwmon PMUs
- [Medium] perf pmus: broken x86 and cross-arch CPUID lookup in perf_pmus__read_from_env()
- [Medium] perf pmus: env_pmu_find_or_add() misclassifies non-'uncore_' uncore PMUs
--

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

perf pmus: Add perf_pmus__read_from_env()

This commit adds perf_pmus__read_from_env() and perf_pmu__add_cap() to
reconstruct core_pmus and other_pmus lists and their capabilities from
a perf.data header environment.

> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 1355b317f3eda..ea727d577c89d 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
[ ... ]
> @@ -916,3 +919,205 @@ struct perf_pmu *perf_pmus__fake_pmu(void)
[ ... ]
> +static struct perf_pmu *env_pmu_find_or_add(const char *name, __u32 type,
> +					    struct perf_env *env,
> +					    const struct pmu_events_table *core_table)
> +{
> +	struct perf_pmu *pmu = pmu_find(name);
> +
> +	if (pmu) {
> +		if (pmu->type == PERF_PMU_TYPE_FAKE && type != PERF_PMU_TYPE_FAKE)
> +			pmu->type = type;
> +		return pmu;
> +	}
> +
> +	pmu = zalloc(sizeof(*pmu));
> +	if (!pmu)
> +		return NULL;
> +
> +	if (perf_pmu__init(pmu, type, name, /*from_env=*/true) != 0) {
> +		perf_pmu__delete(pmu);
> +		return NULL;
> +	}
> +
> +	pmu->sysfs_aliases_loaded = true;

[Severity: Medium]
Can allocating a plain struct perf_pmu here trigger an out-of-bounds read via
container_of() when type belongs to a DRM or HWMON PMU?

When HEADER_PMU_MAPPINGS contains drm_* or hwmon_* PMUs (with type in
PERF_PMU_TYPE_DRM_START..PERF_PMU_TYPE_DRM_END or
PERF_PMU_TYPE_HWMON_START..PERF_PMU_TYPE_HWMON_END), env_pmu_find_or_add()
allocates only sizeof(struct perf_pmu) with pmu->from_env = true and
pmu->sysfs_aliases_loaded = true.

Unlike perf_pmu__have_event() or perf_pmu__delete(), perf_pmu__config_terms()
and perf_pmu__check_alias() in tools/perf/util/pmu.c do not check
!pmu->from_env before dispatching on perf_pmu__is_drm() and
perf_pmu__is_hwmon():

tools/perf/util/pmu.c:perf_pmu__config_terms() {
	if (perf_pmu__is_hwmon(pmu))
		return hwmon_pmu__config_terms(pmu, attr, terms, err);
	if (perf_pmu__is_drm(pmu))
		return drm_pmu__config_terms(pmu, attr, terms, err);
}

tools/perf/util/pmu.c:perf_pmu__check_alias() {
	if (perf_pmu__is_hwmon(pmu)) {
		ret = hwmon_pmu__check_alias(head_terms, info, err);
		goto out;
	}
	if (perf_pmu__is_drm(pmu)) {
		ret = drm_pmu__check_alias(pmu, head_terms, info, err);
		goto out;
	}
}

drm_pmu__check_alias() and drm_pmu__config_terms() cast pmu via
container_of(pmu, struct drm_pmu, pmu) and read drm->num_events and
drm->events past the end of struct perf_pmu. Similarly,
hwmon_pmu__config_terms() casts pmu via container_of(pmu, struct hwmon_pmu,
pmu), sees pmu->sysfs_aliases_loaded == true in hwmon_pmu__read_events(), and
reads hwm->events out-of-bounds in hwmon_pmu__config_term().

Should perf_pmu__config_terms() and perf_pmu__check_alias() also guard the
DRM and HWMON paths with !pmu->from_env?

> +	pmu->caps_initialized = true;
> +	pmu->max_precise = -1;
> +	pmu->is_core = env_is_pmu_core(name, env);
> +	pmu->is_uncore = strstarts(name, "uncore_");
> +
> +	if (!strcmp(name, "software") || !strcmp(name, "tool"))
> +		pmu->events_table = perf_pmu__default_core_events_table();
> +	else if (core_table && (pmu->is_core || pmu->is_uncore))
> +		pmu->events_table = core_table;

[Severity: Medium]
Does checking strstarts(name, "uncore_") leave non-"uncore_" uncore PMUs
without pmu->is_uncore and pmu->events_table?

Many uncore and package PMUs whose JSON events are compiled into core_table by
jevents.py do not have names starting with "uncore_" in sysfs and
HEADER_PMU_MAPPINGS, such as amd_l3, amd_df, and amd_umc_<N> on AMD x86,
hisi_sccl<N>_ddrc<M> / hisi_sccl<N>_hha<M> / hisi_sccl<N>_l3c<M> on arm64, and
pai_crypto / pai_ext on s390.

Because pmu->is_uncore is false for these PMUs, env_pmu_find_or_add() leaves
pmu->events_table as NULL, and perf_pmu___name_match() in
tools/perf/util/pmu.c takes the !pmu->is_uncore exact-match branch:

tools/perf/util/pmu.c:perf_pmu___name_match() {
	...
	if (!pmu->is_uncore) {
		/*
		 * PMU isn't core or uncore, some kind of broken CPU mask
		 * situation. Only match exact name.
		 */
		return is_same;
	}
	return pmu->is_core ?
		perf_pmu__match_ignoring_suffix(pmu_name, to_match) :
		(wildcard_match ?
		 perf_pmu__match_wildcard_uncore(pmu_name, to_match) :
		 perf_pmu__match_ignoring_suffix_uncore(pmu_name, to_match));
}

As a result, perf_pmu__num_events(), perf_pmu__for_each_event(), and
perf_pmu__have_event() miss all JSON events for these uncore PMUs.

> +
> +	list_add_tail(&pmu->list, pmu->is_core ? &core_pmus : &other_pmus);
> +	return pmu;
> +}
[ ... ]
> +int perf_pmus__read_from_env(struct perf_env *env)
> +{
> +	const struct pmu_events_table *core_table = NULL;
> +	struct perf_pmu *pmu;
> +
> +	if (!env)
> +		return -EINVAL;
> +
> +	perf_pmus__destroy();
> +	read_pmu_types = PERF_TOOL_PMU_TYPE_ALL_MASK;

[Severity: Medium]
When perf_pmus__read_from_env() destroys and replaces core_pmus, how are the
cached states in perf_pmus__num_core_pmus() and
perf_pmus__supports_extended_type() handled?

First, perf_pmus__num_core_pmus() caches the core PMU count in a function-local
static variable that is not reset by perf_pmus__destroy():

tools/perf/util/pmus.c:perf_pmus__num_core_pmus() {
	static int count;

	if (!count) {
		struct perf_pmu *pmu = NULL;

		while ((pmu = perf_pmus__scan_core(pmu)) != NULL)
			count++;
	}
	return count;
}

If perf_pmus__num_core_pmus() is called before perf_pmus__read_from_env() (or
after a later perf_pmus__destroy()), it returns the stale count to callers
such as evsel__is_hybrid(), perf_pmu__auto_merge_stats(),
perf_env__read_core_pmu_caps(), and hybrid_topology__new() that use it to
detect hybrid topologies and size heap allocations before iterating
perf_pmus__scan_core().

Second, perf_pmus__supports_extended_type() caches
perf_pmus__do_support_extended_type via pthread_once, and
__perf_pmus__supports_extended_type() probes the live host kernel via
is_event_supported():

tools/perf/util/pmus.c:__perf_pmus__supports_extended_type() {
	struct perf_pmu *pmu = NULL;

	if (perf_pmus__num_core_pmus() <= 1)
		return false;

	while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
		if (!is_event_supported(PERF_TYPE_HARDWARE, PERF_COUNT_HW_CPU_CYCLES | ((__u64)pmu->type << PERF_PMU_TYPE_SHIFT)))
			return false;
	}

	return true;
}

When analyzing a hybrid perf.data on a non-hybrid or cross-architecture host
(or when a hybrid PMU reconstructed from HEADER_HYBRID_TOPOLOGY has
PERF_PMU_TYPE_FAKE), is_event_supported() fails against the host kernel.
Consequently, perf_pmus__find_by_attr() and evsel__find_pmu() skip resolving
attr->config >> PERF_PMU_TYPE_SHIFT and assign PERF_TYPE_HARDWARE and
PERF_TYPE_HW_CACHE events to the first core PMU.

> +
> +	if (env->cpuid) {
> +		const char *arch = perf_env__arch(env);
> +		char cpuid_str[128];
> +		char vendor[32];
> +		unsigned int family, model, step;
> +		char extra;
> +
> +		core_table = find_core_events_table(arch, env->cpuid);
> +		/*
> +		 * x86 HEADER_CPUID stores "vendor,family,model,step" in decimal
> +		 * while pmu-events tables match "vendor-family-HEXMODEL[-HEXSTEP]".
> +		 */
> +		if (!core_table && !strcmp(arch, "x86") &&
> +		    sscanf(env->cpuid, "%31[^,],%u,%u,%u%c",
> +			   vendor, &family, &model, &step, &extra) == 4) {
> +			snprintf(cpuid_str, sizeof(cpuid_str), "%s-%u-%X-%X",
> +				 vendor, family, model, step);
> +			core_table = find_core_events_table(arch, cpuid_str);
> +			if (!core_table) {
> +				snprintf(cpuid_str, sizeof(cpuid_str), "%s-%u-%X",
> +					 vendor, family, model);
> +				core_table = find_core_events_table(arch, cpuid_str);
> +			}
> +		}
> +	}

[Severity: Medium]
Does calling find_core_events_table(arch, env->cpuid) with the unformatted x86
env->cpuid emit error messages and misbehave across architectures?

On x86, HEADER_CPUID is recorded as comma-separated "vendor,family,model,step"
with no '-' characters. Passing raw env->cpuid into find_core_events_table()
calls x86's strcmp_cpuid_str() against every x86 entry in pmu_events_map. When
it reaches 3-dash map entries such as GenuineIntel-6-55-[56789ABCDEF],
full_mapcpuid is true while full_cpuid is false:

tools/perf/arch/x86/util/header.c:strcmp_cpuid_str() {
	...
	if (full_mapcpuid && !full_cpuid) {
		pr_info("Invalid CPUID %s. Full CPUID is required, "
			"vendor-family-model-stepping\n", id);
		return 1;
	}
}

This prints "Invalid CPUID ... Full CPUID is required,
vendor-family-model-stepping" to stderr on every x86 perf.data file before the
sscanf() conversion runs, and the 3-field "%s-%u-%X" fallback triggers the
same message again if the 4-field lookup does not find a match (while x86's
strcmp_cpuid_str() already strips stepping when !full_mapcpuid && full_cpuid).

In addition, find_core_events_table() calls the host binary's
strcmp_cpuid_str() rather than matching according to the target architecture
in arch:

tools/perf/arch/arm64/util/header.c:strcmp_cpuid_str() {
	u64 map_id = strtoull(mapcpuid, NULL, 16);
	char map_id_variant = FIELD_GET(MIDR_VARIANT_MASK, map_id);
	char map_id_revision = FIELD_GET(MIDR_REVISION_MASK, map_id);
	u64 id = strtoull(idstr, NULL, 16);
	...
}

When an arm64 host binary inspects an x86 perf.data file, arm64's
strcmp_cpuid_str() parses both non-hex x86 CPUID strings as 0 via strtoull()
and falsely matches the first x86 table entry. Conversely, on a host using the
default __weak strcmp_cpuid_str(), 4-field "%s-%u-%X-%X" strings do not strip
the stepping to match 3-field x86 table entries.

[ ... ]

> +}

([ ... ])

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