Re: [PATCH] perf trace: Add --bitmask-list option and correct cpumask formatting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The cpumask byte parsing and bitmask rendering ignores the endianness and word size of the trace payload, breaking cross-platform analysis.
--

--- Patch [1]: [PATCH] perf trace: Add --bitmask-list option and correct cpumask formatting ---
commit 3ae8add0cf042fd56dee83eecdf3dd3e5eeef32c
Author: Aaron Tomlin <atomlin@atomlin.com>

perf trace: Add --bitmask-list option and correct cpumask formatting

This commit corrects the parsing of dynamic non-array fields like cpumask_t
in perf trace to resolve raw pointers properly. It also updates the display
formatting for cpumasks to show zero-suppressed hex strings by default and
adds a --bitmask-list option for human-readable bitmask formatting.

Fixes: c5e006cdbd27 ("perf trace: Support tracepoint dynamic char arrays")

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index ba0f8749fc7d7..cd75ac4e51693 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3238,17 +3239,65 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct perf_sample *

[ ... ]

> +			if (trace->bitmask_list) {
> +				if (syscall_arg.len > 0) {
> +					unsigned long *mask = zalloc(BITS_TO_LONGS(syscall_arg.len * 8) * sizeof(unsigned long));
> +					if (!mask) {
> +						pr_err("Problem processing %s field, skipping...\n", field->name);
> +						printed = saved_printed;
> +						continue;
> +					}
> +					memcpy(mask, (void *)val, syscall_arg.len);
> +					printed += bitmap_scnprintf(mask, syscall_arg.len * 8,
> +								    bf + printed, size - printed);

[Severity: Medium]
Will this memcpy() and bitmask formatting break cross-platform analysis if the
perf.data file was recorded on an architecture with a different endianness or
word size than the host?

It appears we are doing a direct copy of the target payload bytes into a
host-native unsigned long array without utilizing evsel->needs_swap.

Could this result in garbled or reversed cpumask values when viewing trace
data from a machine with different architectural characteristics?

> +					free(mask);
> +				}
> +			} else {
> +				unsigned char *b = (unsigned char *)val;
> +				int i;
> +				bool skip_zero = true;
> +
> +				printed += scnprintf(bf + printed, size - printed, "0x");
> +				/* Print in little-endian order */
> +				for (i = syscall_arg.len - 1; i >= 0; i--) {

[Severity: Medium]
Does this backward loop assume the trace data is unconditionally little-endian?

If the trace data comes from a big-endian architecture, will this hardcoded
assumption fail to print the values correctly?

> +					if (skip_zero && b[i] == 0 && i > 0)
> +						continue;
> +					if (skip_zero) {
> +						printed += scnprintf(bf + printed, size - printed, "%x", b[i]);
> +						skip_zero = false;
> +					} else {
> +						printed += scnprintf(bf + printed, size - printed, "%02x", b[i]);
> +					}
> +				}
> +			}
> +			continue;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714162947.214270-1-atomlin@atomlin.com?part=1