Re: [PATCH perf-tools-next v5 5/5] perf ftrace: Filter extraneous functions in function graph tracer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] perf ftrace: inaccurate target_root_depth capture causes premature filter termination
- [Medium] perf ftrace: deep target subtrees suppressed as orphans on stack overflow
- [Medium] perf ftrace: permanent stack desynchronization on dropped orphan exit
--

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

perf ftrace: Filter extraneous functions in function graph tracer
    
When tracing with the function graph tracer using a targeted filter
(-G or --graph-funcs), users occasionally observe unrelated functions in
the trace output:

[ ... ]

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> index 71f00d393349e..90d0e3c9f8b5b 100644
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c

[ ... ]

> @@ -476,12 +514,23 @@ int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FIL
>  	if (is_exit) {
>  		char exit_name[128];
>  
> +		if (cs->orphan_depth > 0) {
> +			cs->orphan_depth--;
> +			return 0;
> +		}

[Severity: Medium]
If an orphan call executes and increments cs->orphan_depth, but its
corresponding exit event is dropped by the kernel ring buffer,
cs->orphan_depth will remain > 0.

Will the next valid exit event (belonging to the target callgraph) hit this
block and be blindly consumed without popping its frame from cs->stack?

Could this lead to permanent desynchronization of the tracked call stack?

[ ... ]

> @@ -539,6 +601,98 @@ int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FIL
>  	has_open_brace = (strchr(func_start, '{') != NULL);
>  	has_semicolon = (strchr(func_start, ';') != NULL);
>  
> +	if (cs->orphan_depth > 0) {
> +		if (has_open_brace && !has_semicolon)
> +			cs->orphan_depth++;
> +		return 0;
> +	}
> +
> +	if (has_filter) {
> +		if (cs->depth == 0 || !cs->in_target) {
> +			char entry_name[128];
> +
> +			extract_entry_func_name(func_start, entry_name, sizeof(entry_name));
> +			if (ftrace_is_graph_func(ftrace, entry_name)) {
> +				cs->in_target = true;
> +				cs->target_root_depth = cs->depth;

[Severity: Medium]
Since cs->target_root_depth is captured here as the current cs->depth, what
happens if the --inline option is used?

Can subsequent inline resolution logic pop stale inline frames from previous
sibling calls, decrementing cs->depth before the target root itself is pushed?

If so, could the target root end up being pushed at a depth lower than the
captured target_root_depth, causing the target filter to prematurely
terminate when a child of the target root exits (since
cs->depth <= cs->target_root_depth would evaluate to true)?

> +				cs->orphan_depth = 0;
> +				is_target_root = true;
> +			} else if (cs->depth == 0) {
> +				cs->in_target = false;
> +				cs->orphan_depth = 0;
> +			}
> +		}

[ ... ]

> +	if (!ftrace->use_inline) {
> +		if (has_filter && cs->in_target && cs->depth > cs->target_root_depth) {
> +			if (ftrace_parse_retaddr(func_start, caller_sym,
> +						 sizeof(caller_sym), &caller_offset)) {
> +				caller_idx = -1;
> +				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;
> +					}
> +				}
> +				if (caller_idx < cs->target_root_depth) {
> +					if (has_open_brace && !has_semicolon)
> +						cs->orphan_depth = 1;
> +					return 0;
> +				}

[Severity: Medium]
If the call stack depth exceeds FTRACE_MAX_STACK, cs->overflow_depth is
incremented instead of pushing new frames to cs->stack.

When a subsequent legitimate child call is processed, its caller won't be
found in the stack, leaving caller_idx as -1.

Will this cause caller_idx < cs->target_root_depth to evaluate to true,
incorrectly flagging the valid child as an orphan and suppressing it and
all its subtrees?

[ ... ]

> @@ -565,6 +719,21 @@ int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FIL
>  		}
>  	}
>  
> +	/*
> +	 * When inside a target graph filter, child functions must descend
> +	 * from an active frame on the target stack. Suppress detached
> +	 * calls originating outside the target hierarchy.
> +	 */
> +	if (has_filter && cs->in_target && cs->depth > cs->target_root_depth) {
> +		if (has_retaddr && caller_idx < cs->target_root_depth) {
> +			if (has_open_brace && !has_semicolon)
> +				cs->orphan_depth = 1;
> +			for (i = 0; i < num_inlines; i++)
> +				free((void *)inlined_names[i]);
> +			return 0;
> +		}
> +	}

[Severity: Medium]
Similar to the non-inline case, if the stack has overflowed FTRACE_MAX_STACK,
caller_idx will be -1 because the parent frame was never pushed to the stack.

Does this mean caller_idx < cs->target_root_depth will incorrectly evaluate
to true, wrongly classifying deep legitimate subtrees as orphans and dropping
them?

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