netdev - Re: [PATCH bpf 1/3] ftrace: Fix function_graph tracer interaction with BPF trampoline

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20191210234931.mfaklfs7s4i4fsxo@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 10 Dec 2019 15:49:33 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Alexei Starovoitov <ast@...nel.org>, davem@...emloft.net,
        daniel@...earbox.net, x86@...nel.org, netdev@...r.kernel.org,
        bpf@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH bpf 1/3] ftrace: Fix function_graph tracer interaction
 with BPF trampoline

On Tue, Dec 10, 2019 at 06:35:19PM -0500, Steven Rostedt wrote:
> On Sun, 8 Dec 2019 16:01:12 -0800
> Alexei Starovoitov <ast@...nel.org> wrote:
> 
> >  #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> > index 67e0c462b059..a2659735db73 100644
> > --- a/kernel/trace/fgraph.c
> > +++ b/kernel/trace/fgraph.c
> > @@ -101,6 +101,15 @@ int function_graph_enter(unsigned long ret, unsigned long func,
> >  {
> >  	struct ftrace_graph_ent trace;
> >  
> > +	/*
> > +	 * Skip graph tracing if the return location is served by direct trampoline,
> > +	 * since call sequence and return addresses is unpredicatable anymore.
> > +	 * Ex: BPF trampoline may call original function and may skip frame
> > +	 * depending on type of BPF programs attached.
> > +	 */
> > +	if (ftrace_direct_func_count &&
> > +	    ftrace_find_rec_direct(ret - MCOUNT_INSN_SIZE))
> 
> My only worry is that this may not work for all archs that implement
> it. But I figure we can cross that bridge when we get to it.

Right. Since bpf trampoline is going to be the only user in short term
it's not an issue, since trampoline is x86-64 only so far.

> > +		return -EBUSY;
> >  	trace.func = func;
> >  	trace.depth = ++current->curr_ret_depth;
> >  
> 
> I added this patch to my queue and it's about 70% done going through my
> test suite (takes around 10 - 13 hours).
> 
> As I'm about to send a pull request to Linus tomorrow, I could include
> this patch (as it will be fully tested), and then you could apply the
> other two when it hits Linus's tree.
> 
> Would that work for you?

Awesome. Much appreciate additional testing. I can certainly wait another day.
I was hoping to get patch 2 all the way to Linus's tree before rc2 to make sure
register_ftrace_direct() API is used for real in this kernel cycle. When
everything will land I'll backport to our production kernel and then the actual
stress testing begins :)