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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Silent fallback on memory allocation failure for `evsel->nvme.initial_values` leads to reporting absolute lifetime metric values instead of interval deltas.
- [High] Passing a potentially NULL `term->config` to `asprintf` with `%s` causes undefined behavior or crashes on certain libcs.
- [Medium] The PMU falsely advertises raw configuration encodings that it actively rejects.
- [Medium] Temperature conversion underflows for sub-zero Celsius values.
--

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

perf pmu: Implement tool-provided NVMe PMU

Add a tool-provided PMU for NVMe devices using libnvme. This PMU exposes
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..e2391726d76e7
> --- /dev/null
> +++ b/tools/perf/util/nvme_pmu.c
[ ... ]
> +static int nvme_pmu__config_term(const struct nvme_pmu *nvm __maybe_unused,
> +				 struct perf_event_attr *attr,
> +				 struct parse_events_term *term,
> +				 struct parse_events_error *err)
> +{
> +	if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER) {

[Severity: Medium]
In nvme_pmu__for_each_event(), the encoding description is formatted as
config=0x%lx. However, here in nvme_pmu__config_term() it seems only
PARSE_EVENTS__TERM_TYPE_USER is accepted.

Would this incorrectly reject valid config terms that match the advertised
encoding if a user manually specifies them? For example, if they were to
use a command like: perf stat -e nvme_nvme0/config=0x123/

> +		size_t i;
> +
> +		for (i = 0; i < ARRAY_SIZE(nvme_events); i++) {
> +			if (!strcasecmp(term->config, nvme_events[i].name)) {
> +				attr->config = nvme_events[i].config;
> +				return 0;
> +			}
> +		}
> +	}
> +	if (err) {
> +		char *err_str;
> +
> +		parse_events_error__handle(err, term->err_val,
> +					asprintf(&err_str,
> +						"unexpected nvme event term (%s) %s",
> +						parse_events__term_type_str(term->type_term),
> +						term->config) < 0

[Severity: High]
If a user specifies a non-string config term like name=myname or a bare
number, the parser might create a term with a NULL term->config string key.

Does passing a potentially NULL term->config to asprintf() with %s cause
undefined behavior or a crash on certain libc implementations (like musl)?

> +					? strdup("unexpected nvme event term")
> +					: err_str,
> +					NULL);
> +	}
> +	return -EINVAL;
> +}
[ ... ]
> +int nvme_pmu__check_alias(struct parse_events_terms *terms, struct perf_pmu_info *info,
> +			  struct parse_events_error *err)
> +{
[ ... ]
> +	if (err) {
> +		char *err_str;
> +
> +		parse_events_error__handle(err, term->err_val,
> +					asprintf(&err_str,
> +						"unexpected nvme event term (%s) %s",
> +						parse_events__term_type_str(term->type_term),
> +						term->config) < 0

[Severity: High]
Similarly, this error formatting path also appears to suffer from the same
potential NULL dereference of term->config.

> +					? strdup("unexpected nvme event term")
> +					: err_str,
> +					NULL);
> +	}
> +	return -EINVAL;
> +}
[ ... ]
> +static int nvme_pmu__read_val(int fd, uint64_t config, uint64_t *val)
> +{
[ ... ]
> +	if (log_type == NVME_LOG_SMART) {
> +		if (offset + size > sizeof(struct nvme_smart_log))
> +			return -EINVAL;
> +		if (nvme_get_log_smart(fd, NVME_NSID_ALL, true, (struct nvme_smart_log *)buf) != 0)
> +			return -EINVAL;
> +
> +		if (offset == offsetof(struct nvme_smart_log, temperature)) {
> +			uint64_t kelvin = ((struct nvme_smart_log *)buf)->temperature[0] |
> +					  (((struct nvme_smart_log *)buf)->temperature[1] << 8);
> +			*val = (kelvin * 1000) - 273150;

[Severity: Medium]
Does this temperature conversion risk an unsigned integer underflow for
sub-zero Celsius values?

If the NVMe SMART temperature in Kelvin drops below 0 degrees Celsius
(less than 273 K), kelvin * 1000 would be less than 273150. Since *val is
assigned to a uint64_t, this underflow could result in a massive value
being reported instead of a valid negative temperature.

> +			return 0;
> +		}
> +	} else if (log_type == NVME_LOG_ENDURANCE) {
[ ... ]
> +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->nvme.initial_values) {
> +		int max_cpus = evsel->core.cpus ? perf_cpu_map__nr(evsel->core.cpus) : 1;
> +
> +		evsel->nvme.initial_values = xyarray__new(max_cpus, nthreads, sizeof(uint64_t));

[Severity: High]
What happens if xyarray__new() fails and returns NULL here?

It looks like the allocation failure is silently ignored, and the open
operation succeeds with evsel->nvme.initial_values left as NULL.
Later in evsel__nvme_pmu_read(), the baseline subtraction defaults to 0.

Could this cause the perf tool to silently report the absolute lifetime
device counter on the first measurement interval instead of the expected
interval deltas? Should this check for allocation failure and return
-ENOMEM instead?

> +	}
> +
> +	for (idx = start_cpu_map_idx; idx < end_cpu_map_idx; idx++) {

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