Re: [PATCH v5 2/2] perf tests: mmap-basic: fix user rdpmc detection logic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] perf: System state leak when encountering unexpected user read state
- [Medium] perf: Logic bug in user_read_file_op() overwrites read failure error code
- [Medium] perf tests: NULL pointer dereference in perf_evsel__delete() on allocation failure
- [Low] perf: Missing POSIX header <unistd.h> for musl compatibility
- [Low] perf: TEST_FAIL is unconditionally overwritten by TEST_SKIP
--

commit 5162e68ee94e9013764b1ff51c5530feccced06c
Author: James Clark <james.clark@linaro.org>

perf tests: mmap-basic: fix user rdpmc detection logic

This commit modifies the mmap-basic perf test to use
/proc/sys/kernel/perf_user_access as a fallback for user counter access.
It tracks and restores the user read state, checks constraints for
different architectures, and handles file permission edge cases.

> diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
> index 208cd889f23db..2c3276e82da8f 100644
> --- a/tools/perf/tests/mmap-basic.c
> +++ b/tools/perf/tests/mmap-basic.c
> @@ -12,6 +12,8 @@
>  #include <perf/evlist.h>
>  #include <perf/mmap.h>
>  
> +#include <tools/config.h>
> +
>  #include "cpumap.h"

[Severity: Low]
Does this file need to explicitly include <unistd.h>? The patch adds calls
to POSIX file I/O operations like read, write, close, and lseek in this file.
Without direct inclusion, this might fail to compile on systems using musl
libc.

[ ... ]

> +static enum user_read_state user_read_file_op(int fd, enum user_read_state enabled)
>  {
>  	char buf[2] = {0, '\n'};
> +	ssize_t len = read(fd, buf, 1);
> +	enum user_read_state old_user_read = USER_READ_ERR_NO_FILE;
>  
> +	if (len <= 0)
>  		pr_debug("%s read failed\n", __func__);
> +	old_user_read = buf[0];

[Severity: Medium]
If read() fails and len is less than or equal to zero, buf[0] retains its
initialized value of 0. By unconditionally assigning old_user_read = buf[0],
does this overwrite the -1 error state (USER_READ_ERR_NO_FILE) initialized
above?

Returning 0 on error seems to trigger an unexpected state failure later on
in the test.

[ ... ]

> @@ -256,8 +284,28 @@ static int test_stat_user_read(u64 event, enum user_read_state enabled)
>  		bool mapped = false, opened = false, rdpmc_supported;
>  		struct perf_counts_values counts = { .val = 0 };
>  
> +		if (__is_defined(__aarch64__) && enabled == USER_READ_ENABLED)
> +			attr.config1 = 0x2;  /* Request user access */
>  
>  		pr_debug("User space counter reading for PMU %s\n", pmu->name);
> +
> +		if (saved_user_read_state == USER_READ_ERR_PERM) {
> +			pr_debug("User space counter reading for PMU %s [Failed to get user read state]\n",
> +				 pmu->name);
> +			ret = TEST_SKIP;
> +			continue;
> +		}

[Severity: Low]
If an earlier iteration of the PMU loop encountered a failure and set
ret = TEST_FAIL, does setting ret = TEST_SKIP unconditionally here overwrite
the previous failure state? 

This could mask test failures on heterogenous systems where multiple core
PMUs are tested.

> +
> +		/* Check value is in the expected range */
> +		if (saved_user_read_state != USER_READ_ERR_NO_FILE &&
> +		    (saved_user_read_state < USER_READ_DISABLED ||
> +		     saved_user_read_state >= USER_READ_MAX)) {
> +			pr_err("User space counter reading for PMU %s [Unexpected current state %d]\n",
> +				pmu->name, saved_user_read_state);
> +			ret = TEST_FAIL;
> +			continue;
> +		}

[Severity: High]
By using continue here, we bypass the loop's cleanup block and the call
to set_user_read(pmu, saved_user_read_state) at the very end of the loop.

Does this leak the state modification and leave user-space counter reading
permanently enabled or modified on the system?

[ ... ]

>  		/* Make the evsel. */
>  		evsel = perf_evsel__new(&attr);
>  		if (!evsel) {
>  			pr_err("User space counter reading for PMU %s [Failed to allocate evsel]\n",
>  				pmu->name);
>  			ret = TEST_FAIL;
>  			goto cleanup;
>  		}

[ ... ]

> +cleanup:
> +		if (mapped)
> +			perf_evsel__munmap(evsel);
> +		if (opened)
> +			perf_evsel__close(evsel);
> +		perf_evsel__delete(evsel);

[Severity: Medium]
If perf_evsel__new() fails and returns NULL, we jump directly to cleanup
and call perf_evsel__delete(evsel) with a NULL pointer. 

Looking at perf_evsel__exit(evsel) inside perf_evsel__delete, it
dereferences evsel immediately without checking for NULL:

tools/lib/perf/evsel.c:perf_evsel__exit() {
    assert(evsel->fd == NULL);
    ...
    perf_cpu_map__put(evsel->cpus);
}

Will this result in a segmentation fault if perf_evsel__new() fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-rdpmc-detection-logic-v5-0-b25c9d49baa1@linaro.org?part=2