Re: [PATCH v1 09/10] perf config, intel-tpebs, env: Add __guarded_by and __must_hold annotations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf: double close of tpebs_cmd.out file descriptor
--

commit 6138e7835465a4c85543c9abbf52c07f583bbb7a
Author: Ian Rogers <irogers@google.com>

perf config, intel-tpebs, env: Add __guarded_by and __must_hold annotations

Add thread safety annotations to the static parser and config state,
the TPEBS state, and the bpf_progs and cgroups trees. It also fixes some
unlocked reads and modernizes mutex initialization.

> diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> --- a/tools/perf/util/intel-tpebs.c
> +++ b/tools/perf/util/intel-tpebs.c
> @@ -331,13 +317,15 @@ 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;
> -		mutex_unlock(tpebs_mtx_get());
> -		pthread_join(tpebs_reader_thread, NULL);
> -		mutex_lock(tpebs_mtx_get());
> +		reader_thread = tpebs_reader_thread;
> +		mutex_unlock(&tpebs_mtx);
> +		pthread_join(reader_thread, NULL);
> +		mutex_lock(&tpebs_mtx);
>  		if (control_fd[0] >= 0) {

[Severity: High]
Could this sequence result in a double close of the tpebs_cmd.out file
descriptor?

The reader thread __sample_reader() assigns tpebs_cmd.out to data.file.fd
and passes it to perf_session__new(). When the thread finishes, it calls
perf_session__delete(), which invokes perf_data__close() and closes the
underlying file descriptor:

tools/perf/util/data.c:perf_data_file__close() {
...
	} else {
		close(file->fd);
		file->fd = -1;
	}
...
}

Meanwhile, after pthread_join() completes here in tpebs_stop(), the global
tpebs_cmd.out variable is still >= 0 because the reader thread only closed
its local struct copy without updating the global state.

This causes tpebs_stop() to explicitly call close() a second time just
below this block:

tools/perf/util/intel-tpebs.c:tpebs_stop() {
...
		if (tpebs_cmd.out >= 0) {
			close(tpebs_cmd.out);
			tpebs_cmd.out = -1;
		}
...
}

If another thread is assigned the same fd number in the window between the
first close in __sample_reader() and this second close in tpebs_stop(),
the second close could silently destroy an unrelated resource.

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