Re: [PATCH v10 15/20] perf intel-tpebs: Fix race in tpebs_stop() and tpebs_cmd.out fd handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] perf intel-tpebs: race between evsel__tpebs_open() cleanup
  and tpebs_stop()
--

commit 7a1ce986169f6d07b45e9adbf7d1dd00eff88dbc
Author: Ian Rogers <irogers@google.com>

perf intel-tpebs: Fix race in tpebs_stop() and tpebs_cmd.out fd handling

This commit fixes a race on tpebs_reader_thread in tpebs_stop(), transfers
tpebs_cmd.out ownership to __sample_reader(), cleans up tpebs_cmd and
tpebs_reader_thread on error paths, and adds explicit header includes.

> diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> index 7c0b8b15bd10e..47c939fc2058c 100644
> --- a/tools/perf/util/intel-tpebs.c
> +++ b/tools/perf/util/intel-tpebs.c
[ ... ]
> @@ -567,12 +588,15 @@ int evsel__tpebs_open(struct evsel *evsel)
>  			goto out;
>  
>  		if (pthread_create(&tpebs_reader_thread, /*attr=*/NULL, __sample_reader,
> -				   /*arg=*/NULL)) {
> +				   (void *)(intptr_t)tpebs_cmd.out)) {
>  			kill(tpebs_cmd.pid, SIGTERM);
>  			pr_err("Could not create thread to process sample data.\n");
>  			ret = -1;
>  			goto out;
>  		}
> +		/* __sample_reader now owns and closes the output fd. */
> +		tpebs_cmd.out = -1;
> +		started_thread = true;
>  		ret = tpebs_send_record_cmd(EVLIST_CTL_CMD_ENABLE_TAG);
>  	}
>  out:
> @@ -586,9 +610,22 @@ int evsel__tpebs_open(struct evsel *evsel)
>  
>  		if (started_process) {
>  			if (tpebs_cmd.pid > 0) {
> -				kill(tpebs_cmd.pid, SIGTERM);
> +				pid_t actual_pid = tpebs_cmd.pid;
> +
> +				tpebs_stopping = true;
> +				kill(actual_pid, SIGTERM);
> +				tpebs_cmd.pid = 0;
> +				if (started_thread) {
> +					pthread_t reader_thread = tpebs_reader_thread;
> +
> +					mutex_unlock(tpebs_mtx_get());
> +					pthread_join(reader_thread, NULL);
> +					mutex_lock(tpebs_mtx_get());
> +				}
> +				tpebs_cmd.pid = actual_pid;
>  				finish_command(&tpebs_cmd);
>  				tpebs_cmd.pid = 0;
> +				tpebs_stopping = false;
>  			}

[Severity: Medium]
Can this error path race with tpebs_stop() and call pthread_join() twice on
the same reader_thread?

tpebs_send_record_cmd() drops tpebs_mtx while writing the command and
polling ack_fd[0].  During that window a concurrent evsel__tpebs_close()
can enter tpebs_stop(), which also drops tpebs_mtx inside
tpebs_send_record_cmd() before clearing tpebs_cmd.pid:

CPU0                                  CPU1
evsel__tpebs_open()
  started_process = true
  started_thread = true
  tpebs_send_record_cmd(ENABLE)
    mutex_unlock(tpebs_mtx)
                                      evsel__tpebs_close()
                                        tpebs_stop()
                                          tpebs_stopping = true
                                          tpebs_send_record_cmd(STOP)
                                            mutex_unlock(tpebs_mtx)
    mutex_lock(tpebs_mtx)
  if (started_process)
    if (tpebs_cmd.pid > 0)
      reader_thread = tpebs_reader_thread
      tpebs_cmd.pid = 0
      mutex_unlock(tpebs_mtx)
      pthread_join(reader_thread)
                                            mutex_lock(tpebs_mtx)
                                          reader_thread = tpebs_reader_thread
                                          mutex_unlock(tpebs_mtx)
                                          pthread_join(reader_thread)
                                            <-- double pthread_join()

Both threads then also restore tpebs_cmd.pid = actual_pid and call
finish_command(&tpebs_cmd).  Even if tpebs_stop() has already cleared
tpebs_cmd.pid = 0 and is unlocked inside pthread_join(), the
started_process block still closes control_fd and ack_fd underneath it.

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