Re: [PATCH 5/5] selftests/perf_events: add AUX kernel API selftest script

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] selftests/perf_events: masked exit code of usermode binary
- [Medium] selftests/perf_events: missed dmesg errors due to ring buffer wrap
- [Medium] selftests/perf_events: compilation failure due to quoted $CC
- [Medium] selftests/perf_events: hardcoded period breaks intel_pt PMU testing
- [Medium] selftests/perf_events: arbitrary fallback to /usr/src/linux breaks installed testing
- [Low] selftests/perf_events: pass() helper discards subsequent string arguments
--

commit ae8124722ffc34fbe1b637d67fb27766b7c48d04
Author: Kunwu Chan <kunwu.chan@gmail.com>

selftests/perf_events: add AUX kernel API selftest script

Add a shell script that runs the perf_aux_kernel KUnit suite through
debugfs, builds and runs the userspace regression program, and
performs static-analysis checks on the source tree.

> diff --git a/tools/testing/selftests/perf_events/aux_kernel.sh b/tools/testing/selftests/perf_events/aux_kernel.sh
> new file mode 100755
> index 0000000000000..2271f105f55b7
> --- /dev/null
> +++ b/tools/testing/selftests/perf_events/aux_kernel.sh
> @@ -0,0 +1,631 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0

[ ... ]

> +ROOT="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel 2>/dev/null || true)"
> +if [[ -z "$ROOT" ]]; then
> +	ROOT="/usr/src/linux"
> +fi
> +
> +# Source-level checks need the kernel tree; installed runs have none.
> +SRC_EVENTS="$ROOT/kernel/events"
> +if [[ -f "$SRC_EVENTS/core.c" ]]; then
> +	HAVE_SRC=1
> +else
> +	HAVE_SRC=0
> +	ROOT=""
> +fi

[Severity: Medium]
Does falling back to /usr/src/linux in the main script logic break the
skipping logic for installed selftests?

If the tests are run installed without a kernel tree, git rev-parse fails and
ROOT becomes /usr/src/linux. If the system has host kernel headers installed
there, HAVE_SRC evaluates to 1. 

This causes the script to run static analysis against the mismatched host
kernel, resulting in spurious test failures instead of gracefully skipping.

[ ... ]

> +if [[ -x "$UMODE_BIN" && ( ! -f "$UMODE_SRC" || ! "$UMODE_SRC" -nt "$UMODE_BIN" ) ]]; then
> +	:
> +elif [[ -f "$UMODE_SRC" ]]; then
> +	echo "  Building aux_kernel_usermode with ${CC}..."
> +	if "$CC" -o "$UMODE_BIN" "$UMODE_SRC" -Wall -Wextra -O2; then

[Severity: Medium]
Will this compile command in the main script logic fail if the CC environment
variable contains spaces?

By double-quoting "$CC", bash is prevented from performing word splitting. 

If CC contains flags or prefixes like "ccache gcc" or "clang --target=...",
bash attempts to execute a binary literally named with spaces and will fail
with a command not found error.

> +		:
> +	else
> +		echo "  $CC build failed"
> +		UMODE_BIN=""
> +	fi
> +else
> +	UMODE_BIN=""
> +fi
> +
> +if [[ -n "$UMODE_BIN" && -x "$UMODE_BIN" ]]; then
> +	if "$UMODE_BIN" "$PMU_NAME" 2>&1 | tee "$RESULTS_DIR/usermode.log" | \
> +		grep -c '\[PASS\]' > /dev/null; then

[Severity: Medium]
Does this pipeline evaluate only the exit status of grep, swallowing the test
binary's exit status?

Since the pipeline is evaluated without pipefail in the main script logic,
if the test program emits at least one [PASS] before crashing, grep -c
returns 0 and the if condition is satisfied. 

Because UMODE_FAIL only counts explicit [FAIL] strings, crashes without
explicit failure logs bypass the error checks.

> +		# Count pass/fail from the output
> +		UMODE_PASS=$(grep -c '\[PASS\]' "$RESULTS_DIR/usermode.log" || true)
> +		UMODE_FAIL=$(grep -c '\[FAIL\]' "$RESULTS_DIR/usermode.log" || true)
> +		UMODE_SKIP=$(grep -c '\[SKIP\]' "$RESULTS_DIR/usermode.log" || true)
> +		pass "Userspace API regression: $UMODE_PASS passed," \
> +			" $UMODE_FAIL failed, $UMODE_SKIP skipped"

[Severity: Low]
Because the pass() helper function only consumes $1, are the second and third
string arguments dropped?

When invoked here with a multi-argument string, the pass() helper drops all
arguments after the first. This truncates the test log output, losing the
counts of failed and skipped tests.

[ ... ]

> +		if [[ -n "$PERF_BIN" ]]; then
> +			DMESG_BEFORE=$(dmesg 2>/dev/null | wc -l)
> +			if "$PERF_BIN" record -e "$PMU_NAME/period=100000/" \
> +				-o "$RESULTS_DIR/perf_hw.data" -- sleep 0.5 2>/dev/null; then

[Severity: Medium]
Could this hardcoded config term break compatibility with instruction tracing
PMUs like intel_pt?

The script's usage advertises support for intel_pt. However, it hardcodes
the /period=100000/ config term in the perf record command here in the main
script logic. 

Instruction tracing PMUs like Intel PT do not support sampling periods,
causing the perf_event_open syscall to reject the configuration.

> +				pass "Hardware AUX: perf record with $PMU_NAME succeeded"
> +			else
> +				fail "Hardware AUX: perf record with $PMU_NAME failed"
> +			fi
> +
> +			DMESG_AFTER="$RESULTS_DIR/dmesg-hardware.log"
> +			dmesg 2>/dev/null > "$DMESG_AFTER" || true
> +			DMESG_LINES_AFTER=$(wc -l < "$DMESG_AFTER")
> +			if [[ "$DMESG_LINES_AFTER" -ge "$DMESG_BEFORE" ]]; then
> +				NEW_DMESG=$(tail -n "+$((DMESG_BEFORE + 1))" "$DMESG_AFTER")

[Severity: Medium]
Does this logic fail when the kernel log buffer wraps around and maintains a
constant maximum line count?

dmesg outputs the contents of a circular buffer. If the buffer is full,
DMESG_LINES_AFTER will equal DMESG_BEFORE (or be very close). 

When DMESG_LINES_AFTER >= DMESG_BEFORE, the script uses 
tail -n "+$((DMESG_BEFORE + 1))". If the line count hasn't increased because
the buffer reached max capacity and wrapped, this command attempts to read past
the end of the buffer, yielding zero lines and missing all new kernel messages.

> +			else
> +				NEW_DMESG=$(cat "$DMESG_AFTER")
> +			fi

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