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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/perf_events: truncation of 64-bit aux_head to 32-bit unsigned long and missing -latomic
- [Low] selftests/perf_events: manual printf bypasses SKIP() macro and corrupts skipped test accounting
--

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

    selftests/perf_events: add userspace AUX regression test
    
    Add a standalone C program that opens a perf event, mmaps the metadata
    and data ring, sets up the AUX area via a second mmap, enables tracing,
    reads aux_head from the mmap page, and tears down the mapping.  This
    verifies the kernel-side aux_kernel_count changes do not break the
    existing userspace AUX mmap protocol.

> 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..2c8f4baf0825f
> --- /dev/null
> +++ b/tools/testing/selftests/perf_events/aux_kernel_usermode.c

[ ... ]

> +	/*
> +	 * Step 3: mmap the AUX area at aux_offset.
> +	 * The kernel reads aux_offset from the metadata page, verifies
> +	 * the mmap offset matches, and allocates the AUX buffer.
> +	 */
> +	aux_base = mmap(NULL, aux_size, PROT_READ, MAP_SHARED, fd,
> +			aux_offset);
> +	if (aux_base == MAP_FAILED) {
> +		printf("  [SKIP] AUX mmap: AUX area mmap failed (%s)\n",
> +		       strerror(errno));

[Severity: Low]
Is this manual printf() bypassing the SKIP() macro?

The test suite relies on the SKIP() macro to update the global skipped flag. By
using printf() directly here in test_aux_mmap(), the flag isn't updated and the
final note "=== Note: some tests were SKIPped (environment)" might be
incorrectly omitted if this is the only skipped test.

> +		munmap(mp, mmap_size);
> +		close(fd);
> +		return 0;
> +	}
> +	PASS("AUX mmap: metadata mmap OK, AUX area mmap OK");

[ ... ]

> +static int test_aux_head_monotonic(int pmu_type)
> +{
> +	struct perf_event_attr attr = {};
> +	struct perf_event_mmap_page *mp;
> +	unsigned long aux_size, mmap_size, head_before, head_after;

[Severity: Medium]
Will this truncate the 64-bit hardware trace cursors on 32-bit architectures?

The aux_head field in struct perf_event_mmap_page is defined as a 64-bit __u64.
Since head_before and head_after are declared as unsigned long, they will be
32-bit on 32-bit architectures, causing the upper 32 bits to be silently
discarded when reading aux_head.

> +	void *aux_base;
> +	int fd, ret;

[ ... ]

> +	/* Read initial head */
> +	head_before = __atomic_load_n(&mp->aux_head, __ATOMIC_RELAXED);

[Severity: Medium]
Can this atomic load of a 64-bit value cause a build failure on 32-bit
architectures?

Using __atomic_load_n() on a 64-bit integer on 32-bit platforms often requires
linking against libatomic. The Makefile modifications in this patch do not add
-latomic to LDFLAGS, which might lead to link failures on those architectures.

> +
> +	usleep(100000); /* 100ms */

[ ... ]

> +	/* Find an AUX-capable PMU: explicit name from argv, or auto-detect. */
> +	if (argc > 1)
> +		pmu_type = find_aux_pmu_type_by_name(argv[1]);
> +	else
> +		pmu_type = find_aux_pmu_type();
> +	if (pmu_type < 0) {
> +		printf("--- 2-5. AUX Tests ---\n");
> +		printf("  [SKIP] No AUX-capable PMU found\n");

[Severity: Low]
Does this manual printf() also bypass the SKIP() macro?

Similar to the issue in test_aux_mmap(), bypassing the SKIP() macro here
in main() prevents the skipped flag from being updated, which corrupts the
skipped test accounting at the end of the run.

> +	} else {
> +		/* Test 2: AUX mmap lifecycle */
> +		printf("\n--- 2. AUX Mmap Lifecycle ---\n");

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