Re: [PATCH v2 01/12] perf parse-events: Restrict core PMU bypass to --cputype option

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardcoded `false` for `cputype_filter` in `add_default_events()` breaks parsing of default metrics when `--cputype` is used.

Pre-existing issues:
- [High] In `__parse_events()`, an error during group sorting leads to a memory leak and incorrect success return code.
- [High] In `parse_groups()`, `combined_evlist` is leaked on error paths.
--

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

perf parse-events: Restrict core PMU bypass to --cputype option

This commit introduces a cputype_filter flag to distinguish PMU filtering
initiated by --cputype from --pmu-filter, restricting the core-only PMU
bypass to the former.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index a04466ea3b0a0..2505bc604e532 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
[ ... ]
> @@ -1999,7 +2001,7 @@ static int add_default_events(void)
>  			ret = -1;
>  			goto out;
>  		}
> -		ret = metricgroup__parse_groups(evlist, pmu, "transaction",
> +		ret = metricgroup__parse_groups(evlist, pmu, false, "transaction",

[Severity: High]
Should parse_events_option_args.cputype_filter be passed here instead of
hardcoding false?

If the --cputype option is active, hardcoding false causes
parse_events__filter_pmu() to filter out software and uncore PMU events
required by default metrics (like duration_time). This causes default metric
parsing to fail and can abort perf stat entirely.

[ ... ]
> @@ -2036,7 +2038,7 @@ static int add_default_events(void)
>  		if (!force_metric_only)
>  			stat_config.metric_only = true;
>  
> -		ret = metricgroup__parse_groups(evlist, pmu, "smi",
> +		ret = metricgroup__parse_groups(evlist, pmu, false, "smi",

[Severity: High]
Similarly here, does this hardcoded false break the smi metric when using
the --cputype option?

[ ... ]
> @@ -2073,7 +2075,7 @@ static int add_default_events(void)
>  		}
>  		str[8] = stat_config.topdown_level + '0';
>  		if (metricgroup__parse_groups(evlist,
> -						pmu, str,
> +					      pmu, false, str,

[Severity: High]
Does this hardcoded false break the Topdown metric group parsing when using
the --cputype option?

[ ... ]
> @@ -2112,7 +2114,7 @@ static int add_default_events(void)
>  				ret = -ENOMEM;
>  				break;
>  			}
> -			if (metricgroup__parse_groups(metric_evlist, pmu, default_metricgroup_names[i],
> +			if (metricgroup__parse_groups(metric_evlist, pmu, false, default_metricgroup_names[i],

[Severity: High]
Does this hardcoded false break the standard default metrics when using
the --cputype option?

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index c2ce3e53aaee7..9c9b048838411 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
[ ... ]
> @@ -1476,7 +1480,8 @@ static int parse_groups(struct evlist *perf_evlist,
>  		if (!metric_evlist) {
>  			ret = parse_ids(metric_no_merge, fake_pmu, m->pctx, m->modifier,
>  					m->group_events, tool_events, &m->evlist,
> -					(pmu && strcmp(pmu, "all") == 0) ? NULL : pmu);
> +					(pmu && strcmp(pmu, "all") == 0) ? NULL : pmu,
> +					cputype_filter);
>  			if (ret)
>  				goto out;

[Severity: High]
This is a pre-existing issue, but does this error path leak combined_evlist?

If a subsequent operation in the metric_list loop fails, the function jumps
to the out: label:

tools/perf/util/metricgroup.c:parse_groups() {
    ...
    if (combined_evlist) {
        evlist__splice_list_tail(perf_evlist, &combined_evlist->core.entries);
        evlist__delete(combined_evlist);
    }
    ...
out:
    metricgroup__free_metrics(&metric_list);
    return ret;
}

Since the cleanup for combined_evlist is located above the out: label, it gets
completely skipped, leaking the list and its events.

> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 943569e82b82f..9f64d5197b8a8 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
[ ... ]
> @@ -2288,18 +2291,20 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
>  	return (idx_changed || num_leaders != orig_num_leaders) ? 1 : 0;
>  }
>  
> -int __parse_events(struct evlist *evlist, const char *str, const char *pmu_filter,
> +int __parse_events(struct evlist *evlist, const char *str,
> +		   const char *pmu_filter, bool cputype_filter,
>  		   struct parse_events_error *err, bool fake_pmu,
>  		   bool warn_if_reordered, bool fake_tp)
>  {
>  	struct parse_events_state parse_state = {

[Severity: High]
This is a pre-existing issue, but does __parse_events() leak memory and
falsely report success if group sorting fails?

If parse_events__sort_events_and_fix_groups() returns a negative error code
(e.g., due to ENOMEM), the function returns the successful ret instead of
the error code ret2:

tools/perf/util/parse-events.c:__parse_events() {
    ...
    ret = parse_events__scanner(str, &parse_state);
    ...
    ret2 = parse_events__sort_events_and_fix_groups(&parse_state.list);
    if (ret2 < 0)
        return ret;
    ...
}

Returning here also bypasses the call to evlist__splice_list_tail(), which
causes all dynamically allocated evsel elements in parse_state.list to be
lost and leaked when the list goes out of scope.

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