Re: [PATCH v7] 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:
- [Medium] Ignoring file descriptor 0 prevents capturing test output and causes resource leaks.
- [Medium] Using `sigprocmask()` in a multi-threaded application is undefined behavior according to POSIX.

Pre-existing issues:
- [High] Incomplete async-signal-safety mitigation leaves the heap vulnerable to deadlocks on `siglongjmp`.
- [High] Calling `waitpid` on pid -1 reaps arbitrary child processes if `fork` fails.
--

commit 2522220ee0e19bb1625cae967792e0223c0e6d7d
Author: Ian Rogers <irogers@google.com>

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

This commit refactors the perf test suite to poll and drain output
pipes from all running children simultaneously. It aims to prevent
tests from blocking on full pipe buffers during parallel execution.

> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 06507066213b1..dccd5d733272c 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
[ ... ]
> @@ -488,16 +496,241 @@ static void finish_test(struct child_test **child_tests, int running_test, int c
>  	}
>  	/* Clean up child process. */
>  	ret = finish_command(&child_test->process);

[Severity: High]
This is a pre-existing issue, but I noticed something about how failed
commands are handled in this cleanup path.

In start_test(), child->process.pid is initialized to -1. If start_command()
fails to fork, the pid remains -1 and an error is returned.

The __cmd_test() function catches this error, jumps to err_out, and calls
finish_test() on all initialized children. finish_test() then calls
finish_command(), which invokes waitpid().

Because the pid is still -1, does waitpid(-1, ...) wait for any child
process in the process group, potentially stealing the exit status of other
parallel tests or unrelated processes?

> +	child_test->process.pid = 0;
> +	if (child_test->err_output.len > 0) {
> +		struct strbuf merged = STRBUF_INIT;
> +
> +		if (child_test->err_output.buf)
> +			strbuf_addstr_safe(&merged, child_test->err_output.buf);
> +		if (err_output.buf)
> +			strbuf_addstr_safe(&merged, err_output.buf);
> +		strbuf_release(&err_output);
> +		err_output = merged;
>  	}
[ ... ]
> +static int strbuf_addstr_safe(struct strbuf *sb, const char *s)
> +{
> +	sigset_t set, oldset;
> +	int ret;
> +
> +	sigemptyset(&set);
> +	sigaddset(&set, SIGINT);
> +	sigaddset(&set, SIGTERM);
> +	sigprocmask(SIG_BLOCK, &set, &oldset);

[Severity: Medium]
Since perf is linked with pthreads and employs multi-threading, does
sigprocmask() result in unspecified behavior according to POSIX?

Would it be safer to use pthread_sigmask() here to guarantee correct signal
masking across threads?

> +	ret = strbuf_addstr(sb, s);
> +	sigprocmask(SIG_SETMASK, &oldset, NULL);
> +	return ret;
> +}
[ ... ]
> +static int finish_tests_parallel(struct child_test **child_tests, size_t num_tests, int width)
> +{
> +	size_t next_to_print = 0;
> +	struct pollfd *pfds;
> +	size_t *pfd_indices;
> +	size_t num_pfds = 0;
> +	int last_running = -1;
> +	size_t i;
> +	int last_suite_printed = -1;
> +
> +	global_pfds = calloc(num_tests, sizeof(*pfds));

[Severity: High]
This isn't a bug introduced by this patch, but the test harness uses a
signal handler (cmd_test_sig_handler) that triggers siglongjmp() on SIGINT
or SIGTERM.

If a signal interrupts a heap allocation or deallocation, the libc malloc
lock is left permanently held. The cleanup path err_out then calls
finish_test(), which allocates memory, causing a guaranteed deadlock.

While strbuf_addstr_safe() aims to mitigate this, the newly added heap
operations in finish_tests_parallel() like calloc(), zfree(), and
strbuf_release() remain unprotected. Additionally, the pre-existing
strbuf_addstr() call inside the poll loop of finish_test() was not updated
to use the new safe wrapper.

Is there a way we can ensure these allocations don't deadlock if a signal
arrives during execution?

> +	global_pfd_indices = calloc(num_tests, sizeof(*pfd_indices));
> +	pfds = global_pfds;
> +	pfd_indices = global_pfd_indices;
> +	if (!pfds || !pfd_indices) {
> +		free(pfds);
> +		free(pfd_indices);
> +		global_pfds = NULL;
> +		global_pfd_indices = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < num_tests; i++) {
> +		struct child_test *child = child_tests[i];
> +
> +		if (!child)
> +			continue;
> +		strbuf_init(&child->err_output, 0);
> +		if (child->process.err > 0)

[Severity: Medium]
If the perf test is run in an environment with standard input closed,
pipe() can legitimately return file descriptor 0 for the read end.

Does checking > 0 instead of >= 0 incorrectly treat this valid file
descriptor as invalid, causing the harness to silently fail to poll or
drain it?

> +			fcntl(child->process.err, F_SETFL, O_NONBLOCK);
> +	}
[ ... ]
> +			if (child->process.err > 0) {
> +				pfds[num_pfds].fd = child->process.err;
> +				pfds[num_pfds].events = POLLIN | POLLERR | POLLHUP | POLLNVAL;
> +				pfd_indices[num_pfds] = i;
> +				num_pfds++;
> +			}

[Severity: Medium]
As mentioned above, if child->process.err happens to be 0, it will also
be skipped here, meaning the output will be dropped entirely.

[ ... ]
> +		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) {

[Severity: Medium]
And similarly, skipping file descriptor 0 here would cause a resource leak
since close() will be bypassed.

> +					drain_child_process_err(child);
> +					close(child->process.err);
> +					child->process.err = -1;
> +				}
> +				child->result = finish_command(&child->process);

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