Re: [PATCH v3 06/14] perf test: Refactor parallel poll loop to drain all pipes simultaneously

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Race Condition / Infinite Wait from PID Reuse
- [Medium] Comment/Code Mismatch and Incorrect Fallback Logic

Pre-existing issues:
- [High] Unintended Process Reaping via waitpid(-1)
- [High] Use-After-Free in Signal Handler during Test Teardown
--

commit 85338a93349cbaff2c4d5fe37f997271e81388e1
Author: Ian Rogers <irogers@google.com>

perf test: Refactor parallel poll loop to drain all pipes simultaneously

When running tests in parallel with verbose output (-v), child processes
write to pipes. If a test produces significant output, it fills the pipe
buffer and blocks. This commit introduces finish_tests_parallel() to
poll and drain output pipes from all running children simultaneously.

> diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
> index b7510f83209a0..e1341080dbae7 100644
> --- a/tools/lib/subcmd/run-command.c
> +++ b/tools/lib/subcmd/run-command.c
> @@ -146,6 +146,10 @@ int start_command(struct child_process *cmd)
>  			close(cmd->out);
>  		if (need_err)
>  			close_pair(fderr);
> +		cmd->pid = -1;
> +		cmd->in = -1;
> +		cmd->out = -1;
> +		cmd->err = -1;

[Severity: High]
This isn't a bug introduced by this patch, but does assigning -1 to the PID
on error create an unintended process reaping hazard?

If start_command() encounters an error, it explicitly sets cmd->pid to -1.
The error propagates up to __cmd_test(), jumping to the err_out block where
it unconditionally loops over the child_tests array to clean up.

Because finish_test() does not verify if child_test->process.pid > 0 before
proceeding to check_if_command_finished() and finish_command(), this cleanup
flow ultimately executes a waitpid() call:

waitpid(cmd->pid, &status, block ? 0 : WNOHANG);

Calling waitpid() with -1 forces the kernel to wait for any child process,
which can inadvertently reap an unrelated concurrent test process and discard
its exit status.

>  		return err == ENOENT ?
>  			-ERR_RUN_COMMAND_EXEC :
>  			-ERR_RUN_COMMAND_FORK;

[ ... ]

> @@ -241,8 +247,45 @@ int check_if_command_finished(struct child_process *cmd)
>  	sprintf(filename, "/proc/%u/status", cmd->pid);
>  	status_file = fopen(filename, "r");
>  	if (status_file == NULL) {
> -		/* Open failed assume finish_command was called. */
> -		return true;
> +		/*
> +		 * fopen() can fail with ENOENT if the process has been reaped.
> +		 * It can also fail with EMFILE/ENFILE if RLIMIT_NOFILE is reached,
> +		 * or with EINTR/ENOMEM. Use kill(pid, 0) as a robust fallback
> +		 * to distinguish between active processes and dead ones without
> +		 * consuming file descriptors.
> +		 */

[Severity: Medium]
Does the implementation contradict the documented intent here?

The comment explicitly states that the logic uses kill(pid, 0) as a robust
fallback. However, the subsequent code calls waitpid() instead. Using waitpid
actively reaps the process, whereas kill(pid, 0) would safely let the process
remain a zombie until explicitly reaped by finish_command().

> +		if (errno == ENOENT)
> +			return 1;
> +		waiting = waitpid(cmd->pid, &status, WNOHANG);

[Severity: High]
Could this waitpid() fallback introduce an infinite loop due to PID reuse?

Since check_if_command_finished() is polled repeatedly in
finish_tests_parallel(), and it does not verify if cmd->finished is already
true before querying the procfs status file, there is a risk that a previous
waitpid() call already reaped the child.

Once reaped, the OS can reuse the PID. If an unrelated system daemon is
assigned this PID before the next loop iteration, fopen() on the procfs
status file will succeed and report the active state of that unrelated
process. The test harness would then loop indefinitely waiting for the
unrelated daemon to exit.

> +		if (waiting == cmd->pid) {
> +			int result;

[ ... ]

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 2ccb52a776ccb..9f71f11928c60 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c

[ ... ]

> @@ -503,12 +509,205 @@ static void finish_test(struct child_test **child_tests, int running_test, int c
>  		fprintf(stderr, "%s", err_output.buf);
>  
>  	strbuf_release(&err_output);
> +	strbuf_release(&child_test->err_output);
>  	print_test_result(t, curr_suite, curr_test_case, ret, width, /*running=*/0);
>  	if (err > 0)
>  		close(err);
>  	zfree(&child_tests[running_test]);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
signal teardown when using zfree() here?

The zfree() macro generally expands to a free() operation followed by
setting the pointer to NULL. If a signal like SIGINT arrives exactly
between the memory being freed and the pointer being nulled, the signal
handler preempts the thread.

The cmd_test_sig_handler handler uses siglongjmp() to return to err_out in
__cmd_test(), which iterates over child_tests to terminate active children.
The loop will see a non-NULL pointer, dereference the freed memory to read
child_test->process.pid, and issue a kill() signal to whatever garbage
value was read. This could potentially terminate critical system processes.

>  }

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