Re: [PATCH v3 01/14] perf stat: Introduce core generic print traversal engine and header stubs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Crash in `is_basic_shadow_metric` if `mexp->metric_name` is NULL.
- [High] NULL pointer dereferences in metric calculation when `config->aggr_map` is NULL.
- [High] The new `is_basic_shadow_metric` filter erroneously drops all custom user metrics in standard output mode.
- [Medium] The core display traversal driver completely omits `should_skip_zero_counter()` logic, resulting in massive log spam.
- [Low] Manual forward declaration of standard POSIX type `struct timespec` violates musl compatibility guidelines.

Pre-existing issues:
- [High] Uninitialized pointer `unit` is passed to `cb->print_metric()` if `perf_pmu__convert_scale()` fails.
--

--- Patch [1]: [PATCH v3 01/14] perf stat: Introduce core generic print traversal engine and header stubs ---
commit 82eac7a69c1cb36c36fa86107ab3ce1a2fd91c7d
Author: Ian Rogers <irogers@google.com>

perf stat: Introduce core generic print traversal engine and header stubs

This patch introduces the initial infrastructure for decoupling the perf stat
printing API. It declares the struct perf_stat_print_callbacks interface
and the generic traversal driver perf_stat__print_cb() alongside format-agnostic
shared helpers.

