|SUCCESS| [v2,5/5] test/pmu: improve error handling and enable test

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163561

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-05

# Review of PMU Patch Series

Commit: 20260505053023.3854665-1-rkudurumalla at marvell.com
Author: Rakesh Kudurumalla <rkudurumalla at marvell.com>
Subject: [PATCH v2 1/5] trace: change scope of conditional block

This patch modifies the conditional compilation block structure in the
trace point header to allow struct definitions outside ALLOW_EXPERIMENTAL_API.

> diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
> index c0ebf1ce13..394b2619c5 100644
> --- a/lib/eal/include/rte_trace_point.h
> +++ b/lib/eal/include/rte_trace_point.h
> @@ -289,7 +289,6 @@ int __rte_trace_point_register(rte_trace_point_t *trace, const char *name,
>  #ifndef __DOXYGEN__
>  
>  #ifndef _RTE_TRACE_POINT_REGISTER_H_
> -#ifdef ALLOW_EXPERIMENTAL_API
>  
>  #define __RTE_TRACE_EVENT_HEADER_ID_SHIFT (48)
>  
> @@ -314,6 +313,8 @@ struct __rte_trace_header {
>  	uint8_t mem[];
>  };
>  
> +#ifdef ALLOW_EXPERIMENTAL_API
> +

Can this change break applications that include this header without
defining ALLOW_EXPERIMENTAL_API? The struct __rte_trace_header is now
visible without the experimental API flag.

[ ... ]

---

Commit: 20260505053023.3854665-3-rkudurumalla at marvell.com
Author: Rakesh Kudurumalla <rkudurumalla at marvell.com>
Subject: [PATCH v2 3/5] trace: add PMU

This patch adds PMU tracing support with a dedicated tracepoint.

> diff --git a/lib/eal/common/eal_common_trace_pmu.c b/lib/eal/common/eal_common_trace_pmu.c
> new file mode 100644
> index 0000000000..7c83ef11e1
> --- /dev/null
> +++ b/lib/eal/common/eal_common_trace_pmu.c

[ ... ]

> +void
> +trace_pmu_args_apply(const char *arg)
> +{
> +	static bool once;
> +
> +	if (!once) {
> +		if (rte_pmu_init())
> +			return;
> +		once = true;
> +	}
> +
> +	rte_pmu_add_events_by_pattern(arg);
> +}

Does this code leak resources if rte_pmu_add_events_by_pattern() fails?
Should error handling propagate failures back to the caller?

> +void
> +trace_pmu_args_free(void)
> +{
> +	rte_pmu_fini();
> +}

Can trace_pmu_args_free() be called when trace_pmu_args_apply() never
succeeded? Does rte_pmu_fini() handle uninitialized state correctly?

[ ... ]

> diff --git a/lib/pmu/pmu.c b/lib/pmu/pmu.c

[ ... ]

> @@ -417,13 +419,77 @@ rte_pmu_add_event(const char *name)
>  	return event->index;
>  }
>  
> +static int
> +add_events(const char *pattern)
> +{
> +	char *token, *copy, *tmp;
> +	int ret = 0;
> +
> +	copy = strdup(pattern);
> +	if (copy == NULL)
> +		return -ENOMEM;
> +
> +	token = strtok_r(copy, ",", &tmp);
> +	while (token) {
> +		ret = rte_pmu_add_event(token);
> +		if (ret < 0)
> +			break;
> +
> +		token = strtok_r(NULL, ",", &tmp);
> +	}
> +
> +	free(copy);
> +
> +	return ret >= 0 ? 0 : ret;
> +}

Does add_events() need to clean up partially added events on failure?
If rte_pmu_add_event() fails midway through the list, do earlier events
remain registered?

> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmu_add_events_by_pattern, 25.11)
> +int
> +rte_pmu_add_events_by_pattern(const char *pattern)
> +{
> +	regmatch_t rmatch;
> +	char buf[BUFSIZ];
> +	unsigned int num;
> +	regex_t reg;
> +	int ret;
> +
> +	/* events are matched against occurrences of e=ev1[,ev2,..] pattern */
> +	ret = regcomp(&reg, "e=([_[:alnum:]-],?)+", REG_EXTENDED);
> +	if (ret) {
> +		PMU_LOG(ERR, "Failed to compile event matching regexp");
> +		return -EINVAL;
> +	}
> +
> +	for (;;) {
> +		if (regexec(&reg, pattern, 1, &rmatch, 0))
> +			break;
> +
> +		num = rmatch.rm_eo - rmatch.rm_so;
> +		if (num > sizeof(buf))
> +			num = sizeof(buf);
> +
> +		/* skip e= pattern prefix */
> +		memcpy(buf, pattern + rmatch.rm_so + 2, num - 2);
> +		buf[num - 2] = '\0';

Can this code overflow buf[]? Does the truncation at sizeof(buf)
account for the -2 offset and NUL terminator when copying?

> +		ret = add_events(buf);
> +		if (ret)
> +			break;
> +
> +		pattern += rmatch.rm_eo;
> +	}
> +
> +	regfree(&reg);
> +
> +	return ret;
> +}

