| 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: <CAErzpmsecSfSTKWHetu1rRnF08spunV_OLR4pKznFpB_QCYc8A@mail.gmail.com>
Date: Tue, 25 Nov 2025 10:05:54 +0800
From: Donglin Peng <dolinux.peng@...il.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-trace-kernel@...r.kernel.org, linux-kernel@...r.kernel.org,
Donglin Peng <pengdonglin@...omi.com>, Sven Schnelle <svens@...ux.ibm.com>,
Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH v7] function_graph: Enable funcgraph-args and
funcgraph-retaddr to work simultaneously
On Tue, Nov 25, 2025 at 9:12 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Mon, 24 Nov 2025 16:54:40 -0500
> Steven Rostedt <rostedt@...dmis.org> wrote:
>
> > On Mon, 24 Nov 2025 16:47:01 -0500
> > Steven Rostedt <rostedt@...dmis.org> wrote:
> >
> > > > --- a/kernel/trace/trace_entries.h
> > > > +++ b/kernel/trace/trace_entries.h
> > > > @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> > > > TRACE_GRAPH_RETADDR_ENT,
> > > >
> > > > F_STRUCT(
> > > > - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> > > > + __field_struct( struct ftrace_graph_ent, graph_ent )
> > > > __field_packed( unsigned long, graph_ent, func )
> > > > __field_packed( unsigned int, graph_ent, depth )
> > > > - __field_packed( unsigned long, graph_ent, retaddr )
> > >
> > > You can't delete the retaddr without breaking user space.
> > >
> > > Please keep that here.
> >
> > I added this, and user space works again:
> >
> > diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> > index 593a74663c65..4666b6179056 100644
> > --- a/kernel/trace/trace_entries.h
> > +++ b/kernel/trace/trace_entries.h
> > @@ -98,6 +98,7 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> > __field_struct( struct ftrace_graph_ent, graph_ent )
> > __field_packed( unsigned long, graph_ent, func )
> > __field_packed( unsigned int, graph_ent, depth )
> > + __field_packed( unsigned long, graph_ent, retaddr )
> > __dynamic_array(unsigned long, args )
> > ),
> >
>
> Nope, this wasn't enough. I got garbage from this. The following patch
> appears to make it all work though (this is against your patch that was
> forward ported to my for-next branch):
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 88cb54d73bdb..ba180d19423e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1121,12 +1121,10 @@ static inline void ftrace_init(void) { }
>
> /*
> * Structure that defines an entry function trace.
> - * It's already packed but the attribute "packed" is needed
> - * to remove extra padding at the end.
> */
> struct ftrace_graph_ent {
> unsigned long func; /* Current function */
> - int depth;
> + unsigned long depth;
> } __packed;
>
> /*
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 05fb6c9279e3..d509a5041d38 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -60,6 +60,13 @@ enum trace_type {
> __TRACE_LAST_TYPE,
> };
>
> +/*
> + * Structure that defines an entry function trace with retaddr.
> + */
> +struct fgraph_retaddr_ent {
> + struct ftrace_graph_ent ent;
> + unsigned long retaddr; /* Return address */
> +} __packed;
>
> #undef __field
> #define __field(type, item) type item;
> diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> index 593a74663c65..87f4228374cd 100644
> --- a/kernel/trace/trace_entries.h
> +++ b/kernel/trace/trace_entries.h
> @@ -80,11 +80,11 @@ FTRACE_ENTRY(funcgraph_entry, ftrace_graph_ent_entry,
> F_STRUCT(
> __field_struct( struct ftrace_graph_ent, graph_ent )
> __field_packed( unsigned long, graph_ent, func )
> - __field_packed( unsigned int, graph_ent, depth )
> + __field_packed( unsigned long, graph_ent, depth )
> __dynamic_array(unsigned long, args )
> ),
>
> - F_printk("--> %ps (%u)", (void *)__entry->func, __entry->depth)
> + F_printk("--> %ps (%lu)", (void *)__entry->func, __entry->depth)
> );
>
> #ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> @@ -95,13 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> TRACE_GRAPH_RETADDR_ENT,
>
> F_STRUCT(
> - __field_struct( struct ftrace_graph_ent, graph_ent )
> - __field_packed( unsigned long, graph_ent, func )
> - __field_packed( unsigned int, graph_ent, depth )
> + __field_struct( struct fgraph_retaddr_ent, graph_ent )
> + __field_packed( unsigned long, graph_ent.ent, func )
> + __field_packed( unsigned long, graph_ent.ent, depth )
> + __field_packed( unsigned long, graph_ent, retaddr )
> __dynamic_array(unsigned long, args )
> ),
>
> - F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
> + F_printk("--> %ps (%lu) <- %ps", (void *)__entry->func, __entry->depth,
> (void *)__entry->args[0])
Thanks. The args[0] holds the return address in this patch, so if the retaddr
member is added back, we may need to reconstruct this patch to store the
return address in the retaddr member, with the args array storing only function
parameters. I will fix it in the next version.
Thanks
Donglin
> );
>
> -- Steve
Powered by blists - more mailing lists