Re: [PATCH perf-tools-next v2 3/4] perf ftrace: Support display of inlined functions in function graph tracer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] perf: fgraph inline tracking fails to close frames if num_inlines == 0
- [Medium] perf: fgraph stack unwinding fails permanently on dropped trace events
- [Low] perf: unmatched exit events erroneously pop active inlined frames
- [Low] perf: memory leak of trace buffers on open failure
- [Low] perf tests: memory leak on inline processing test failure
--

commit d46c8024964e46cbda045d2aa35a4276291e5d25
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Support display of inlined functions in function graph tracer

This commit introduces the --inline option to perf ftrace for the
function_graph tracer. It resolves the return address to its inlined
callchain using DWARF debug information and synthesizes intermediate
inlined frames directly into the streamed call graph.

> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
[ ... ]
> @@ -769,6 +774,9 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  		goto out_reset;
>  	}
>  
> +	if (perf_ftrace__setup_inlines(ftrace) < 0)
> +		goto out_reset;
> +
>  	setup_pager();
>  
>  	buf = malloc(TRACE_BUF_SIZE);
> @@ -827,9 +835,10 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
[ ... ]
>  	trace_file = get_tracing_instance_file("trace_pipe");
>  	if (!trace_file) {
>  		pr_err("failed to open trace_pipe\n");
>  		goto out_reset;
>  	}
>  
>  	trace_fd = open(trace_file, O_RDONLY);
>  
>  	put_tracing_file(trace_file);
>  
>  	if (trace_fd < 0) {
>  		pr_err("failed to open trace_pipe\n");
>  		goto out_reset;
>  	}

[Severity: Low]
Does this error path leak the trace buffers?

In __cmd_ftrace(), buf is allocated and linebuf is initialized just before
opening the trace pipe. If get_tracing_instance_file() or open() fails, the
code jumps directly to out_reset, which bypasses the out_close_fd block
where free(buf) and strbuf_release(&linebuf) are called.

> --- a/tools/perf/tests/ftrace.c
> +++ b/tools/perf/tests/ftrace.c
[ ... ]
> +		/* Feed simulated function graph trace lines */
> +		ftrace_process_fgraph_line(&ftrace, line_entry, out);
> +		ftrace_process_fgraph_line(&ftrace, line_enqueue, out);
> +		ftrace_process_fgraph_line(&ftrace, line_exit_enqueue, out);
> +		ftrace_process_fgraph_line(&ftrace, line_exit_task, out);
> +
> +		fclose(out);
> +		ftrace_cpu_states__clear();
> +
> +		pr_debug("Processed output:\n%s\n", out_buf);
> +
> +		/* Verify activate_task entry with inline hint */
> +		TEST_ASSERT_VAL("contains activate_task entry with inline hint",
> +				strstr(out_buf, "activate_task() { /* (inline) */") != NULL);

[Severity: Low]
Can a failure here leak memory and machine resources during tests?

If TEST_ASSERT_VAL fails, it immediately returns TEST_FAIL. Since
test_inline_processing() allocates ftrace.machine and out_buf, returning
early bypasses free(out_buf) and perf_ftrace__cleanup_inlines(&ftrace).

> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c
[ ... ]
> +	/* Determine which inlined frames are currently open above the caller */
> +	caller_idx = -1;
> +	if (num_inlines > 0) {
> +		for (i = cs->depth - 1; i >= 0; i--) {
> +			if (!cs->stack[i].inlined && cs->stack[i].name &&
> +			    strcmp(cs->stack[i].name, caller_sym) == 0) {
> +				caller_idx = i;
> +				break;
> +			}
> +		}
> +	}

[Severity: Medium]
Does guarding this loop with if (num_inlines > 0) prevent finding the
caller when a non-inlined function executes?

If a non-inlined function runs, ftrace_resolve_inlines() returns
num_inlines == 0. Because of this check, the stack search is skipped and
caller_idx remains -1. 

As a result, active_inlines evaluates to 0, which bypasses the loop meant
to close stale inlined frames. This causes the new non-inlined function to
be incorrectly pushed to the stack with the extra indentation of the 
unclosed inlined function, disrupting the call graph hierarchy.

[ ... ]
> +		/* Close all inlined frames above match_idx */
> +		while (cs->depth > 0 && (match_idx < 0 || cs->depth - 1 > match_idx)) {

[Severity: Low]
Can spurious exit events erroneously pop active inlined frames?

If a trace output contains an unmatched or spurious exit event, the stack 
search finds no matching real frame, leaving match_idx as -1. The condition
(match_idx < 0 || cs->depth - 1 > match_idx) will evaluate to true, which
indiscriminately pops and closes currently active inlined frames from the
top of the stack until it hits a real frame.

> +			if (cs->stack[cs->depth - 1].inlined) {
> +				struct ftrace_stack_frame *top = &cs->stack[cs->depth - 1];
> +				int exit_indent = top->indent;
> +
> +				if (cs->inlined_depth > 0)
> +					cs->inlined_depth--;
> +				fprintf(out, "%s%*s} /* %s (inline) */\n",
> +					blank_prefix, exit_indent, "", top->name);
> +				free(top->name);
> +				cs->depth--;
> +			} else {
> +				break;
> +			}
> +		}

[Severity: Medium]
Does this loop permanently trap frames if it encounters a dropped trace event?

If an exit event for a non-inlined function is dropped (e.g., from a ring
buffer overrun), an orphaned non-inlined frame remains on the stack. When
this unwinding loop hits that orphaned frame (!cs->stack[...].inlined),
it calls break instead of popping it. 

Because cs->depth doesn't decrease past the orphaned frame, the subsequent
check for the correctly matched real frame fails, leaving both the matched
frame and orphaned frames permanently stuck on the stack until depth is
exhausted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006232756.65620-1-atomlin@atomlin.com?part=3