Re: [PATCH V5 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM

> On 7 Aug 2026, at 8:29 PM, [email protected] wrote:
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Heap buffer overflow in htm_info_fill() due to undersized PERF_RECORD_AUXTRACE_INFO allocation for HTM evsels covering multiple CPUs.
> - [Low] Missing explicit inclusion of `<string.h>` for `strcmp()`, violating musl libc compatibility guidelines.
> --
> 
> --- Patch [2]: [PATCH V5 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM ---
> commit 36cc90227b9052ae32b72e6b7df67e4e6bc7ac1e
> Author: Athira Rajeev <[email protected]>
> 
> tools/perf: Add AUXTRACE recording support for powerpc HTM
> 
> Add powerpc perf tool support for the HTM PMU AUXTRACE recording path.
> Introduce htm_recording_init() and the associated auxtrace callbacks so
> perf record can create PERF_RECORD_AUXTRACE records for HTM data.
> 
>> diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc/util/htm.c
>> new file mode 100644
>> index 0000000000000..d2fdef4880320
>> --- /dev/null
>> +++ b/tools/perf/arch/powerpc/util/htm.c
>> @@ -0,0 +1,185 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * HTM AUX tracing support
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/types.h>
>> +#include <linux/string.h>
>> +#include <linux/zalloc.h>
> 
> [Severity: Low]
> This isn't a bug, but this file uses strcmp() which is declared in <string.h>.
> Does this code need to explicitly include <string.h> to avoid relying on
> implicit inclusion, which might cause build failures in strictly conformant
> musl libc environments?
> 
> [ ... ]
> 
>> +/* Count htm evsels in the evlist */
>> +static int htm_nr_events(struct evlist *evlist)
>> +{
>> + struct evsel *pos;
>> + int n = 0;
>> +
>> + evlist__for_each_entry(evlist, pos) {
>> + if (!strcmp(evsel__pmu_name(pos), "htm"))
>> + n++;
>> + }
>> + return n;
>> +}
>> +
>> +static size_t htm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
>> + struct evlist *evlist)
>> +{
>> + return HTM_AUXTRACE_PRIV_SIZE(htm_nr_events(evlist));
>> +}
> 
> [ ... ]
> 
>> +/*
>> + * Fill the PERF_RECORD_AUXTRACE_INFO private data with:
>> + *   priv[POWERPC_HTM_PMU_TYPE]   = pmu->type of the first htm evsel
>> + *   priv[POWERPC_HTM_NUM_EVENTS] = total number of HTM CPU entries
>> + *   priv[POWERPC_HTM_EVENT_DATA + n*2]     = CPU number for nth entry
>> + *   priv[POWERPC_HTM_EVENT_DATA + n*2 + 1] = attr.config for nth entry
>> + *
>> + * One entry is written per CPU in each evsel's cpu map.  An evsel opened
>> + * with -C 0,1,2 contributes three entries (one per CPU), each carrying
>> + * the same attr.config.  The decode side uses event->auxtrace.cpu to look
>> + * up the matching config and derive (node, chip, core) for the output
>> + * file name.
>> + */
>> +static int
>> +htm_info_fill(struct auxtrace_record *itr,
>> +      struct perf_session *session,
>> +      struct perf_record_auxtrace_info *auxtrace_info,
>> +      size_t priv_size)
>> +{
>> + struct htm_recording *htm_r = container_of(itr, struct htm_recording, itr);
>> + struct evlist *evlist = session->evlist;
>> + struct evsel *pos;
>> + int n = 0;
>> + int expected_n = htm_nr_events(evlist);
>> +
>> + if (priv_size != HTM_AUXTRACE_PRIV_SIZE(expected_n))
>> + return -EINVAL;
>> +
>> + auxtrace_info->type = PERF_AUXTRACE_POWERPC_HTM;
>> + auxtrace_info->priv[POWERPC_HTM_PMU_TYPE] = htm_r->evsel->core.attr.type;
>> + auxtrace_info->priv[POWERPC_HTM_NUM_EVENTS] = expected_n;
>> +
>> + evlist__for_each_entry(evlist, pos) {
>> + struct perf_cpu_map *cpus;
>> + int i, nr;
>> +
>> + if (strcmp(evsel__pmu_name(pos), "htm"))
>> + continue;
>> +
>> + /*
>> + * Emit one (cpu, config) entry for every CPU in this evsel's
>> + * map.  perf record -C 0,1,2 creates one evsel with a
>> + * three-entry cpu map; each CPU gets its own AUX buffer and
>> + * must be individually mapped so the decoder can match
>> + * event->auxtrace.cpu to the correct (node, chip, core).
>> + */
>> + cpus = evsel__cpus(pos);
>> + nr = cpus ? perf_cpu_map__nr(cpus) : 0;
>> +
>> + if (nr > 0) {
>> + for (i = 0; i < nr; i++) {
>> + int cpu = perf_cpu_map__cpu(cpus, i).cpu;
>> +
>> + auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + n * 2]     = cpu;
>> + auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + n * 2 + 1] =
>> + pos->core.attr.config;
>> + n++;
> 
> [Severity: High]
> Does this code overflow the auxtrace_info->priv[] array when an HTM evsel
> covers multiple CPUs?
> 
> The auxtrace_info->priv[] buffer size is calculated by htm_nr_events() above,
> which only counts the total number of HTM evsels. If a user runs perf record
> with a CPU list (e.g., -C 0,1,2), there might be only 1 evsel, resulting in
> space allocated for a single (cpu, config) pair.
> 
> However, this loop iterates over the CPU map and writes a pair for every CPU
> in that evsel (e.g., 3 CPUs), incrementing n past the allocated size.
Thanks for the review
Will address this in next version

Thanks
Athira
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2