Re: [PATCH 07/15] perf test: Refactor parallel poll loop to drain all pipes simultaneously

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Reaped child PID remains in the test tracking array and may be signaled after reuse, potentially killing an unrelated process.
--

commit 50fe4cbf4a3441a4b15d1298d9f0e698e2a7adec
Author: Ian Rogers <irogers@google.com>

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

This commit addresses parallel test child output blocking by simultaneously
polling all child pipes and reaping them out of order as they finish, which
significantly reduces the execution time for the PMU events suite.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c

[ ... ]

> @@ -499,16 +507,233 @@ static void finish_test(struct child_test **child_tests, int running_test, int c

[ ... ]

> +		for (i = next_to_print; i < num_tests; i++) {
> +			struct child_test *child = child_tests[i];
> +
> +			if (!child || child->done)
> +				continue;
> +
> +			if (check_if_command_finished(&child->process)) {
> +				if (child->process.err > 0) {
> +					drain_child_process_err(child);
> +					close(child->process.err);
> +					child->process.err = -1;
> +				}
> +				child->result = finish_command(&child->process);
> +				child->done = true;
> +			}
> +		}

[Severity: High]
When a child finishes, this calls finish_command() which reaps the process
and releases its PID to the OS, but it doesn't clear child->process.pid.

If a user interrupts the test with a signal before this child is printed
and removed from the array, the error handler in __cmd_test() will loop
through child_tests and send a kill signal:

__cmd_test() {
    ...
    for (size_t x = 0; x < num_tests; x++) {
        struct child_test *child_test = child_tests[x];
        
        if (!child_test || child_test->process.pid <= 0)
            continue;
        
        kill(child_test->process.pid, err);
    }
    ...
}

Since the error handler in __cmd_test() doesn't check if the child is already
done, could this rogue kill() terminate an unrelated system process that
happened to reuse the PID?

[ ... ]

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