> diff --git a/tools/perf/util/stat-print.c b/tools/perf/util/stat-print.c
> new file mode 100644
> index 0000000000000..0fff50dce80be
> --- /dev/null
> +++ b/tools/perf/util/stat-print.c
[ ... ]
> +static bool tool_pmu__is_time_event(const struct perf_stat_config *config,
> +				    const struct evsel *evsel, int *tool_aggr_idx)
> +{
> +	enum tool_pmu_event event = evsel__tool_event(evsel);
> +	int aggr_idx;
> +
> +	if (event != TOOL_PMU__EVENT_DURATION_TIME && event != TOOL_PMU__EVENT_USER_TIME &&
> +	    event != TOOL_PMU__EVENT_SYSTEM_TIME)
> +		return false;
> +
> +	if (config) {
> +		cpu_aggr_map__for_each_idx(aggr_idx, config->aggr_map) {

[Severity: High]
Can this dereference a NULL config->aggr_map?

In perf_stat__print_cb(), there is a fallback path where config->aggr_map is
NULL. It calls perf_stat_print_metrics(..., 0), which calls prepare_metric().
When prepare_metric() invokes tool_pmu__is_time_event(), the macro
cpu_aggr_map__for_each_idx() appears to dereference aggr_map->nr without
checking if the map is NULL.

[ ... ]
> +static int prepare_metric(const struct perf_stat_config *config, const struct metric_expr *mexp,
> +			  struct evsel *evsel, struct expr_parse_ctx *pctx, int aggr_idx)
> +{
[ ... ]
> +		if (!ps || !metric_events[i]->supported) {
> +			val = NAN;
> +			source_count = 0;
> +		} else {
> +			struct perf_stat_aggr *aggr =
> +				&ps->aggr[is_tool_time ? tool_aggr_idx : aggr_idx];
> +
> +			if (aggr->counts.run == 0) {

[Severity: High]
Does this code dereference a NULL pointer when config->aggr_map is NULL?

If we reach this path from the fallback branch in perf_stat__print_cb() where
config->aggr_map is NULL, ps->aggr will also be NULL.

[ ... ]
> +static int calculate_and_print_metric(const struct perf_stat_config *config,
> +				      const struct perf_stat_print_callbacks *cb, void *outer_ctx,
> +				      struct metric_expr *mexp, struct evsel *evsel, int aggr_idx)
> +{
[ ... ]
> +			char *unit;
> +
> +			if (metric_threshold &&
> +			    expr__parse(&threshold, pctx, metric_threshold) == 0 &&
> +			    !isnan(threshold)) {
> +				thresh = fpclassify(threshold) == FP_ZERO ? METRIC_THRESHOLD_GOOD :
> +									    METRIC_THRESHOLD_BAD;
> +			}
> +
> +			if (metric_unit && metric_name) {
> +				if (perf_pmu__convert_scale(metric_unit, &unit, &scale) >= 0)
> +					ratio *= scale;
> +
> +				ret = cb->print_metric(outer_ctx, config, evsel, aggr_idx,
> +						       metric_name, unit, ratio, thresh);

[Severity: High]
This is a pre-existing issue, but does this pass an uninitialized unit pointer
to print_metric() if perf_pmu__convert_scale() fails?

If perf_pmu__convert_scale() returns a negative value, unit will remain
uninitialized. The call to cb->print_metric() sits outside the success check,
so it seems we'll pass whatever happens to be on the stack.

> +			} else {
> +				ret = cb->print_metric(outer_ctx, config, evsel, aggr_idx,
> +						       metric_name ?: (evsel->name ?: ""), NULL,
> +						       ratio, thresh);
> +			}
> +		}
> +	}
> +
> +	expr__ctx_free(pctx);
> +	return ret;
> +}
> +
> +/**
> + * perf_stat_print_metricgroup - Traverse metrics for an event.
> + *
> + * Returns 0 on success, or a negative error code on failure.
> + */
> +static bool is_basic_shadow_metric(const char *name)
> +{
> +	static const char *const basic_metrics[] = {
> +		"insn_per_cycle",   "branch_miss_rate",	      "branch_frequency",
> +		"cycles_frequency", "page_faults_per_second", "migrations_per_second",
> +		"cs_per_second",    "CPUs_utilized",
> +	};
> +	for (size_t i = 0; i < ARRAY_SIZE(basic_metrics); i++) {
> +		if (!strcmp(basic_metrics[i], name))

[Severity: High]
Can this strcmp() trigger a segmentation fault if name is NULL?

In calculate_and_print_metric() just above, metric_name is handled as
potentially NULL. If mexp->metric_name is NULL here, we might crash.

> +			return true;
> +	}
> +	return false;
> +}
> +
> +static int perf_stat_print_metricgroup(const struct perf_stat_config *config,
> +				       const struct perf_stat_print_callbacks *cb, void *outer_ctx,
> +				       struct evsel *evsel, int aggr_idx)
> +{
> +	struct metric_event *me;
> +	struct metric_expr *mexp;
> +	struct rblist *metric_events = &evsel->evlist->metric_events;
> +	int ret;
> +
> +	me = metricgroup__lookup(metric_events, evsel, false);
> +	if (me == NULL)
> +		return 0;
> +
> +	list_for_each_entry(mexp, &me->head, nd) {
> +		if (!config->metric_only &&
> +		    (!evsel->default_metricgroup || evsel->default_show_events)) {
> +			if (!is_basic_shadow_metric(mexp->metric_name))
> +				continue;

[Severity: High]
Does this logic erroneously filter out all custom user metrics?

When a user specifies a custom metric, evsel->default_metricgroup is false.
Unless running in --metric-only mode, the condition evaluates to true, and
is_basic_shadow_metric() will return false for the custom metric, causing the
loop to drop it entirely.

> +		}
> +
> +		ret = calculate_and_print_metric(config, cb, outer_ctx, mexp, evsel, aggr_idx);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
[ ... ]
> +int perf_stat__print_cb(struct evlist *evlist, const struct perf_stat_config *config,
> +			const struct target *target __maybe_unused,
> +			const struct timespec *ts __maybe_unused, int argc __maybe_unused,
> +			const char **argv __maybe_unused,
> +			const struct perf_stat_print_callbacks *cb, void *ctx)
> +{
[ ... ]
> +		if (config->aggr_map) {
> +			cpu_aggr_map__for_each_idx(aggr_idx, config->aggr_map) {
> +				evlist__for_each_entry(evlist, counter) {
[ ... ]
> +					if (cb->print_event) {
> +						double stdev_pct = 0.0;
> +
> +						if (ps && ps->res_stats.n > 1) {
> +							stdev_pct = rel_stddev_stats(
> +								stddev_stats(&ps->res_stats), val);
> +						}
> +						ret = cb->print_event(ctx, config, counter,
> +								      aggr_idx, val, ena, run,
> +								      stdev_pct);

[Severity: Medium]
Will this omission of should_skip_zero_counter() logic result in log spam?

The loop iterates over all counters and unconditionally calls print_event()
for them, even if they have zero counts and shouldn't be printed for this CPU
map, which could break legacy output expectations by printing irrelevant uncore
events.

> +						if (ret)
> +							goto out;
> +					}
[ ... ]
> diff --git a/tools/perf/util/stat-print.h b/tools/perf/util/stat-print.h
> new file mode 100644
> index 0000000000000..a86414f325841
> --- /dev/null
> +++ b/tools/perf/util/stat-print.h
> @@ -0,0 +1,133 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __PERF_STAT_PRINT_H
> +#define __PERF_STAT_PRINT_H
> +
> +#include <linux/types.h>
> +
> +#include "stat.h"
> +
> +#define CNTR_NOT_SUPPORTED "<not supported>"
> +#define CNTR_NOT_COUNTED "<not counted>"
> +
> +struct evlist;
> +struct perf_stat_config;
> +struct target;
> +struct timespec;

[Severity: Low]
Is this manual forward declaration of struct timespec going to cause build
failures on musl libc systems?

According to POSIX compatibility guidelines for musl, standard libc types
should not be forward declared manually. Instead, the appropriate header
should be explicitly included.

> +struct evsel;
> +struct aggr_cpu_id;

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