Re: [PATCH v1 14/26] perf stat: Use generic hashmap for per_pkg_mask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] perf stat: static variable corruption in
  process_counter_values() [1]
--

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

perf stat: Use generic hashmap for per_pkg_mask

This commit converts evsel::per_pkg_mask to use the typed hashmap wrapper
DECLARE_HASHMAP(per_pkg_mask_map, uint64_t *, bool) and updates its
allocation, iteration, and cleanup callers.

> diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
> index 097985da3dbb8..c502f4928f9d2 100644
> --- a/tools/perf/util/stat.c
> +++ b/tools/perf/util/stat.c

[ ... ]

> @@ -364,7 +364,7 @@ static int check_per_pkg(struct evsel *counter, struct perf_counts_values *vals,
>  		*skip = true;
>  		free(key);
>  	} else {
> -		ret = hashmap__add(mask, key, 1);
> +		ret = hashmap__add(mask, key, true);
>  		if (ret)
>  			free(key);
>  	}

[Severity: Medium]
This problem wasn't introduced by this patch, but can setting *skip = true
here cause evsel__compute_deltas() to corrupt the static zero struct in
process_counter_values() [1]?

When check_per_pkg() sets skip, process_counter_values() points count at a
local static struct perf_counts_values zero and passes it to
evsel__compute_deltas():

tools/perf/util/stat.c:process_counter_values() {
	static struct perf_counts_values zero;
	bool skip = false;

	if (check_per_pkg(evsel, count, cpu_map_idx, &skip)) {
		pr_err("failed to read per-pkg counter\n");
		return -1;
	}

	if (skip)
		count = &zero;

	if (!evsel->snapshot)
		evsel__compute_deltas(evsel, cpu_map_idx, thread, count);
    ...
}

evsel__compute_deltas() subtracts the previous interval's raw counts from
*count in place:

tools/perf/util/evsel.c:evsel__compute_deltas() {
    ...
	tmp = *perf_counts(evsel->prev_raw_counts, cpu_map_idx, thread);
	*perf_counts(evsel->prev_raw_counts, cpu_map_idx, thread) = *count;

	count->val = count->val - tmp.val;
	count->ena = count->ena - tmp.ena;
	count->run = count->run - tmp.run;
}

If a CPU had non-zero counts in a previous interval and is later skipped,
subtracting tmp from count underflows the static zero struct in place.
Every subsequent skipped CPU then reuses the corrupted static struct,
producing bogus aggregated counter values for the rest of the perf stat run.

[1] https://sashiko.dev/#/bug/linux-d1692581-98d0-4148-a6ee-49bd64e01cec

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