Re: [PATCH] perf ftrace latency: Do not read trace files when BPF is used

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] API misuse where `IS_ERR()` is used to check libbpf attachment functions that return `NULL` on error, leading to completely bypassed error handling.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition between checking the `done` flag and blocking in `poll()` causes the `perf ftrace` tool to hang indefinitely.
--

commit 005a20a206112aa28a48619fcb3cf2e39a48a24a
Author: Namhyung Kim <namhyung@kernel.org>

perf ftrace latency: Do not read trace files when BPF is used

This commit modifies the perf ftrace latency command to avoid reading tracing
files when BPF is in use, as the tracing files are not set up and poll()
returns immediately. Instead, it introduces a small sleep loop to wait for
the target process to exit or a signal to arrive.

> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 4f881a40c311a..e61b9de11e1e4 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -1146,6 +1146,11 @@ static int __cmd_latency(struct perf_ftrace *ftrace)
>  
>  	line[0] = '\0';
>  	while (!done) {
> +		if (ftrace->target.use_bpf) {
> +			usleep(1000);
> +			continue;
> +		}
> +
>  		if (poll(&pollfd, 1, -1) < 0)
>  			break;

[Severity: High]
This isn't a bug introduced by this patch, but does this main loop have a
race condition between checking the done flag and blocking in poll?

If a terminating signal (like sigchld from a completed workload or sigint
from the user) is handled immediately after the while (!done) check but
before the poll(&pollfd, 1, -1) syscall, the done flag will be set but the
signal is consumed. 

The main thread then executes poll() with an infinite timeout (-1). 

Since the workload has exited, no new trace events will arrive on trace_fd,
causing poll() to block forever and freeze the tool until killed manually.

Could this be avoided by using ppoll() with a signal mask, or by using a
timeout instead of blocking indefinitely? 

This same pattern also appears to be present in the standard trace command
loop in __cmd_ftrace().


While reviewing the BPF functionality for ftrace, I also noticed a potential
issue in perf_ftrace__latency_prepare_bpf():

tools/perf/util/bpf_ftrace.c:perf_ftrace__latency_prepare_bpf() {
	...
	if (func) {
		skel->links.func_begin = bpf_program__attach_kprobe(skel->progs.func_begin,
								    false, func->name);
		if (IS_ERR(skel->links.func_begin)) {
			pr_err("Failed to attach fentry program\n");
			err = PTR_ERR(skel->links.func_begin);
			goto out;
		}
	...
}

[Severity: High]
This is a pre-existing issue, but does this error check work as intended?

Functions like bpf_program__attach_kprobe() and
bpf_program__attach_raw_tracepoint() typically return NULL on error and set
errno.

If the attachment fails, IS_ERR(NULL) evaluates to false, which bypasses
this error handling path entirely. 

This would cause the tool to falsely report success while silently failing
to collect data.

Should these attachment checks compare against NULL instead? 

A similar check is also used for skel->links.event_begin later in the same
function.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826185815.280451-1-namhyung@kernel.org?part=1