Re: [PATCH v13 15/21] 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 and broken error cleanup in
  evsel__tpebs_open()
--

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

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

This commit fixes lifecycle races around the background perf record
process, sample reader thread, and control/ack file descriptors in
intel-tpebs.

> diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
> index 7c0b8b15bd10e..5cb090b27fbd7 100644
> --- a/tools/perf/util/intel-tpebs.c
> +++ b/tools/perf/util/intel-tpebs.c

[ ... ]

> @@ -567,13 +635,22 @@ 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);
> +		if (!ret) {
> +			if (tpebs_gen != open_gen)
> +				ret = -EBUSY;
> +			else
> +				tpebs_enabled = true;
> +		}
>  	}
>  out:
>  	if (ret) {
> @@ -584,11 +661,25 @@ int evsel__tpebs_open(struct evsel *evsel)
>  			tpebs_retire_lat__delete(t);
>  		}
>  
> -		if (started_process) {
> -			if (tpebs_cmd.pid > 0) {
> -				kill(tpebs_cmd.pid, SIGTERM);
> -				finish_command(&tpebs_cmd);
> +		if (started_process && tpebs_gen == open_gen) {
> +			pid_t actual_pid = tpebs_cmd.pid;

[Severity: Medium]
Can concurrent or failing calls to evsel__tpebs_open() corrupt
tpebs_results or return before events are enabled?

First, evsel__tpebs_start_perf_record() sets tpebs_cmd.pid and marks all
entries in tpebs_results with t->started = true before
tpebs_send_record_cmd(EVLIST_CTL_CMD_ENABLE_TAG) drops tpebs_mtx:

tools/perf/util/intel-tpebs.c:evsel__tpebs_open() {
    ...
	if (tpebs_cmd.pid != 0) {
		struct tpebs_retire_lat *t;
		bool valid;

		t = tpebs_retire_lat__find(evsel);
		valid = t && t->started;
		mutex_unlock(tpebs_mtx_get());
		/* May fail as the event wasn't started. */
		return valid ? 0 : -EBUSY;
	}
    ...
}

A second caller entering evsel__tpebs_open() while ENABLE is in flight
sees tpebs_cmd.pid != 0 and t->started == true without checking
tpebs_enabled, returning 0 before events are enabled (or even if ENABLE
then fails).

Second, in the out: error path, tpebs_retire_lat__find(evsel) removes and
deletes t outside the (started_process && tpebs_gen == open_gen) guard:

tools/perf/util/intel-tpebs.c:evsel__tpebs_open() {
    ...
	mutex_lock(tpebs_mtx_get());
	if (tpebs_stopping || tpebs_cmd.pid != 0) {
		ret = -EBUSY;
		goto out;
	}
    ...
out:
	if (ret) {
		struct tpebs_retire_lat *t = tpebs_retire_lat__find(evsel);

		if (t) {
			list_del_init(&t->nd);
			tpebs_retire_lat__delete(t);
		}

		if (started_process && tpebs_gen == open_gen) {
    ...
}

If a second thread races through evsel__tpebs_prepare() and hits
tpebs_cmd.pid != 0 (or if tpebs_gen != open_gen after a concurrent stop
and reopen), jumping to out: deletes the active tpebs_retire_lat entry
owned by the running session.  When that entry is the only one in
tpebs_results, a later evsel__tpebs_close() will not find it and will
never call tpebs_stop(), leaking the background perf record process,
reader thread, and pipes.

Conversely, when (started_process && tpebs_gen == open_gen) is true on
failure, evsel__tpebs_prepare() added all retire_lat evsels on
evsel->evlist to tpebs_results, but out: only deletes the single evsel
passed to evsel__tpebs_open(), leaving the sibling entries behind with
stale started state.

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