Re: [PATCH 06/15] tools subcmd: Robust fallback and existence checks for process reaping

[sashiko-bot@kernel.org]

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

New issues:
- [High] Early returns for errors fail to set `cmd->finished = 1`, causing catastrophic `waitpid` errors and arbitrary process reaping in downstream calls.
- [Medium] The implementation contradicts the commit message by aggressively reaping processes with `waitpid()` instead of preserving zombies with `kill()`, and completely fails to fix the PID reuse vulnerability.

Pre-existing issues:
- [High] `tpebs_stop()` clears `tpebs_cmd.pid` to 0 before calling `finish_command()`, causing `waitpid(0)` to accidentally reap unrelated child processes.
- [High] `tpebs_send_record_cmd()` drops `tpebs_mtx` while polling, allowing concurrent entry into shutdown sequences.
--

commit 69e7ae6fd81118d00c12a9bae1bdf8a2eb52f3fd
Author: Ian Rogers <irogers@google.com>
tools subcmd: Robust fallback and existence checks for process reaping

This commit modifies the check_if_command_finished() function to handle
invalid PIDs gracefully and introduces a fallback mechanism when
/proc/pid/status is inaccessible.

> Transition the procfs status fopen() fallback from active waitpid() calls
> to robust kill(pid, 0) existence and ESRCH detection logic. This allows
> zombie processes to remain until explicitly reaped by finish_command(),
> eradicating the risk of infinite loops induced by kernel PID reuse across
> unrelated system daemons.

[Severity: Medium]
Does the implementation contradict the commit message?

The commit message states we transition to kill(pid, 0) existence and ESRCH
detection logic to allow zombie processes to remain.

However, looking at check_if_command_finished(), waitpid() with WNOHANG is
used instead, which actively reaps the process.

Additionally, since the fopen("/proc/<pid>/status") check is retained as the
primary path, if an unrelated daemon reuses the PID, fopen will succeed, read
its running state, and return 0 indefinitely. Does this mean the infinite loop
vulnerability the commit message claims to eradicate is still present?

> diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
> --- a/tools/lib/subcmd/run-command.c
> +++ b/tools/lib/subcmd/run-command.c
> @@ -233,7 +233,14 @@ int check_if_command_finished(struct child_process *cmd)
>  	char filename[6 + MAX_STRLEN_TYPE(typeof(cmd->pid)) + 7 + 1];
>  	char status_line[256];
>  	FILE *status_file;
> +#endif
> +
> +	if (cmd->finished)
> +		return 1;
> +	if (cmd->pid <= 0)
> +		return 1;

[Severity: High]
Does returning early here without setting cmd->finished = 1 cause issues later?

If check_if_command_finished() returns 1 but cmd->finished is still 0, a
subsequent call to finish_command() will eventually call wait_or_whine(),
which executes a blocking waitpid(cmd->pid, ...).

If cmd->pid is -1 due to a fork failure, executing waitpid(-1, ...) instructs
the kernel to blindly wait for any child process, potentially reaping parallel
tests and causing state corruption.

> +
> +#ifdef __linux__
>  	/*
>  	 * Check by reading /proc/<pid>/status as calling waitpid causes
>  	 * stdout/stderr to be closed and data lost.
> @@ -241,8 +248,48 @@ 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;
> +		int status;
> +		pid_t waiting;
> +
> +		/*
> +		 * fopen() can fail with ENOENT if the process has been reaped.
> +		 * It can also fail with EMFILE/ENFILE if RLIMIT_NOFILE is reached.
> +		 * In those cases, use waitpid(..., WNOHANG) to robustly check
> +		 * and reap the process if it has exited.
> +		 */
> +		if (errno == ENOENT)
> +			return 1;

[Severity: High]
Are we also missing cmd->finished = 1 here?

> +
> +		waiting = waitpid(cmd->pid, &status, WNOHANG);

[Severity: Medium]
This is the waitpid() call mentioned earlier that seems to contradict the
commit message's claim of using kill().

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

[ ... ]

> +			cmd->finish_result = result;
> +			return 1;
> +		}
> +		if (waiting < 0 && (errno == ECHILD || errno == ESRCH))
> +			return 1;

[Severity: Medium]
Can waitpid() actually return ESRCH?

[Severity: High]
Are we missing cmd->finished = 1 here as well, similar to the other early
returns?

While reviewing these changes, I noticed a couple of related issues in the
subsystem:

tools/perf/util/intel-tpebs.c:tpebs_stop() {
        ...
        tpebs_cmd.pid = 0;
        mutex_unlock(tpebs_mtx_get());
        pthread_join(tpebs_reader_thread, NULL);
        mutex_lock(tpebs_mtx_get());
        close(control_fd[0]);
        ...
        ret = finish_command(&tpebs_cmd);
        ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does setting tpebs_cmd.pid to 0
before calling finish_command() cause waitpid(0) to execute?

Passing PID 0 to waitpid() instructs the kernel to wait for any child process
in the current process group. Could this intercept and reap unrelated
background child processes spawned by the perf tool, permanently losing their
exit statuses?

tools/perf/util/intel-tpebs.c:tpebs_send_record_cmd() {
        ...
        mutex_unlock(tpebs_mtx_get());

        /* Send perf record command.*/
        len = strlen(msg);
        ret = write(control_fd[1], msg, len);
        ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does dropping tpebs_mtx while
polling in tpebs_send_record_cmd() break mutual exclusion?

If tpebs_stop() is called concurrently, it invokes
tpebs_send_record_cmd(EVLIST_CTL_CMD_STOP_TAG), dropping the lock during the
shutdown sequence. Does this allow another thread to enter tpebs_stop()
simultaneously, leading to undefined behavior like double pthread_join(),
double close(), or double calls to finish_command()?

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