Re: [PATCH v11 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 2 potential
issues to consider:
- [Medium] perf intel-tpebs: control pipe command coalescing deadlock in
  tpebs_stop()
- [Low] perf intel-tpebs: race in process_sample_event() during
  tpebs_stop()
--

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

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

This commit serializes control command writes with teardown, transfers
ownership of tpebs_cmd.out to __sample_reader(), and guards
evsel__tpebs_open() against a concurrent tpebs_stop().

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

[ ... ]

> @@ -262,20 +291,20 @@ static int tpebs_send_record_cmd(const char *msg) __must_hold(tpebs_mtx_get())
>  
>  	/* Send perf record command.*/
>  	len = strlen(msg);
> -	ret = write(control_fd[1], msg, len);
> +	ret = write(ctrl_fd, msg, len);
>  	if (ret != len) {
>  		pr_err("perf record control write control message '%s' failed\n", msg);
>  		ret = -EPIPE;
>  		goto out;
>  	}
>  
> -	if (!strcmp(msg, EVLIST_CTL_CMD_STOP_TAG)) {
> +	if (is_stop) {
>  		ret = 0;
>  		goto out;
>  	}
>  
>  	/* Wait for an ack. */
> -	pollfd.fd = ack_fd[0];
> +	pollfd.fd = resp_fd;
>  
>  	/*
>  	 * We need this poll to ensure the ack_fd PIPE will not hang
> @@ -284,7 +313,17 @@ static int tpebs_send_record_cmd(const char *msg) __must_hold(tpebs_mtx_get())
>  	 */
>  again:
>  	if (!poll(&pollfd, 1, 500)) {
> -		if (check_if_command_finished(&tpebs_cmd)) {
> +		bool stopping, finished;
> +
> +		mutex_lock(tpebs_mtx_get());
> +		stopping = tpebs_stopping;
> +		finished = !stopping && check_if_command_finished(&tpebs_cmd);
> +		mutex_unlock(tpebs_mtx_get());
> +		if (stopping) {
> +			ret = -EBUSY;
> +			goto out;
> +		}

[Severity: Medium]
Can aborting the poll() wait on tpebs_stopping coalesce the in-flight
command with EVLIST_CTL_CMD_STOP_TAG in control_fd and hang tpebs_stop()
in pthread_join()?

tpebs_send_record_cmd() writes msg without a trailing newline or NUL
delimiter:

Thread 1                          Thread 2
evsel__tpebs_open()
  tpebs_send_record_cmd("enable")
    write(ctrl_fd, "enable", 6)
    poll(&pollfd, 1, 500)
                                  evsel__tpebs_close()
                                    tpebs_stop()
                                      tpebs_stopping = true
                                      tpebs_gen++
                                      tpebs_send_record_cmd("stop")
                                        cond_wait(&tpebs_cmd_cond, ...)
    /* poll() times out */
    stopping == true -> goto out
    tpebs_cmd_in_flight = false
    cond_broadcast(&tpebs_cmd_cond)
  /* tpebs_gen != open_gen, skips SIGTERM */
                                        write(ctrl_fd, "stop", 4)
                                      pthread_join(reader_thread, NULL)
                                        <-- hangs forever

When perf record reads from the control pipe, evlist__ctlfd_recv() reads
all available bytes until '\n', '\0', or EAGAIN into a single command
buffer:

tools/perf/util/evlist.c:evlist__ctlfd_recv() {
	do {
		err = read(evlist__ctl_fd_fd(evlist), &c, 1);
		if (err > 0) {
			if (c == '\n' || c == '\0')
				break;
			cmd_data[bytes_read++] = c;
    ...
}

If perf record has not consumed "enable" (or "ping") before "stop" is
written, it reads "enablestop" (or "pingstop") as one command and drops
the stop request.  Because evsel__tpebs_open() skips kill(..., SIGTERM)
once tpebs_gen has changed and tpebs_stop() waits in pthread_join()
before closing the pipes or killing the child, perf record never exits
and pthread_join() blocks forever.

[ ... ]

> @@ -331,12 +382,21 @@ 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_gen++;
> +		cond_broadcast(&tpebs_cmd_cond);
>  		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;

[Severity: Low]
Can process_sample_event() still see a non-zero tpebs_cmd.pid and return
-EINVAL while tpebs_send_record_cmd(EVLIST_CTL_CMD_STOP_TAG) drops
tpebs_mtx?

evsel__tpebs_close() empties tpebs_results before calling tpebs_stop(),
and tpebs_send_record_cmd() releases tpebs_mtx in cond_wait() and around
write().  Because tpebs_cmd.pid is only cleared after
tpebs_send_record_cmd() returns and process_sample_event() does not check
tpebs_stopping:

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;
	}
	t = tpebs_retire_lat__find(sample->evsel);
	if (!t) {
		mutex_unlock(tpebs_mtx_get());
		return -EINVAL;
	}
    ...
}

Any sample processed by tpebs_reader_thread during that window fails the
lookup in the empty tpebs_results list and returns -EINVAL, causing
__perf_session__process_pipe_events() to log an error and abort draining
the pipe early.  Checking tpebs_stopping in process_sample_event() would
avoid this race.

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