Re: [PATCH v1 2/3] perf pmu: Implement tool-provided NVMe PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unaligned memory access and strict aliasing violation in nvme_pmu__read_val() due to byte-aligned stack array cast to wider types.
- [High] The NVMe PMU implementation hijacks the tool-owned `evsel->priv` field, leading to arbitrary memory corruption and memory leaks.
--

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

perf pmu: Implement tool-provided NVMe PMU

This commit adds a tool-provided PMU for NVMe devices using libnvme to
expose metrics from various NVMe logs as perf events under the
nvme_nvmeX PMUs.

> diff --git a/tools/perf/util/nvme_pmu.c b/tools/perf/util/nvme_pmu.c
> new file mode 100644
> index 0000000000000..17ba758aec59b
> --- /dev/null
> +++ b/tools/perf/util/nvme_pmu.c
[ ... ]
> +static int nvme_pmu__read_val(int fd, uint64_t config, uint64_t *val)
> +{
> +	int log_type = (config >> 24) & 0xFF;
> +	unsigned int size = (config >> 16) & 0xFF;
> +	unsigned int offset = config & 0xFFFF;
> +	uint8_t buf[4096];
> +	uint8_t *p;
[ ... ]
> +	p = buf + offset;
> +	if (size == 16 || size == 8)
> +		*val = le64_to_cpu(*(uint64_t *)p);

[Severity: High]
Does this cast violate C strict aliasing rules and risk unaligned memory
access crashes?

Since buf is a byte-aligned stack array and offset can be an arbitrary
value, p is not guaranteed to be 8-byte aligned. On architectures that
strictly enforce memory alignment, dereferencing an unaligned 64-bit
pointer can cause an immediate bus error (SIGBUS).

Additionally, casting a byte array to a wider type violates strict aliasing,
which is enforced in the tools/ directory with -fstrict-aliasing.

Could we use get_unaligned_le64() here instead?

> +	else if (size == 4)
> +		*val = le32_to_cpu(*(uint32_t *)p);
> +	else if (size == 2)
> +		*val = le16_to_cpu(*(uint16_t *)p);
> +	else if (size == 1)
> +		*val = *(uint8_t *)p;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
[ ... ]
> +int evsel__nvme_pmu_open(struct evsel *evsel,
> +			 struct perf_thread_map *threads,
> +			 int start_cpu_map_idx, int end_cpu_map_idx)
> +{
> +	struct nvme_pmu *nvm = container_of(evsel->pmu, struct nvme_pmu, pmu);
> +	int idx = 0, thread = 0, nthreads, err = 0;
> +	char path[PATH_MAX];
> +
> +	snprintf(path, sizeof(path), "/dev/%s", nvm->dev_name);
> +
> +	nthreads = perf_thread_map__nr(threads);
> +
> +	if (!evsel->priv) {
> +		int max_cpus = evsel->core.cpus ? perf_cpu_map__nr(evsel->core.cpus) : 1;
> +
> +		evsel->priv = xyarray__new(max_cpus, nthreads, sizeof(uint64_t));
> +	}

[Severity: High]
Can this leak the xyarray allocation?

Since evsel->priv is conventionally owned by the specific perf tool executing
the command, allocating an xyarray here does not seem to have a matching
destructor for the PMU, which might leave the memory unfreed when the event
is closed.

> +
> +	for (idx = start_cpu_map_idx; idx < end_cpu_map_idx; idx++) {
> +		for (thread = 0; thread < nthreads; thread++) {
> +			int fd = open(path, O_RDONLY);
> +
> +			FD(evsel, idx, thread) = fd;
> +			if (fd < 0) {
> +				err = -errno;
> +				goto out_close;
> +			}
> +			if (evsel->priv) {
> +				uint64_t *initial_val = xyarray__entry(evsel->priv, idx, thread);
> +
> +				if (nvme_pmu__read_val(fd, evsel->core.attr.config, initial_val))
> +					*initial_val = 0;
> +			}

[Severity: High]
Is there a risk of memory corruption if a tool has already populated
evsel->priv?

If an active tool like perf stat has already set evsel->priv to its own
structure, this code assumes it is an xyarray. The xyarray__entry()
function will read from the tool's struct to compute a pointer offset,
and then nvme_pmu__read_val() will write a 64-bit value to that
invalid address, causing arbitrary heap corruption.

> +		}
> +	}
> +	return 0;

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