Re: [PATCH v1] perf tests: top/kvm don't depend on coreutils timeout

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] Orphaned watcher process can send SIGTERM to unrelated system processes (PID recycling).
- [Medium] Exit code mismatch in timeout emulation falsely fails tests.
- [Medium] Implicit standard input redirection breaks the protective pipeline.
- [Medium] Non-POSIX sleep argument causes immediate timeout on strict minimal systems.
--

commit 17f50e3b3f4410cb65917a2951f5c28c36d9bf25
Author: Ian Rogers <irogers@google.com>

perf tests: top/kvm don't depend on coreutils timeout

This commit adds a bash-based fallback for the coreutils timeout command
to prevent failures on minimal test systems. It then updates the kvm and
top test scripts to use this new bash_timeout function.

> diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
> index f88e859025c42..a27b3d6414e9d 100755
> --- a/tools/perf/tests/shell/kvm.sh
> +++ b/tools/perf/tests/shell/kvm.sh
[ ... ]
> @@ -102,7 +105,8 @@ test_kvm_stat_live() {
>  
>          # Run perf kvm live for 5 seconds, monitoring that PID
>  	# Use sleep to keep stdin open but silent, preventing EOF loop or interactive spam
> -	if ! sleep 10 | timeout 5s perf kvm stat live -p "${qemu_pid}" > "${log_file}" 2>&1; then
> +	if ! sleep 10 | bash_timeout 5s perf kvm stat live -p "${qemu_pid}" > "${log_file}" 2>&1

Does POSIX sleep support the "s" suffix passed as "5s" here?

On minimal systems lacking coreutils, sleep "5s" might fail instantly, causing
the watcher script to kill the target command in 0 seconds.

> +	then
>  		retval=$?
>  		if [ $retval -ne 124 ] && [ $retval -ne 0 ]; then

When bash_timeout kills the process via SIGTERM, it returns 143 (128 + 15), not
the GNU timeout exit code of 124.

Since this code checks for 124, will an expected timeout incorrectly register
as a test failure?

> diff --git a/tools/perf/tests/shell/lib/timeout.sh b/tools/perf/tests/shell/lib/timeout.sh
> new file mode 100644
> index 0000000000000..b4d2c0acf0e22
> --- /dev/null
> +++ b/tools/perf/tests/shell/lib/timeout.sh
[ ... ]
> +bash_timeout() {
[ ... ]
> +    local timeout_secs="$1"
> +    shift
> +    local command=("$@")
> +
> +    # Execute the target command in the background
> +    "${command[@]}" &

Does placing the target command in the background automatically redirect its
standard input from /dev/null?

This appears to sever the connection to the "sleep 10 |" pipe used in the
calling scripts, causing perf to read an immediate EOF and triggering the loop
and interactive spam the pipe was meant to prevent.

> +    local cmd_pid=$!
> +
> +    # Start a watcher process in the background
> +    (
> +        sleep "$timeout_secs"
> +        kill -TERM "$cmd_pid" 2>/dev/null || true
> +    ) &

If the test script exits early (for example, due to set -e or an interrupt)
before killing the watcher process, does the watcher survive as an orphaned
background task?

Since perf tests often run as root, this orphaned watcher might wake up later
and send SIGTERM to a recycled PID, potentially terminating unrelated system
services.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260413213300.1700116-1-irogers@google.com?part=1