Does rte_pmu_add_events_by_pattern()->add_events() need cleanup in the
error path? If pattern contains "e=ev1,ev2 e=ev3,ev4" and ev3 fails,
do ev1 and ev2 remain registered?

[ ... ]

> @@ -440,11 +506,11 @@ rte_pmu_init(void)
>  
>  	TAILQ_INIT(&rte_pmu.event_list);
>  	rte_pmu.initialized = 1;
> +
> +	return 0;
>  out:
> -	if (ret) {
> -		free(rte_pmu.name);
> -		rte_pmu.name = NULL;
> -	}
> +	free(rte_pmu.name);
> +	rte_pmu.name = NULL;

Is the 'out' label now unreachable? The function returns early at line
510.

[ ... ]

---

Commit: 20260505053023.3854665-4-rkudurumalla at marvell.com
Author: Rakesh Kudurumalla <rkudurumalla at marvell.com>
Subject: [PATCH v2 4/5] lib/pmu: fix out-of-bound access

This patch fixes buffer overflow issues in PMU code.

> diff --git a/lib/pmu/pmu.c b/lib/pmu/pmu.c

[ ... ]

> @@ -126,7 +126,7 @@ get_event_config(const char *name, uint64_t config[3])
>  	if (fp == NULL)
>  		return -errno;
>  
> -	ret = fread(buf, 1, sizeof(buf), fp);
> +	ret = fread(buf, 1, sizeof(buf) - 1, fp);
>  	if (ret == 0) {
>  		fclose(fp);
>  

Does get_event_config() NUL-terminate buf after the fread()? Without
explicit termination, subsequent use of buf could read uninitialized
memory.

[ ... ]

> @@ -419,6 +419,45 @@ rte_pmu_add_event(const char *name)
>  	return event->index;
>  }
>  
> +static int
> +check_perf_permissions(void)
> +{
> +	const char *paranoid_path = "/proc/sys/kernel/perf_event_paranoid";
> +	int level, ret;
> +	FILE *fp;
> +
> +	/* Check if user is root */
> +	if (getuid() == 0)
> +		return 0;  /* Root has access */
> +
> +	fp = fopen(paranoid_path, "r");
> +	if (!fp)
> +		return -ENOENT;  /* File doesn't exist */
> +
> +	ret = fscanf(fp, "%d", &level);
> +	fclose(fp);

Does check_perf_permissions() need to handle fscanf() reading partial
data? Should level be initialized before fscanf()?

[ ... ]

> @@ -492,10 +531,21 @@ rte_pmu_init(void)
>  	if (rte_pmu.initialized && ++rte_pmu.initialized)
>  		return 0;
>  
> +	/* Check permissions first */
> +	ret = check_perf_permissions();
> +	if (ret == -EACCES) {
> +		PMU_LOG(ERR, "Insufficient privileges for PMU access (check perf_event_paranoid)");
> +		return -EACCES;
> +	}
> +	if (ret == -ENOENT) {
> +		PMU_LOG(ERR, "Cannot access perf_event_paranoid file");
> +		return -ENODEV;
> +	}

Does rte_pmu_init() need to handle check_perf_permissions() returning
other error codes? Only -EACCES, -ENOENT, and 0 are explicitly checked.

[ ... ]

> diff --git a/lib/pmu/pmu_arm64.c b/lib/pmu/pmu_arm64.c
> index 2c40b5f702..f3a817b42f 100644
> --- a/lib/pmu/pmu_arm64.c
> +++ b/lib/pmu/pmu_arm64.c
> @@ -24,12 +24,13 @@ read_attr_int(const char *path, int *val)
>  	if (fd == -1)
>  		return -errno;
>  
> -	ret = read(fd, buf, sizeof(buf));
> +	ret = read(fd, buf, sizeof(buf) - 1);
>  	if (ret == -1) {
>  		close(fd);
>  
>  		return -errno;
>  	}
> +	buf[ret] = '\0';

Does read_attr_int() need to validate ret is non-negative before using
it as an array index?

[ ... ]