Re: [PATCH v2 25/30] perf pmus: Add perf_pmus__read_from_env()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf pmus: empty core_pmus in perf_pmus__read_from_env()
  without cpu caps
- [Medium] perf pmus: broken event config name lookup in
  env_pmu_find_or_add()
--

commit 62379d346fd8a3e273dd7df18d358ca721a7bc84
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 and other PMU lists and capabilities from perf.data
header metadata, and lazily populates env->pmus in perf_env__pmus().

> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 6d68371107aa1..b924ff6180e41 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c

[ ... ]

> @@ -981,3 +984,234 @@ struct perf_pmu *perf_pmus__fake_pmu(void)
>  
>  	return &fake;
>  }

[ ... ]

> +	pmu->pmus = pmus;
> +	pmu->sysfs_aliases_loaded = true;
> +	pmu->caps_initialized = true;
> +	pmu->max_precise = -1;
> +	pmu->is_core = env_is_pmu_core(name, env);
> +	pmu->auxtrace = !strcmp(name, "intel_pt") ||
> +			!strcmp(name, "intel_bts") ||
> +			!strcmp(name, "cs_etm") ||
> +			strstarts(name, "arm_spe") ||
> +			strstarts(name, "hisi_ptt");
> +	pmu->is_uncore = !pmu->is_core && !pmu->auxtrace &&
> +			 !perf_pmu__is_software(pmu) &&
> +			 (perf_pmu__kind(pmu) == PERF_PMU_KIND_PE ||
> +			  pmu->type == PERF_PMU_TYPE_FAKE);
> +
> +	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 setting pmu->events_table on an env-backed PMU while leaving
pmu->format empty break perf_pmu__name_from_config() and leak help_msg?

When print_event_desc() or evsel__fprintf() calls
perf_event_attr__fprintf() with perf_env__pmus(env),
perf_pmu__for_each_name_from_config() loads the JSON aliases from
pmu->events_table via pmu_add_cpu_aliases() and calls
perf_pmu__parse_terms_to_attr() -> pmu_config_term() with err == NULL on
each alias.  Because pmu->format is empty, pmu_find_format() returns NULL
for every format term (such as event= or umask=):

tools/perf/util/pmu.c:pmu_config_term() {
    ...
	format = pmu_find_format(&pmu->format, term->config);
	if (!format) {
		char *pmu_term = pmu_formats_string(&pmu->format);
		char *unknown_term;
		char *help_msg;
    ...
		help_msg = parse_events_formats_error_string(pmu_term);
		if (err) {
			parse_events_error__handle(err, term->err_term,
						   unknown_term,
						   help_msg);
		} else {
			pr_debug("%s (%s)\n", unknown_term, help_msg);
			free(unknown_term);
		}
		free(pmu_term);
		return -EINVAL;
	}
    ...
}

As a result, perf_pmu__name_from_config() fails to resolve format-based
event names, and every alias iteration leaks help_msg in the !err branch.

[ ... ]

> +int perf_pmus__read_from_env(struct perf_pmus *pmus, struct perf_env *env)
> +{
> +	const struct pmu_events_table *core_table = NULL;
> +	struct perf_pmu *pmu;
> +
> +	if (!pmus || !env)
> +		return -EINVAL;
> +
> +	perf_pmus__exit(pmus);
> +	pmus->read_pmu_types = PERF_TOOL_PMU_TYPE_ALL_MASK;

[ ... ]

> +	if (env->nr_cpu_pmu_caps > 0 && env->cpu_pmu_caps) {
> +		pmu = pmu_find(pmus, "cpu");
> +		if (!pmu && !list_empty(&pmus->core_pmus)) {
> +			/* Match the writer's perf_pmus__find_core_pmu() order. */
> +			list_sort(NULL, &pmus->core_pmus, pmus_cmp);
> +			pmu = list_first_entry(&pmus->core_pmus, struct perf_pmu, list);
> +			if (pmu->nr_caps > 0)
> +				pmu = NULL;
> +		} else if (!pmu) {
> +			pmu = env_pmu_find_or_add(pmus, "cpu", PERF_TYPE_RAW,
> +						  env, core_table);
> +		}
> +		if (pmu)
> +			env_pmu_add_caps_strings(pmu, env->nr_cpu_pmu_caps, env->cpu_pmu_caps);
> +	}

[Severity: Medium]
Can perf_pmus__read_from_env() leave pmus->core_pmus empty when
env->nr_cpu_pmu_caps is 0?

Unlike pmu_read_sysfs(), which always creates a placeholder core PMU when
pmus->core_pmus is empty, the fallback env_pmu_find_or_add(pmus, "cpu",
PERF_TYPE_RAW, ...) here only runs when env->nr_cpu_pmu_caps > 0.  If a
perf.data file has HEADER_PMU_MAPPINGS or HEADER_PMU_CAPS without CPU PMU
capabilities and no PMU name matches env_is_pmu_core() (for example, a VM
recording with only software and tracepoint PMUs), pmus->core_pmus stays
empty while pmus->read_pmu_types is set to PERF_TOOL_PMU_TYPE_ALL_MASK,
preventing pmu_read_sysfs() from creating the placeholder.
perf_pmus__find_core_pmu() and evsel__find_pmu() then return NULL for
PERF_TYPE_HARDWARE, PERF_TYPE_HW_CACHE, and PERF_TYPE_RAW events.

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