linux-kernel - Re: [RFC PATCH] function_graph: Support recording and printing the function return address

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240910125033.0a3986fa@gandalf.local.home>
Date: Tue, 10 Sep 2024 12:50:33 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Donglin Peng <dolinux.peng@...il.com>
Cc: mhiramat@...nel.org, mark.rutland@....com,
 mathieu.desnoyers@...icios.com, linux-trace-kernel@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] function_graph: Support recording and printing the
 function return address

On Tue, 10 Sep 2024 14:15:39 +0800
Donglin Peng <dolinux.peng@...il.com> wrote:
> > >
> > > See below logs with both funcgraph-retval and funcgraph-retaddr enabled.
> > >
> > > 4)               |  load_elf_binary() { /* A=bprm_execve+0x249/0x600 */  
> >
> > I wonder if we should make this look more like the function tracer when it
> > shows the parent. That is:
> >
> >   4)               |  load_elf_binary() { /* <-bprm_execve+0x249/0x600 */  
> 
> Nice, Is it necessary to add a prefix letter for the return value? For
> example, when
>  both funcgraph-retval and funcgraph-retaddr are enabled, like this:
> 
>          avc_policy_seqno(); /* <-selinux_file_permission+0x107/0x140 V=0x0 */

How about:

>          avc_policy_seqno(); /* <-selinux_file_permission+0x107/0x140 ret=0x0 */

As "ret" is more obvious than "V".

> 
> >  
> > > 4)               |    load_elf_phdrs() { /* A=load_elf_binary+0x84/0x1730 */
> > > 4)               |      __kmalloc_noprof() { /* A=load_elf_phdrs+0x4a/0xb0 */
> > > 4) + 47.910 us   |        __cond_resched(); /* V=0x0 A=__kmalloc_noprof+0x28c/0x390 */
> > > 4) ! 204.142 us  |      } /* __kmalloc_noprof V=0xffff888201e32c00 */
> > > 4)               |      kernel_read() { /* A=load_elf_phdrs+0x6c/0xb0 */
> > > 4)               |        rw_verify_area() { /* A=kernel_read+0x2b/0x50 */
> > > 4)               |          security_file_permission() {
> > > 4)               |            selinux_file_permission() { /* A=security_file_permission+0x26/0x40 */
> > > 4)               |              __inode_security_revalidate() { /* A=selinux_file_permission+0x6d/0x140 */
> > > 4)   1.182 us    |                __cond_resched(); /* V=0x0 A=__inode_security_revalidate+0x5f/0x80 */
> > > 4)   4.138 us    |              } /* __inode_security_revalidate V=0x0 */
> > > 4)   1.513 us    |              avc_policy_seqno(); /* V=0x0 A=selinux_file_permission+0x107/0x140 */
> > > 4) + 12.133 us   |            } /* selinux_file_permission V=0x0 */
> > > 4) + 39.834 us   |          } /* security_file_permission V=0x0 */
> > > 4) + 42.710 us   |        } /* rw_verify_area V=0x0 */
> > >

> > > +#ifdef CONFIG_FUNCTION_GRAPH_RETVAL
> > >  /* Function call entry */
> > >  FTRACE_ENTRY_PACKED(funcgraph_entry, ftrace_graph_ent_entry,
> > >
> > > @@ -79,6 +80,7 @@ FTRACE_ENTRY_PACKED(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 long,  graph_ent,      retaddr         )
> > >               __field_packed( int,            graph_ent,      depth           )
> > >       ),
> > >  
> >
> > Let's make this a new event, so that when this option is not enabled, we
> > don't waste the ring buffer. For function tracing, every element added to
> > the event will add megabytes extra to the ring buffer.
> >
> > It should be possible to switch what event gets created at the time of the
> > trace. Even calling different functions to do it.  
> 
> Sounds good, we may create a new event as follows:
> 
> struct ftrace_graph_ent {
> unsigned long func; /* Current function */
> unsigned long retaddr;
> int depth;
> } __packed;
> 
> FTRACE_ENTRY_PACKED(funcgraph_retaddr_entry, ftrace_graph_ent_readdr_entry,
> 
> TRACE_GRAPH_RETADDR_ENT,
> 
> F_STRUCT(
> __field_struct( struct ftrace_graph_retaddr_ent, graph_retaddr_ent )
> __field_packed( unsigned long, graph_retaddr_ent, func )
> __field_packed( unsigned long, graph_retaddr_ent, retaddr )
> __field_packed( int, graph_retaddr_ent, depth )
> ),
> 
> F_printk("--> %ps (%d)", (void *)__entry->func, __entry->depth)

Why not add the ret in the printk?

  F_printk("--> %ps (%d) <-%ps", (void *)__entry->func, __entry->depth, (void *)__entry->retaddr)

Then user space parsing tools could use this by default, when it doesn't
know how to parse it.

> );
> 
> If we were to introduce another new feature such as funcgraph-args which
> adds a `ftrace_regs` to the `ftrace_graph_ent`, would it be necessary to
> create yet another new event?

Probably yes.

> 
>  In the scenario where both funcgrah-retaddr and funcgraph-args are enabled,
> it is possible that an additional new event may also be required.

Yes, that would likely be another event.

-- Steve