Re: [PATCH v7] perf tpebs: Fix concurrent stop races and PID reuse hazards in tpebs_stop

[sashiko-bot@kernel.org]

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

New issues:
- [High] Spurious `-EBUSY` failures and skipped event initialization due to a lockless `tpebs_cmd.pid` check racing with temporary PID restoration.

Pre-existing issues:
- [Critical] Unsynchronized check of `tpebs_cmd.pid` allows concurrent threads to hit `assert(tpebs_cmd.pid == 0)` and crash the process.
- [High] Unprotected `pthread_join` window allows concurrent session start, leading to orphaned processes and state corruption.
- [High] Statically allocated `tpebs_cmd` struct is not re-initialized between executions, silently skipping `waitpid()` and leaking zombie processes.
- [High] Error paths in `evsel__tpebs_open()` fail to clean up file descriptors, child processes, and global state, permanently breaking TPEBS functionality.
--

commit 6304c5f7736398fadcc6a882cdc4bef08d2b9994
Author: Ian Rogers <irogers@google.com>

perf tpebs: Fix concurrent stop races and PID reuse hazards in tpebs_stop

This commit introduces a tpebs_stopping flag to prevent redundant stop
execution paths. It also temporarily restores cmd.pid during
finish_command() to ensure the process is reaped correctly.

> diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> index 8b615dc94e9ea..6d642356fd9dc 100644
> --- a/tools/perf/util/intel-tpebs.c
> +++ b/tools/perf/util/intel-tpebs.c
> @@ -37,6 +37,7 @@ static pthread_t tpebs_reader_thread;
>  static struct child_process tpebs_cmd;
>  static int control_fd[2], ack_fd[2];
>  static struct mutex tpebs_mtx;
> +static bool tpebs_stopping;

[Severity: High]
This is a pre-existing issue, but since tpebs_cmd is a static global, does
it need to be re-initialized between executions?

When a session finishes, finish_command() internally sets
tpebs_cmd.finished = 1. When a subsequent session begins,
evsel__tpebs_start_perf_record() calls start_command(&tpebs_cmd), which
does not clear the finished flag from previous executions.

When the second session stops, finish_command() will see finished == 1
and skip waitpid() entirely, leaving subsequent perf record processes as
zombies.

[Severity: Critical]
This isn't a bug introduced by this patch, but the unsynchronized check of
tpebs_cmd.pid in evsel__tpebs_open() allows concurrent threads to bypass
validation:

int evsel__tpebs_open(...) {
	if (tpebs_cmd.pid != 0) {

If two threads call this concurrently while no session is running, both
bypass the check. They serialize on tpebs_mtx in evsel__tpebs_prepare(),
and both will observe !list_empty(&tpebs_results).

Thread 1 successfully spawns the process and sets tpebs_cmd.pid. Once
Thread 1 releases the lock, Thread 2 acquires it and calls
evsel__tpebs_start_perf_record(), where it hits:

	assert(tpebs_cmd.pid == 0);

Can this cause the entire process to crash when the assertion fails?

[Severity: High]
This is a pre-existing issue, but the error path in evsel__tpebs_open()
fails to fully clean up state when pthread_create() fails:

	if (pthread_create(&tpebs_reader_thread, NULL,
			   __sample_reader, NULL)) {
		kill(tpebs_cmd.pid, SIGTERM);
		close(tpebs_cmd.out);
		pr_err("Could not create thread to process sample data.\n");
		ret = -1;
		goto out;
	}

This path does not close the control_fd and ack_fd pipes, and it does not
reap the killed child with finish_command(). It also leaves tpebs_cmd.pid
persistently non-zero.

Would this permanently break TPEBS functionality for the process lifetime
by causing future calls to locklessly observe a non-zero PID and return
-EBUSY?

[ ... ]

> @@ -321,8 +322,14 @@ static int tpebs_stop(void) EXCLUSIVE_LOCKS_REQUIRED(tpebs_mtx_get())
>  {
>  	int ret = 0;
>  
> +	if (tpebs_stopping)
> +		return 0;
> +
>  	/* Like tpebs_start, we should only run tpebs_end once. */
>  	if (tpebs_cmd.pid != 0) {
> +		pid_t actual_pid = tpebs_cmd.pid;
> +
> +		tpebs_stopping = true;
>  		tpebs_send_record_cmd(EVLIST_CTL_CMD_STOP_TAG);
>  		tpebs_cmd.pid = 0;
>  		mutex_unlock(tpebs_mtx_get());
>  		pthread_join(tpebs_reader_thread, NULL);

[Severity: High]
This is a pre-existing issue, but dropping the mutex while tpebs_cmd.pid is
zero allows a concurrent call to evsel__tpebs_open() to spawn a brand new
TPEBS session during the pthread_join() window.

When this thread reacquires the lock, it will blindly close the file
descriptors of the new session and clobber the new session's PID by restoring
the old actual_pid. Does this leave the newly spawned process permanently
untracked and its reader thread leaked?

>  		mutex_lock(tpebs_mtx_get());
>  		close(control_fd[0]);
>  		close(control_fd[1]);
>  		close(ack_fd[0]);
>  		close(ack_fd[1]);
>  		close(tpebs_cmd.out);
> +		tpebs_cmd.pid = actual_pid;
>  		ret = finish_command(&tpebs_cmd);
>  		tpebs_cmd.pid = 0;

[Severity: High]
Does temporarily restoring tpebs_cmd.pid to a non-zero value here create a
race condition with evsel__tpebs_open()?

Since evsel__tpebs_open() performs a lockless check of tpebs_cmd.pid:

int evsel__tpebs_open(...) {
	if (tpebs_cmd.pid != 0) {
		struct tpebs_retire_lat *t;

A concurrent thread could observe the restored non-zero PID, assume a session
is actively running, and incorrectly return -EBUSY because it skipped the
initialization sequence.

> +		tpebs_stopping = false;
>  		if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
>  			ret = 0;
>  	}

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