Re: [PATCH perf-tools-next v11 05/10] perf ftrace: Support context switch tracking 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: broken min_raw_indent tracking in ftrace_task_state__rekey()
- [Medium] perf ftrace: non-contiguous unknown-PID splice in ftrace_task_state__rekey()
--

commit 242f56625a11f73350f70f6bfa60c779b6624a22
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Support context switch tracking in function graph tracer

This commit adds context switch banner parsing and per-task call stack state
tracking to the function graph tracer in perf ftrace, including task state
rekeying and stack splicing across CPU migrations.

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> index 95db57e70d11b..055b5eb6b8327 100644
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c
[ ... ]
> @@ -124,6 +129,208 @@ static int ftrace_task_state__grow_stack(struct ftrace_task_state *cs)
>  	return 0;
>  }
>  
> +/**
> + * ftrace_task_state__rekey - Reassign a task state to a new PID
> + * @ts: The task state to re-key
> + * @new_pid: The new process ID to associate with the state
> + *
> + * Rehash @ts under @new_pid. If a task state for @new_pid already exists,
> + * merge or replace it and update active CPU references accordingly.
> + *
> + * Return: A pointer to the active task state associated with @new_pid.
> + */
> +static struct ftrace_task_state *ftrace_task_state__rekey(struct ftrace_task_state *ts,
> +							  pid_t new_pid)
> +{
> +	struct ftrace_task_state *existing;
> +	u32 key;
> +	int j, c;
> +	int to_copy;
> +
> +	if (ts->pid == new_pid)
> +		return ts;
> +
> +	hlist_del_init(&ts->node);
> +
> +	existing = find_task_state(new_pid);
> +	if (existing) {
> +		int split_idx = existing->depth;
> +		int old_split_idx, post_count;
> +		int base_raw_indent, post_raw_indent;
> +
> +		for (c = 0; c < FTRACE_MAX_CPUS; c++) {
> +			if (ftrace_cpu_tasks[c] == ts)
> +				ftrace_cpu_tasks[c] = existing;
> +		}
> +
> +		/*
> +		 * Find the split point in existing where frames postdate ts.
> +		 * Frames 0 .. split_idx - 1 were pushed prior to ts.
> +		 * Frames split_idx .. existing->depth - 1 were pushed after ts.
> +		 */
> +		for (j = 0; j < existing->depth; j++) {
> +			if (existing->stack[j].seq > ts->seq) {
> +				split_idx = j;
> +				break;
> +			}
> +		}
> +		old_split_idx = split_idx;
> +		post_count = existing->depth - old_split_idx;
> +
> +		/*
> +		 * Prune pre-ts frames on existing that exited before or during ts
> +		 * (i.e. whose raw_indent >= base_raw_indent, along with any
> +		 * synthesized inlined frames).
> +		 */
> +		base_raw_indent = -1;
> +		if (ts->depth > 0)
> +			base_raw_indent = ts->stack[0].raw_indent;
> +		if (ts->min_raw_indent >= 0 &&
> +		    (base_raw_indent < 0 || ts->min_raw_indent < base_raw_indent))
> +			base_raw_indent = ts->min_raw_indent;
> +
> +		if (base_raw_indent >= 0) {
> +			while (split_idx > 0) {
> +				struct ftrace_stack_frame *top =
> +					&existing->stack[split_idx - 1];
> +
> +				if (top->inlined) {
> +					free(top->name);
> +					top->name = NULL;
> +					if (existing->inlined_depth > 0)
> +						existing->inlined_depth--;
> +					split_idx--;
> +					existing->depth--;
> +				} else if (top->raw_indent >= base_raw_indent) {
> +					free(top->name);
> +					top->name = NULL;
> +					split_idx--;
> +					existing->depth--;
> +				} else {
> +					break;
> +				}
> +			}
> +		}

[Severity: Medium]
Can this split_idx search and pre-ts pruning loop in
ftrace_task_state__rekey() drop active intermediate frames from
existing->stack when an initial unknown-PID state accumulates events both
before and after events on existing?

In the kernel's function_graph tracer, verif_pid() only emits a per-CPU
context switch banner when the traced PID on that CPU changes from its
previous traced PID. Consider when task P starts on CPU 0 (tracked under
ts = FTRACE_UNKNOWN_PID(0)), migrates to CPU 1, and then migrates back to
CPU 0 before any other task produces a trace event on CPU 0:

1. On CPU 0, task P pushes outer frames f1 (raw_indent = 2, seq = 1) and
   f2 (raw_indent = 4, seq = 2) onto ts (FTRACE_UNKNOWN_PID(0)).

2. Task P migrates to CPU 1, where a banner "other => P" binds CPU 1 to
   existing (PID P), and pushes intermediate frames f3 (raw_indent = 6,
   seq = 3) and f4 (raw_indent = 8, seq = 4) onto existing->stack.

3. Task P migrates back to CPU 0. Since no other task emitted a trace
   record on CPU 0 in the meantime, no banner is emitted on CPU 0 and
   CPU 0 remains bound to ts. Task P executes another traced event on
   CPU 0 at seq = 5 (for example, a leaf call or inner frame f5 at
   raw_indent = 10), updating ts->seq to 5.

4. Later, another task runs on CPU 0 and emits "0) P => next", triggering
   ftrace_process_fgraph_line() -> parse_context_switch() ->
   ftrace_task_state__rekey(ts, P).

Because ts->seq is now 5 while existing->stack frames f3 and f4 have
seq = 3 and 4, the loop checking existing->stack[j].seq > ts->seq finds no
match and leaves split_idx = existing->depth. That classifies f3 and f4 as
pre-ts frames, and the pruning loop then compares them against
base_raw_indent = ts->stack[0].raw_indent (2 from f1), freeing and dropping
the active intermediate frames f3 and f4.

> +
> +		/*
> +		 * If newer frames or exits exist on existing, prune any frames
> +		 * on ts that exited before them.
> +		 */
> +		post_raw_indent = -1;
> +		if (existing->seq > ts->seq) {
> +			if (post_count > 0)
> +				post_raw_indent = existing->stack[old_split_idx].raw_indent;
> +			if (existing->min_raw_indent >= 0 &&
> +			    (post_raw_indent < 0 || existing->min_raw_indent < post_raw_indent))
> +				post_raw_indent = existing->min_raw_indent;
> +		}
[ ... ]
> @@ -402,6 +725,9 @@ int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FIL
>  	if (is_exit) {
>  		char exit_name[128];
>  
> +		if (cs->min_raw_indent < 0 || leading_spaces < cs->min_raw_indent)
> +			cs->min_raw_indent = leading_spaces;
> +
>  		extract_exit_func_name(func_start, exit_name, sizeof(exit_name));
>  
>  		top_raw_indent = -1;
> @@ -479,7 +805,9 @@ int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FIL
>  			cs->stack[cs->depth].inlined = false;
>  			cs->stack[cs->depth].indent = leading_spaces;
>  			cs->stack[cs->depth].raw_indent = leading_spaces;
> +			cs->stack[cs->depth].seq = cs->seq;
>  			cs->depth++;
> +			cs->min_raw_indent = -1;
>  		}
>  	}

[Severity: Medium]
Does tracking min_raw_indent as a single unsequenced value that is reset to
-1 on function entry cause ftrace_task_state__rekey() to miscompute
post_raw_indent and base_raw_indent across CPU migrations?

There appear to be two ways this can corrupt the tracked call stack when
ftrace_process_fgraph_line() -> parse_context_switch() ->
ftrace_task_state__rekey() merges ts into existing:

1. In ftrace_task_state__rekey(), existing->min_raw_indent is folded into
   post_raw_indent whenever existing->seq > ts->seq, without checking
   whether that minimum exit indentation was actually recorded after
   ts->seq:

   - Suppose task P on CPU 0 (existing) exits a function at
     leading_spaces = 2 (seq = 1), setting existing->min_raw_indent = 2.
   - Task P migrates to CPU 1 (ts = FTRACE_UNKNOWN_PID(1)) and enters
     functions at indentations 2, 4, and 6 (seq = 2..4, so ts->seq = 4).
   - Task P migrates back to CPU 0 (existing) at seq = 5 and executes a
     leaf call at indent 8 or exits only the deepest frame at indent 6.
     Now existing->seq is 5 (> ts->seq), while existing->min_raw_indent is
     still 2 from seq = 1.
   - When CPU 1 later emits its context switch banner and calls
     ftrace_task_state__rekey(ts, P), existing->seq > ts->seq is true and
     post_raw_indent becomes existing->min_raw_indent (2), causing the
     post-ts pruning loop to pop and free the still-active frames on ts at
     indentations 2 and 4.

2. In ftrace_process_fgraph_line(), pushing a new frame onto cs->stack
   unconditionally sets cs->min_raw_indent = -1. If a task on ts (or
   existing after ts) first exits a pre-migration frame at a shallow
   indentation (such as leading_spaces = 2, setting cs->min_raw_indent = 2)
   and then enters a new function at a deeper indentation (such as
   leading_spaces = 4), resetting cs->min_raw_indent = -1 erases the
   shallow exit watermark. When ftrace_task_state__rekey() subsequently
   computes base_raw_indent or post_raw_indent, it only sees indentation 4
   and fails to prune the already-exited frame at indentation 2.

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