Re: [PATCH v4 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 1 potential issue(s) to consider:
- [Medium] perf intel-tpebs: tpebs_cmd.pid left as -1 on start_command() failure
--

commit b2fed373fbeb105d8b92b7db69497b84b98deb1e
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
ownership of tpebs_cmd.out to __sample_reader() to prevent a double-close,
and updates error handling in evsel__tpebs_start_perf_record() and
evsel__tpebs_open().

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

[ ... ]

> @@ -119,6 +125,8 @@ static int evsel__tpebs_start_perf_record(struct evsel *evsel)
>  	tpebs_cmd.argv = record_argv;
>  	tpebs_cmd.out = -1;
>  	ret = start_command(&tpebs_cmd);
> +	if (ret)
> +		tpebs_cmd.out = -1;
>  	zfree(&tpebs_cmd.argv);
>  	list_for_each_entry(t, &tpebs_results, nd)
>  		t->started = true;

[Severity: Medium]
Should tpebs_cmd.pid also be reset to 0 when start_command() fails (and
should setting t->started = true be skipped)?

In tools/lib/subcmd/run-command.c:start_command(), when fork() fails after
pipe(fdout) succeeds, cmd->pid is left set to -1:

tools/lib/subcmd/run-command.c:start_command() {
    ...
	cmd->pid = fork();
	if (!cmd->pid) {
        ...
	}
	if (cmd->pid < 0) {
		int err = errno;
        ...
		if (need_out)
			close_pair(fdout);
		else if (cmd->out)
			close(cmd->out);
        ...
		return err == ENOENT ?
			-ERR_RUN_COMMAND_EXEC :
			-ERR_RUN_COMMAND_FORK;
	}
}

On the error path in evsel__tpebs_open(), tpebs_cmd.pid is only cleared when
tpebs_cmd.pid > 0, leaving tpebs_cmd.pid == -1 and any remaining entries added
by evsel__tpebs_prepare() on tpebs_results marked with t->started = true.

Because tpebs_stop() and the entry checks in evsel__tpebs_open() test
tpebs_cmd.pid != 0, a subsequent evsel__tpebs_close() -> tpebs_stop() on the
remaining prepared evsels will see tpebs_cmd.pid != 0 (-1) and call
pthread_join() on tpebs_reader_thread even though pthread_create() was never
called:

> @@ -331,12 +341,19 @@ 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_results is empty when stopping; clear pid so
> +		 * process_sample_event() ignores any late samples while
> +		 * draining the pipe.
> +		 */
>  		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);
>  		mutex_lock(tpebs_mtx_get());

[ ... ]

> @@ -586,9 +607,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;
>  			}
>  			if (tpebs_cmd.out >= 0) {
>  				close(tpebs_cmd.out);

[ ... ]

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