Re: [PATCH] function_graph: Use the saved entry's size when reprinting it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but state desynchronization when reprinting a previously failed trace entry causes the current entry in the ring buffer to be incorrectly consumed and permanently dropped.
- [High] This problem wasn't introduced by this patch, but if the sequence buffer fills up while processing the final event in the ring buffer, `print_graph_entry()` records a failure state without saving the current entry, causing uninitialized or stale data reads.
- [High] This problem wasn't introduced by this patch, but the local variable `ret` in `print_graph_entry()` is declared as `static`, creating a data race when multiple trace instances or concurrent trace pipe readers are active.
--

commit ca8f0abc7d748db3bcb46f95c502d76a2725b949
Author: Donggeun Yoo <donggeunyoo.kernel@gmail.com>

function_graph: Use the saved entry's size when reprinting it

This patch fixes an issue where a saved graph entry was reprinted using the
size of the newly processed entry instead of its own original size. By
explicitly saving and restoring ent_size, it prevents argument loss or
truncation when reprinting.

> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index ff7cb1a76b953..c5befd0c5b9a2 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c

[ ... ]

> @@ -1274,6 +1275,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
>  		if (s->full) {
>  			data->failed = 1;
>  			data->cpu = cpu;
> +			data->ent_size = iter->ent_size;

[Severity: High]
This isn't a bug introduced by this patch, but if the sequence buffer fills
up while processing the final event in the ring buffer, could this record a
failure state without saving the current entry, causing uninitialized or
stale data reads?

In get_return_for_leaf(), if the ring buffer is empty on peek, it returns
NULL early:

	if (!event)
		return NULL;

	next = ring_buffer_event_data(event);

	if (data) {
		/*
		 * Save current and next entries for later reference
		 * if the output fails.
		 */
		int size = min_t(int, sizeof(data->rent), iter->ent_size);

		memcpy(&data->rent, curr, size);

Because the memcpy() is bypassed on the last event, setting data->failed = 1
unconditionally here on buffer full assumes the entry was successfully saved.
On the next read iteration, the trace output may print uninitialized memory
from data->ent.ent.

[Severity: High]
This is a pre-existing issue, but does declaring ret as a static local
variable in print_graph_entry() create a data race?

static enum print_line_t
print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
			struct trace_iterator *iter, u32 flags)
{
	struct fgraph_data *data = iter->private;
	struct ftrace_graph_ent *call;
	struct ftrace_graph_ret_entry *leaf_ret;
	static enum print_line_t ret;

If multiple userspace processes concurrently read from different instances of
trace_pipe, they execute print_graph_entry() in parallel. If one thread
assigns TRACE_TYPE_PARTIAL_LINE to ret but another thread immediately
overwrites it with TRACE_TYPE_HANDLED, the first thread will incorrectly
return TRACE_TYPE_HANDLED, leading the tracing core to improperly consume an
unprinted event.

>  		} else
>  			data->failed = 0;
>  	}
> @@ -1457,6 +1459,7 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
>  	if (data && data->failed) {
>  		field = &data->ent.ent;
>  		iter->cpu = data->cpu;
> +		iter->ent_size = data->ent_size;
>  		ret = print_graph_entry(field, s, iter, flags);
>  		if (ret == TRACE_TYPE_HANDLED && iter->cpu != cpu) {
>  			per_cpu_ptr(data->cpu_data, iter->cpu)->ignore = 1;

[Severity: High]
This is a pre-existing issue, but could this logic cause trace events to be
permanently dropped without being printed?

For trace_pipe reads, get_return_for_leaf() consumes the current entry from
the ring buffer early to peek at the next one:

			/*
			 * We need to consume the current entry to see
			 * the next one.
			 */
			ring_buffer_consume(iter->array_buffer->buffer, iter->cpu,
					    NULL, NULL);

When reprinting a previously failed trace entry here, if the new event is on
the same CPU (iter->cpu == cpu), the function fails to set
TRACE_TYPE_NO_CONSUME in print_graph_function_flags():

		if (ret == TRACE_TYPE_HANDLED && iter->cpu != cpu) {
			per_cpu_ptr(data->cpu_data, iter->cpu)->ignore = 1;
			ret = TRACE_TYPE_NO_CONSUME;
		}
		iter->cpu = cpu;
		return ret;

Because it returns TRACE_TYPE_HANDLED, the generic iterator assumes the
current entry was handled and consumes it from the ring buffer, permanently
dropping it without printing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906034406.1335316-1-donggeunyoo.kernel@gmail.com?part=1