Re: [PATCH v3 13/17] 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 2 potential issue(s) to consider:
- [High] perf: Samples dropped during teardown due to premature tpebs_cmd.pid clearance
- [Medium] perf: thread leak on tpebs_send_record_cmd error path
--

commit 12668ca70c2a83aae9686820143aafd887a3a879
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 by releasing tpebs_mtx before calling
pthread_join() in tpebs_stop(). It also transfers ownership of the
tpebs_cmd.out file descriptor to the sample reader thread to prevent
a double-close.

> diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> index 7c0b8b15bd10e..b81831f5e5a42 100644
> --- a/tools/perf/util/intel-tpebs.c
> +++ b/tools/perf/util/intel-tpebs.c
[ ... ]
> @@ -331,12 +339,14 @@ static int tpebs_stop(void) __must_hold(tpebs_mtx_get())
>  	/* Like tpebs_start, we should only run tpebs_end once. */
>  	if (tpebs_cmd.pid != 0) {
>  		pid_t actual_pid = tpebs_cmd.pid;
> +		pthread_t reader_thread;
>  
>  		tpebs_stopping = true;
>  		tpebs_send_record_cmd(EVLIST_CTL_CMD_STOP_TAG);
>  		tpebs_cmd.pid = 0;
> +		reader_thread = tpebs_reader_thread;
>  		mutex_unlock(tpebs_mtx_get());
> -		pthread_join(tpebs_reader_thread, NULL);
> +		pthread_join(reader_thread, NULL);

[Severity: High]
Does clearing tpebs_cmd.pid before waiting for the reader thread to finish
cause valid samples to be silently dropped?

When tpebs_stop() clears the PID and releases tpebs_mtx, the reader thread
can run concurrently and drain the pipe. Because process_sample_event()
acquires the mutex and checks if tpebs_cmd.pid is 0, it may erroneously
assume the record has entirely terminated:

tools/perf/util/intel-tpebs.c:process_sample_event() {
    ...
	mutex_lock(tpebs_mtx_get());
	if (tpebs_cmd.pid == 0) {
		/* Record has terminated. */
		mutex_unlock(tpebs_mtx_get());
		return 0;
	}
    ...
}

Should tpebs_cmd.pid remain set until after the reader thread is joined so
flushed samples are fully processed?

>  		mutex_lock(tpebs_mtx_get());
>  		if (control_fd[0] >= 0) {
[ ... ]
> @@ -567,12 +577,14 @@ 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;
>  		ret = tpebs_send_record_cmd(EVLIST_CTL_CMD_ENABLE_TAG);
>  	}
>  out:

[Severity: Medium]
If tpebs_send_record_cmd() fails, execution jumps to the "out:" error cleanup
block without joining the newly created thread.

In the error path, tpebs_cmd.pid is cleared:

tools/perf/util/intel-tpebs.c:evsel__tpebs_open() {
    ...
		if (started_process) {
			if (tpebs_cmd.pid > 0) {
				kill(tpebs_cmd.pid, SIGTERM);
				finish_command(&tpebs_cmd);
				tpebs_cmd.pid = 0;
			}
    ...
}

Since the error cleanup fails to join tpebs_reader_thread, and sets
tpebs_cmd.pid to 0 (which prevents tpebs_stop() from ever joining it later),
does this leak the thread resources?

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