Re: [PATCH perf-tools-next v13 10/10] 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: unmatched IRQ return marker leaked in ftrace_process_fgraph_line()
- [Medium] perf ftrace: premature orphan_depth reset on nested exit in ftrace_process_fgraph_line()
- [Medium] perf ftrace: ancestor frames discarded on unwind in ftrace_check_target_caller()
--

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

perf ftrace: Filter extraneous functions in function graph tracer

This commit adds per-task target callgraph state tracking and return-address
caller validation in userspace for perf ftrace to suppress unrelated top-level
functions and detached orphan calls when filtering with -G / --graph-funcs.

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> index c2cf4a20553e2..a919307920321 100644
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c

[ ... ]

> @@ -879,6 +1087,208 @@ static int print_line(struct perf_ftrace *ftrace, const char *line, FILE *out)

[ ... ]

> +static int ftrace_check_target_caller(struct perf_ftrace *ftrace,
> +				      struct ftrace_task_state *ts,
> +				      const char *func_start,
> +				      const char *entry_name,
> +				      bool is_graph_target,
> +				      int leading_spaces,
> +				      bool has_open_brace,
> +				      bool has_semicolon,
> +				      bool has_retaddr,
> +				      int caller_idx,
> +				      const char *blank_prefix,
> +				      FILE *out,
> +				      bool *is_target_root)
> +{
> +	if (!ftrace_has_graph_funcs(ftrace) || !ts->in_target ||
> +	    ts->depth <= ts->target_root_depth)
> +		return 0;
> +
> +	if (has_retaddr && caller_idx >= 0 && caller_idx < ts->target_root_depth) {
> +		ftrace_unwind_to_depth(ts, caller_idx + 1, blank_prefix, out);
> +		if (is_graph_target) {
> +			ftrace_reset_to_target_root(ts, entry_name, ts->depth,
> +						    leading_spaces);
> +			*is_target_root = true;
> +			return 0;
> +		}
> +		reset_target_root_state(ts);
> +		if (has_open_brace && !has_semicolon &&
> +		    push_nontarget_frame(ts, func_start, leading_spaces) < 0)
> +			return -ENOMEM;
> +		return 1;
> +	}
> +
> +	if (leading_spaces <= ts->target_root_indent) {
> +		ftrace_unwind_to_depth(ts, 0, blank_prefix, out);
> +		if (is_graph_target) {
> +			ftrace_reset_to_target_root(ts, entry_name, 0,
> +						    leading_spaces);
> +			*is_target_root = true;
> +			return 0;
> +		}
> +		reset_target_root_state(ts);\n> +		if (has_open_brace && !has_semicolon &&
> +		    push_nontarget_frame(ts, func_start, leading_spaces) < 0)
> +			return -ENOMEM;
> +		return 1;
> +	}

[Severity: Medium]
When recovering from a dropped target root exit in
ftrace_check_target_caller() because leading_spaces <= ts->target_root_indent
(with caller_idx < 0, such as when funcgraph-retaddr is unavailable or the
call goes through an untraced wrapper), does calling
ftrace_unwind_to_depth(ts, 0, blank_prefix, out) discard active non-target
ancestor frames below ts->target_root_depth whose indentation is strictly less
than leading_spaces?

If an outer ancestor was pushed onto ts->stack before the target root entered,
unwinding all the way to 0 pops that ancestor frame as well. If that ancestor
subsequently calls another function or re-enters a target function, caller_idx
lookup will fail and suppress inline frame synthesis (--inline) when resolving
inlines in ftrace_process_fgraph_line():

	if (caller_idx < 0)
		num_inlines = 0;

Should this unwind to ts->target_root_depth first and then pop only non-target
ancestor frames with indent >= leading_spaces, matching the exit recovery path
in ftrace_process_fgraph_line()?

> +
> +	if (has_retaddr) {
> +		if (caller_idx < 0) {
> +			if (ts->pending_irq_ent[0] != '\0') {
> +				ts->pending_irq_ent[0] = '\0';
> +				ts->suppress_irq_ret = true;
> +			}
> +			if (has_open_brace && !has_semicolon) {
> +				ts->orphan_depth = 1;
> +				ts->orphan_base_indent = leading_spaces;
> +				ts->orphan_seq = ts->seq;
> +			}
> +			return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * ftrace_process_fgraph_line - Process and output a function graph trace line
>   * @ftrace: Perf ftrace context
> @@ -910,16 +1320,54 @@ int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FIL

[ ... ]

>  	if ((!has_filter || ftrace->graph_thresh) && !ftrace->use_inline)
>  		return print_line(ftrace, line, out);
>  
> +	if (strstr(line, "==========>") != NULL ||
> +	    strstr(line, "<==========") != NULL) {
> +		bool is_irq_ent = (strstr(line, "==========>") != NULL);
> +
> +		if (has_filter && parse_graph_cpu(ftrace, line, &cpu)) {
> +			ts = get_cpu_state(ftrace, cpu);
> +			if (ts && ts->in_target && ts->orphan_depth == 0) {
> +				if (is_irq_ent) {
> +					if (ts->pending_irq_ent[0] != '\0')
> +						print_line(ftrace, ts->pending_irq_ent, out);
> +					strlcpy(ts->pending_irq_ent, line,
> +						sizeof(ts->pending_irq_ent));
> +					ts->suppress_irq_ret = false;
> +				} else {
> +					if (ts->pending_irq_ent[0] != '\0') {
> +						print_line(ftrace, ts->pending_irq_ent, out);
> +						ts->pending_irq_ent[0] = '\0';
> +					}
> +					if (ts->suppress_irq_ret)
> +						ts->suppress_irq_ret = false;
> +					else
> +						print_line(ftrace, line, out);
> +				}
> +			} else if (ts && !ts->in_target) {
> +				ts->pending_irq_ent[0] = '\0';
> +				ts->suppress_irq_ret = false;
> +			}
> +		} else if (!has_filter) {
> +			print_line(ftrace, line, out);
> +		}
> +		return 0;
> +	}

[ ... ]

> @@ -950,30 +1398,31 @@ int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FIL
>  
>  		extract_exit_func_name(func_start, exit_name, sizeof(exit_name));
>  
> -		top_indent = -1;
> -		for (i = ts->depth - 1; i >= 0; i--) {
> -			if (!ts->stack[i].inlined) {
> -				top_indent = ts->stack[i].indent;
> -				break;
> +		if (ts->orphan_depth > 0) {
> +			if (has_filter && ts->in_target &&
> +			    (leading_spaces <= ts->target_root_indent ||
> +			     (ts->orphan_base_indent > 0 &&
> +			      leading_spaces < ts->orphan_base_indent))) {
> +				ts->orphan_depth = 0;
> +				ts->orphan_base_indent = 0;
> +				ts->orphan_seq = 0;
> +			} else {
> +				if (ts->orphan_base_indent > 0 &&
> +				    leading_spaces == ts->orphan_base_indent)
> +					ts->orphan_depth = 0;
> +				else
> +					ts->orphan_depth--;
> +				if (ts->orphan_depth == 0) {
> +					ts->orphan_base_indent = 0;
> +					ts->orphan_seq = 0;
> +				}
> +				return 0;
>  			}
>  		}

[Severity: Medium]
Can a dropped child entry inside an orphan subtree cause ts->orphan_depth to
reach 0 prematurely in ftrace_process_fgraph_line()?

When an orphan root frame enters at indentation I, ftrace_check_target_caller()
sets ts->orphan_depth = 1 and ts->orphan_base_indent = I. If a nested child
inside that orphan subtree at indentation > I has its entry event dropped by
the ring buffer, ts->orphan_depth remains 1.

When that nested child exits with leading_spaces > ts->orphan_base_indent, the
else branch decrements ts->orphan_depth from 1 to 0 and clears
ts->orphan_base_indent = 0, even though leading_spaces > ts->orphan_base_indent
indicates execution is still deeper than the outermost orphan frame. Could
subsequent calls or exits within the still-active outer orphan frame then leak
into the output or pop active target frames?

[ ... ]

> @@ -1083,9 +1551,166 @@ int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FIL

[ ... ]

> +		if (ts->orphan_depth > 0) {
> +			if (has_open_brace && !has_semicolon) {
> +				ts->orphan_depth++;
> +				ts->orphan_seq = ts->seq;
> +			}
> +			return 0;
> +		}
> +	}
> +
> +	ts->suppress_irq_ret = false;

[Severity: Medium]
If an interrupt section contains more than one top-level orphan call between
==========> and <========== (for example, when an untraced interrupt entry
sequentially invokes irq_enter_rcu(), the interrupt handler, and
irq_exit_rcu()), will this unconditional reset of ts->suppress_irq_ret = false
in ftrace_process_fgraph_line() leak a dangling <========== marker?

On the first orphan call after ==========>, ftrace_check_target_caller() sees
ts->pending_irq_ent[0] != '\0', clears ts->pending_irq_ent[0] = '\0', and sets
ts->suppress_irq_ret = true.

Once that first orphan call finishes (either immediately if it is a leaf call,
or when its closing brace returns ts->orphan_depth to 0), the entry of the
second top-level orphan call reaches:

	ts->suppress_irq_ret = false;

before calling ftrace_check_target_caller(). Because ts->pending_irq_ent[0] is
already '\0', ftrace_check_target_caller() suppresses the second orphan call
without setting ts->suppress_irq_ret back to true, causing the subsequent
<========== line to be printed without a matching opening marker.

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