Re: [PATCH v4 2/4] ftrace: Add support for function argument to graph tracer

[Sasha Levin]

On Wed, Aug 13, 2025 at 07:53:17PM -0400, Steven Rostedt wrote:
> On Fri, 8 Aug 2025 22:24:05 -0400
> Sasha Levin <sashal@xxxxxxxxxx> wrote:
>
> > So we've added a dynamically sized array to the end of
> > ftrace_graph_ent_entry, but in struct fgraph_data, the saved entry is
> > defined as:
> >
> >    struct fgraph_data {
> >        ...
> >        union {
> >            struct ftrace_graph_ent_entry ent;
> >            struct fgraph_retaddr_ent_entry rent;
> >        } ent;
> >        ...
> >    }
> >
> > Which doesn't seem to have room for args?
>
> No it doesn't :-p
>
> > The code in get_return_for_leaf() does:
> >
> >    data->ent.ent = *curr;
> >
> > This copies the struct, but curr points to a larger entry with args
> > data. The copy operation only copies sizeof(struct
> > ftrace_graph_ent_entry) bytes, which doesn't include the dynamic args
> > array.
> >
> > And then later functions (like print_graph_entry()) would go ahead and
> > assume that iter->ent_size is sane and make a mess out of everything.
> >
> > I can't test right now whether this actually fixes the issues or not,
> > but I wanted to bring this up as this looks somewhat odd and I'm not too
> > familiar with this code.
>
> Thanks for the detail analysis, can you test this patch?
>
> -- Steve
>
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 66e1a527cf1a..25ea71edb8da 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -35,6 +35,11 @@ struct fgraph_data {
> 		struct ftrace_graph_ent_entry	ent;
> 		struct fgraph_retaddr_ent_entry	rent;
> 	} ent;
> +	/*
> +	 * The @args must be right after @ent, as it is where they
> +	 * are stored in case the function graph tracer has arguments.
> +	 */
> +	unsigned long			args[FTRACE_REGS_MAX_ARGS];
> 	struct ftrace_graph_ret_entry	ret;
> 	int				failed;
> 	int				cpu;
> @@ -623,14 +628,29 @@ get_return_for_leaf(struct trace_iterator *iter,
> 		next = ring_buffer_event_data(event);
>
> 		if (data) {
> +			int args_size;
> +			int size;
> +
> 			/*
> 			 * Save current and next entries for later reference
> 			 * if the output fails.
> 			 */
> -			if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT))
> +			if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
> 				data->ent.rent = *(struct fgraph_retaddr_ent_entry *)curr;
> -			else
> +				size = offsetof(struct fgraph_retaddr_ent_entry, args);
> +			} else {
> 				data->ent.ent = *curr;
> +				size = offsetof(struct ftrace_graph_ent_entry, args);
> +			}
> +
> +			/* If this has args, then append them to after the ent. */
> +			args_size = iter->ent_size - size;
> +			if (args_size > sizeof(long) * FTRACE_REGS_MAX_ARGS)
> +				args_size = sizeof(long) * FTRACE_REGS_MAX_ARGS;
> +
> +			if (args_size >= sizeof(long))
> +				memcpy((void *)&data->ent.ent + size,
> +				       (void*)curr + size, args_size);
> 			/*
> 			 * If the next event is not a return type, then
> 			 * we only care about what type it is. Otherwise we can

Got a small build error:

kernel/trace/trace_functions_graph.c: In function ‘get_return_for_leaf’:
./include/linux/stddef.h:16:33: error: ‘struct fgraph_retaddr_ent_entry’ has no member named ‘args’
   16 | #define offsetof(TYPE, MEMBER)  __builtin_offsetof(TYPE, MEMBER)
      |                                 ^~~~~~~~~~~~~~~~~~
kernel/trace/trace_functions_graph.c:640:40: note: in expansion of macro ‘offsetof’
  640 |                                 size = offsetof(struct fgraph_retaddr_ent_entry, args);
      |                                        ^~~~~~~~

Does this look right on top of your patch:

diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 25ea71edb8da..f0f37356ef29 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -637,20 +637,21 @@ get_return_for_leaf(struct trace_iterator *iter,
                         */
                        if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
                                data->ent.rent = *(struct fgraph_retaddr_ent_entry *)curr;
-                               size = offsetof(struct fgraph_retaddr_ent_entry, args);
+                               /* fgraph_retaddr_ent_entry doesn't have args field */
+                               size = sizeof(struct fgraph_retaddr_ent_entry);
+                               args_size = 0;
                        } else {
                                data->ent.ent = *curr;
                                size = offsetof(struct ftrace_graph_ent_entry, args);
+                               /* If this has args, then append them to after the ent. */
+                               args_size = iter->ent_size - size;
+                               if (args_size > sizeof(long) * FTRACE_REGS_MAX_ARGS)
+                                       args_size = sizeof(long) * FTRACE_REGS_MAX_ARGS;
+
+                               if (args_size >= sizeof(long))
+                                       memcpy((void *)&data->ent.ent + size,
+                                              (void*)curr + size, args_size);
                        }
-
-                       /* If this has args, then append them to after the ent. */
-                       args_size = iter->ent_size - size;
-                       if (args_size > sizeof(long) * FTRACE_REGS_MAX_ARGS)
-                               args_size = sizeof(long) * FTRACE_REGS_MAX_ARGS;
-
-                       if (args_size >= sizeof(long))
-                               memcpy((void *)&data->ent.ent + size,
-                                      (void*)curr + size, args_size);
                        /*
                         * If the next event is not a return type, then
                         * we only care about what type it is. Otherwise we can

--
Thanks,
Sasha