Re: [PATCH V3 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The implementation is missing the HTM_MAX_SAFE_TARGETS (1024) cap for num_events that was explicitly promised in the commit message.
- [Medium] htm_config_for_cpu conflates a lookup failure with a valid 0 PMU configuration, causing valid auxtrace events to be dropped.
- [Low] Assigning write() return value to a size_t variable masks the actual error code and produces misleading log messages.
- [Low] Missing explicit inclusion of POSIX standard headers required for musl libc compatibility.
--

--- Patch [6]: [PATCH V3 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records ---
commit b319ae58a675d493de8d8d9853fc6d781771f6b8
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records

powerpc_htm_process_auxtrace_info() reads the (cpu, attr.config) pairs
stored by htm_info_fill() at record time and builds a cpu_configs[]
table.  This lets process_auxtrace_event() map each AUX buffer, which
carries event->auxtrace.cpu, to the correct (node, chip, core) target
and write the raw trace data to htm.bin.nX.pX.cX immediately.

[ ... ]
> - Cap num_events at HTM_MAX_SAFE_TARGETS (1024) to prevent size_t
> truncation on 32-bit platforms when passed to calloc().

[Severity: Medium]
This isn't a bug, but the commit message mentions capping num_events at 1024
(HTM_MAX_SAFE_TARGETS).

Looking at powerpc_htm_process_auxtrace_info(), the arrays are allocated
using num_events directly:

tools/perf/util/powerpc-htm.c:powerpc_htm_process_auxtrace_info() {
    ...
    htm->nr_targets   = (int)num_events;
    htm->cpu_configs  = calloc((size_t)num_events, sizeof(*htm->cpu_configs));
    ...
}

Has the 1024 maximum cap and truncation prevention logic been omitted here?

> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
> index 952ed32997d9a..1d1b4302e2e4a 100644
> --- a/tools/perf/util/powerpc-htm.c
> +++ b/tools/perf/util/powerpc-htm.c
> @@ -12,9 +12,14 @@
>  #include "util/session.h"
>  #include "color.h"
>  #include "powerpc-htm.h"
> +#include <errno.h>
>  #include "debug.h"
>  #include "sample.h"
>  
> +#include <linux/unaligned.h>
> +
> +struct perf_session;

[Severity: Low]
Are the standard POSIX headers missing here?

The code uses functions like open(), write(), lseek(), snprintf(), strerror(),
calloc(), and free().

Without explicitly including fcntl.h, unistd.h, stdio.h, string.h, and
stdlib.h, could this fail to compile on systems using musl libc due to
relying on implicit header inclusions?

[ ... ]
> +/*
> + * Look up attr.config by the CPU number carried in event->auxtrace.cpu.
> + * Returns 0 if not found.
> + */
> +static u64 htm_config_for_cpu(struct powerpc_htm *htm, int cpu)
> +{
> +	int i;
> +
> +	for (i = 0; i < htm->nr_cpu_configs; i++) {
> +		if (htm->cpu_configs[i].cpu == cpu)
> +			return htm->cpu_configs[i].config;
> +	}
> +	return 0;
> +}

[Severity: Medium]
Can a valid configuration evaluate to 0?

If the HTM PMU is configured with node=0, chip=0, core=0 and no other flags,
attr.config could legitimately be 0.

If it returns 0, the caller powerpc_htm_process_auxtrace_event() checks:

tools/perf/util/powerpc-htm.c:powerpc_htm_process_auxtrace_event() {
    ...
    ev_config = htm_config_for_cpu(htm, (int)event->auxtrace.cpu);
    if (!ev_config) {
        ...
        return 0;
    }
    ...
}

Would this cause valid auxtrace events to be dropped by conflating a lookup
failure with a valid 0 PMU configuration?

[ ... ]
> +static int write_htm(struct powerpc_htm *htm, void *data, size_t size,
> +		     u32 node, u32 chip, u32 core, int mem_maps)
> +{
> +	u32 target_key = htm_pack_target(node, chip, core);
> +	char target_file[128];
> +	size_t written;
> +	int flags;
> +	int fd;
[ ... ]
> +		written = write(fd, data, payload);
> +		close(fd);
> +
> +		if (written != payload) {
> +			pr_err("Failed to write memory config: expected %zu bytes, wrote %zu\n",
> +			       payload, written);
> +			return -EIO;
> +		}
> +
> +		return 0;
> +	}
> +
> +	/* AUX bus-trace path */
> +	snprintf(target_file, sizeof(target_file),
> +		 "htm.bin.n%d.p%d.c%d", node, chip, core);
> +	flags |= htm_target_seen(htm->htm_bin_targets,
> +				 &htm->nr_htm_bin_targets,
> +				 htm->nr_targets,
> +				 target_key) ? O_APPEND : O_TRUNC;
> +	fd = open(target_file, flags, 0644);
> +	if (fd == -1) {
> +		pr_err("Failed to open %s: %s\n", target_file, strerror(errno));
> +		return -errno;
> +	}
> +
> +	written = write(fd, data, size);
> +	close(fd);
> +
> +	if (written != size) {
> +		pr_err("Failed to write htm trace data: expected %zu bytes, wrote %zu\n",
> +		       size, written);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
Does this code assign the signed return value of write() to a size_t variable?

If write() fails and returns -1, it will be assigned to size_t written,
wrapping it to SIZE_MAX.

While the error is still caught by the written != payload and written != size
checks, will this result in logging an impossibly large value for bytes written
and mask the actual error code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725070747.81435-1-atrajeev@linux.ibm.com?part=6