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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf ftrace: unshifted target_root_depth in ftrace_task_state__rekey()
- [Low] perf tests ftrace: missing ensure_synthetic_inline() in test_graph_target_filter()
--

commit 79ad3a54d36210f71bcd95c62c94ed00575efdc0
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Filter extraneous functions in function graph tracer

This commit adds userspace callgraph target state tracking and return-address
caller validation to perf ftrace so that extraneous top-level functions and
detached orphan calls are suppressed when filtering with -G / --graph-funcs.
It also replaces the fixed-size call stack with a dynamically reallocated
array and adds unit tests in tools/perf/tests/ftrace.c.

> diff --git a/tools/perf/tests/ftrace.c b/tools/perf/tests/ftrace.c
> index 0aef2cd9b07fb..3591883b32041 100644
> --- a/tools/perf/tests/ftrace.c
> +++ b/tools/perf/tests/ftrace.c
> @@ -1492,6 +1492,2259 @@ static int test_context_switch_handling(void)
>  }
>  #undef TEST_CS_ASSERT
>  
> +static int test_graph_target_filter(void)
> +{
[ ... ]
> +	vmlinux = vmlinux_path__find();
> +
> +	/* 6. Test targeted leaf call with synthesized DWARF inlines closing and resetting filter */
> +	if (vmlinux) {
> +		const char *inlined_names[FTRACE_MAX_INLINES];
> +		int num_inlines;
> +		int j;
> +		const char *saved_vmlinux = symbol_conf.vmlinux_name;
> +		bool saved_ignore_buildid = symbol_conf.ignore_vmlinux_buildid;
> +
> +		symbol_conf.vmlinux_name = vmlinux;
> +		symbol_conf.ignore_vmlinux_buildid = true;
> +		if (perf_ftrace__setup_inlines(&ftrace) == 0 && ftrace.machine) {
> +			num_inlines = ftrace_resolve_inlines(ftrace.machine,
> +							     "wake_up_new_task", 0x1d1,
> +							     inlined_names,
> +							     FTRACE_MAX_INLINES);
> +			if (num_inlines > 0 && !strcmp(inlined_names[0], "activate_task")) {

[Severity: Low]
In test_graph_target_filter(), perf_ftrace__setup_inlines() creates a fresh
machine and loads vmlinux before querying ftrace_resolve_inlines() for
wake_up_new_task at offset 0x1d1.

Other inline tests in tools/perf/tests/ftrace.c, such as
test_inline_processing(), test_stack_saturation(), and
test_context_switch_handling(), call ensure_synthetic_inline() first:

	if (perf_ftrace__setup_inlines(&ftrace) == 0 && ftrace.machine) {
		ensure_synthetic_inline(ftrace.machine, "wake_up_new_task", 0x1d1,
					"activate_task");
		num_inlines = ftrace_resolve_inlines(ftrace.machine,
						     "wake_up_new_task", 0x1d1,
						     inlined_names,
						     FTRACE_MAX_INLINES);

Should test_graph_target_filter() also call ensure_synthetic_inline() before
ftrace_resolve_inlines() so that subtests 6, 6b, and 6c are not silently
skipped when the host vmlinux does not have activate_task inlined at exact
offset 0x1d1 of wake_up_new_task?

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> index 384fc03850f27..4e916096a9380 100644
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c
[ ... ]
> @@ -313,6 +345,26 @@ static struct ftrace_task_state *ftrace_task_state__rekey(struct ftrace_task_sta
>  			for (j = to_copy; j < ts->depth; j++)
>  				free(ts->stack[j].name);
>  
> +			if (ts->in_target &&
> +			    ts->target_root_frame_idx < ts->depth) {
> +				existing->in_target = true;
> +				existing->target_root_depth = ts->target_root_depth;
> +				existing->target_root_frame_idx =
> +					split_idx + ts->target_root_frame_idx;
> +				existing->target_root_indent = ts->target_root_indent;
> +				strlcpy(existing->target_root_name,
> +					ts->target_root_name,
> +					sizeof(existing->target_root_name));
> +				existing->orphan_depth = ts->orphan_depth;
> +			} else if (existing->in_target) {
> +				if (existing->target_root_frame_idx >= old_split_idx) {
> +					existing->target_root_frame_idx +=
> +						(split_idx + to_copy) - old_split_idx;
> +					existing->target_root_depth +=
> +						(split_idx + to_copy) - old_split_idx;
> +				}
> +			}
> +
>  			existing->depth = split_idx + to_copy + post_count;
>  		}

[Severity: Medium]
When ftrace_task_state__rekey() splices ts->stack[0 .. to_copy - 1] into
existing->stack at split_idx, existing->target_root_frame_idx is shifted by
split_idx, and the else-if branch shifts both target_root_frame_idx and
target_root_depth. However, in the ts->in_target branch,
existing->target_root_depth is set to ts->target_root_depth without adding
split_idx (and the guard checks ts->target_root_frame_idx < ts->depth rather
than to_copy).

If a task enters non-target ancestor frames on one CPU (leaving split_idx > 0
on existing->stack) and then migrates to another CPU where it enters a target
root function at ts->target_root_depth == 0 before the context-switch banner
is processed, existing->target_root_depth will remain 0 after the merge
instead of split_idx.

When that happens, won't ftrace_check_target_caller() and
ftrace_process_fgraph_line() treat the pre-migration non-target ancestor
frames at indices 0 .. split_idx - 1 as being at or above target_root_depth:

ftrace_check_target_caller()
	if (has_retaddr && caller_idx >= 0 && caller_idx < cs->target_root_depth) {
		ftrace_unwind_to_depth(cs, caller_idx + 1, blank_prefix, out);

ftrace_process_fgraph_line()
	if (!has_filter || (cs->in_target && match_idx >= cs->target_root_depth)) {

allowing non-target ancestor calls and exits to leak into the filtered output,
or unwinding into ancestor inline frames when the target root exits?

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