| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aJ4XX4qvHUZRAFxF@lappy>
Date: Thu, 14 Aug 2025 13:05:35 -0400
From: Sasha Levin <sashal@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
bpf@...r.kernel.org, Masami Hiramatsu <mhiramat@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Sven Schnelle <svens@...ux.ibm.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Guo Ren <guoren@...nel.org>,
Donglin Peng <dolinux.peng@...il.com>,
Zheng Yejian <zhengyejian@...weicloud.com>
Subject: Re: [PATCH v4 2/4] ftrace: Add support for function argument to
graph tracer
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@...nel.org> 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
Powered by blists - more mailing lists