Re: [PATCH 4/5] selftests/perf_events: add userspace AUX regression test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests/perf_events: spurious failure on strict perf_event_paranoid
- [Medium] selftests/perf_events: reading aux_head before disable causes spurious SKIP
- [Medium] selftests/perf_events: inconsistent PMU config rejection handling
- [Low] selftests/perf_events: missing -latomic causes 32-bit build failure
--

commit d2d3116ff9e5d31817ce92f2b014df0cd0508f17
Author: Lian Wang <lianux.mm@gmail.com>

selftests/perf_events: add userspace AUX regression test

This commit adds a C program to test the userspace AUX mmap protocol.
It verifies that the perf event AUX setup handles configuration, memory mapping,
and independent event buffers correctly without breaking user space.

> diff --git a/tools/testing/selftests/perf_events/Makefile b/tools/testing/selftests/perf_events/Makefile
> index 2e5d85770dfea..fbafbd43f097e 100644
> --- a/tools/testing/selftests/perf_events/Makefile
> +++ b/tools/testing/selftests/perf_events/Makefile
> @@ -3,4 +3,5 @@ CFLAGS += -Wl,-no-as-needed -Wall $(KHDR_INCLUDES)
>  LDFLAGS += -lpthread

[Severity: Low]
Since the tests in aux_kernel_usermode.c use 64-bit atomic operations on
__u64 types, does the Makefile need the -latomic flag? Building on 32-bit
architectures usually requires linking libatomic to avoid build failures
on 64-bit atomic loads.

>  
>  TEST_GEN_PROGS := sigtrap_threads remove_on_exec watermark_signal mmap
> +TEST_GEN_PROGS += aux_kernel_usermode
>  include ../lib.mk

[ ... ]

> diff --git a/tools/testing/selftests/perf_events/aux_kernel_usermode.c b/tools/testing/selftests/perf_events/aux_kernel_usermode.c
> new file mode 100644
> index 0000000000000..5c2da4bac0740
> --- /dev/null
> +++ b/tools/testing/selftests/perf_events/aux_kernel_usermode.c
> @@ -0,0 +1,627 @@
[ ... ]
> +static int test_aux_mmap(int pmu_type)
> +{
> +	struct perf_event_attr attr = {};
> +	struct perf_event_mmap_page *mp;
> +	void *aux_base;
> +	unsigned long aux_size, aux_offset, mmap_size;
> +	int fd, ret;
> +
> +	attr.type = pmu_type;
> +	attr.size = sizeof(attr);
> +	attr.disabled = 1;
> +	attr.sample_period = 256;
> +
> +	/* ARM SPE requires period mode (freq=0) */
> +	attr.freq = 0;
> +
> +	fd = perf_event_open(&attr, 0, -1, -1, 0);
> +	if (fd < 0) {
> +		FAIL("AUX mmap: perf_event_open failed (%s)", strerror(errno));
> +		return 1;
> +	}

[Severity: Medium]
Could this cause a false positive failure if the PMU legitimately rejects
the zeroed configuration? Later in test_aux_head_monotonic, the same fd < 0
condition correctly uses SKIP to handle PMU rejection. Should this also skip
rather than failing entirely on systems with strict AUX PMUs?

[ ... ]

> +static int test_aux_head_monotonic(int pmu_type)
> +{
[ ... ]
> +	/* Read initial head */
> +	head_before = __atomic_load_n(&mp->aux_head, __ATOMIC_RELAXED);
> +
> +	usleep(100000); /* 100ms */
> +
> +	head_after = __atomic_load_n(&mp->aux_head, __ATOMIC_RELAXED);
> +
> +	ioctl(fd, PERF_EVENT_IOC_DISABLE, 0);

[Severity: Medium]
Since head_after is read before the event is disabled, is there a chance
the hardware trace data hasn't been flushed to the ring buffer yet?
Hardware tracing PMUs typically only update the control page when explicitly
disabled or during an interrupt, which could cause a spurious SKIP here.
Would it be safer to disable the event before reading head_after?

[ ... ]

> +int main(void)
> +{
[ ... ]
> +	/* Test 6: Verify /proc/sys/kernel/perf_event_paranoid */
> +	printf("\n--- 6. Perf Event Paranoid ---\n");
> +	{
> +		FILE *f = fopen("/proc/sys/kernel/perf_event_paranoid", "r");
> +		int paranoid;
> +
> +		if (f) {
> +			if (fscanf(f, "%d", &paranoid) == 1) {
> +				printf("  perf_event_paranoid=%d\n", paranoid);
> +				if (paranoid > 2)
> +					FAIL("perf_event_paranoid=%d may block AUX",
> +					     paranoid);

[Severity: Medium]
Since the selftest enforces root execution at the beginning of main,
does this sysctl value actually restrict the test? A strict perf_event_paranoid
value greater than 2 typically doesn't block root users. Could this logic
incorrectly report a failure on systems with strict defaults?

> +				else
> +					PASS("perf_event_paranoid=%d (OK)", paranoid);
> +			}
> +			fclose(f);
> +		} else {
> +			printf("  [SKIP] Cannot read perf_event_paranoid\n");
> +		}
> +	}
> +
> +	printf("\n=== Result: %s ===\n", failed ? "FAIL" : "PASS");
> +	if (skipped)
> +		printf("=== Note: some tests were SKIPped (environment)\n");
> +	return failed ? 1 : 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004173458.837842-1-kunwu.chan@gmail.com?part=4