[PATCH bpf 0/3] bpf: Make BPF trampoline friendly to ftrace

[PATCH bpf 0/3] bpf: Make BPF trampoline friendly to ftrace

[Alexei Starovoitov]

Patch 1 - fix crash function_graph tracer encounters BPF trampoline
Patch 2 - use register_ftrace_direct() API to attach BPF trampoline
Patch 3 - simple test

Alexei Starovoitov (3):
  ftrace: Fix function_graph tracer interaction with BPF trampoline
  bpf: Make BPF trampoline use register_ftrace_direct() API
  selftests/bpf: test function_graph tracer and bpf trampoline together

 arch/x86/kernel/ftrace.c                   | 14 -----
 include/linux/bpf.h                        |  1 +
 include/linux/ftrace.h                     |  5 ++
 kernel/bpf/trampoline.c                    | 64 ++++++++++++++++++++--
 kernel/trace/fgraph.c                      |  9 +++
 kernel/trace/ftrace.c                      | 19 +++----
 tools/testing/selftests/bpf/test_ftrace.sh | 39 +++++++++++++
 7 files changed, 119 insertions(+), 32 deletions(-)
 create mode 100755 tools/testing/selftests/bpf/test_ftrace.sh

-- 
2.23.0

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

[Alexei Starovoitov]

Depending on type of BPF programs served by BPF trampoline it can call original
function. In such case the trampoline will skip one stack frame while
returning. That will confuse function_graph tracer and will cause crashes with
bad RIP. Teach graph tracer to skip functions that have BPF trampoline attached.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 arch/x86/kernel/ftrace.c | 14 --------------
 include/linux/ftrace.h   |  5 +++++
 kernel/trace/fgraph.c    |  9 +++++++++
 kernel/trace/ftrace.c    | 19 +++++++------------
 4 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 060a361d9d11..024c3053dbba 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -1042,20 +1042,6 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;
 
-	/*
-	 * If the return location is actually pointing directly to
-	 * the start of a direct trampoline (if we trace the trampoline
-	 * it will still be offset by MCOUNT_INSN_SIZE), then the
-	 * return address is actually off by one word, and we
-	 * need to adjust for that.
-	 */
-	if (ftrace_direct_func_count) {
-		if (ftrace_find_direct_func(self_addr + MCOUNT_INSN_SIZE)) {
-			self_addr = *parent;
-			parent++;
-		}
-	}
-
 	/*
 	 * Protect against fault, even if it shouldn't
 	 * happen. This tool is too much intrusive to
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7247d35c3d16..db95244a62d4 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -264,6 +264,7 @@ int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
 				struct dyn_ftrace *rec,
 				unsigned long old_addr,
 				unsigned long new_addr);
+unsigned long ftrace_find_rec_direct(unsigned long ip);
 #else
 # define ftrace_direct_func_count 0
 static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
@@ -290,6 +291,10 @@ static inline int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
 {
 	return -ENODEV;
 }
+static inline unsigned long ftrace_find_rec_direct(unsigned long ip)
+{
+	return 0;
+}
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 #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))
+		return -EBUSY;
 	trace.func = func;
 	trace.depth = ++current->curr_ret_depth;
 
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 74439ab5c2b6..ac99a3500076 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2364,7 +2364,7 @@ int ftrace_direct_func_count;
  * Search the direct_functions hash to see if the given instruction pointer
  * has a direct caller attached to it.
  */
-static unsigned long find_rec_direct(unsigned long ip)
+unsigned long ftrace_find_rec_direct(unsigned long ip)
 {
 	struct ftrace_func_entry *entry;
 
@@ -2380,7 +2380,7 @@ static void call_direct_funcs(unsigned long ip, unsigned long pip,
 {
 	unsigned long addr;
 
-	addr = find_rec_direct(ip);
+	addr = ftrace_find_rec_direct(ip);
 	if (!addr)
 		return;
 
@@ -2393,11 +2393,6 @@ struct ftrace_ops direct_ops = {
 			  | FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS
 			  | FTRACE_OPS_FL_PERMANENT,
 };
-#else
-static inline unsigned long find_rec_direct(unsigned long ip)
-{
-	return 0;
-}
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 /**
@@ -2417,7 +2412,7 @@ unsigned long ftrace_get_addr_new(struct dyn_ftrace *rec)
 
 	if ((rec->flags & FTRACE_FL_DIRECT) &&
 	    (ftrace_rec_count(rec) == 1)) {
-		addr = find_rec_direct(rec->ip);
+		addr = ftrace_find_rec_direct(rec->ip);
 		if (addr)
 			return addr;
 		WARN_ON_ONCE(1);
@@ -2458,7 +2453,7 @@ unsigned long ftrace_get_addr_curr(struct dyn_ftrace *rec)
 
 	/* Direct calls take precedence over trampolines */
 	if (rec->flags & FTRACE_FL_DIRECT_EN) {
-		addr = find_rec_direct(rec->ip);
+		addr = ftrace_find_rec_direct(rec->ip);
 		if (addr)
 			return addr;
 		WARN_ON_ONCE(1);
@@ -3604,7 +3599,7 @@ static int t_show(struct seq_file *m, void *v)
 		if (rec->flags & FTRACE_FL_DIRECT) {
 			unsigned long direct;
 
-			direct = find_rec_direct(rec->ip);
+			direct = ftrace_find_rec_direct(rec->ip);
 			if (direct)
 				seq_printf(m, "\n\tdirect-->%pS", (void *)direct);
 		}
@@ -5008,7 +5003,7 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 	mutex_lock(&direct_mutex);
 
 	/* See if there's a direct function at @ip already */
-	if (find_rec_direct(ip))
+	if (ftrace_find_rec_direct(ip))
 		goto out_unlock;
 
 	ret = -ENODEV;
@@ -5027,7 +5022,7 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 	if (ip != rec->ip) {
 		ip = rec->ip;
 		/* Need to check this ip for a direct. */
-		if (find_rec_direct(ip))
+		if (ftrace_find_rec_direct(ip))
 			goto out_unlock;
 	}
 
-- 
2.23.0

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

[Alexei Starovoitov]

On Sun, Dec 8, 2019 at 4:03 PM Alexei Starovoitov <ast@kernel.org> wrote:
>
> Depending on type of BPF programs served by BPF trampoline it can call original
> function. In such case the trampoline will skip one stack frame while
> returning. That will confuse function_graph tracer and will cause crashes with
> bad RIP. Teach graph tracer to skip functions that have BPF trampoline attached.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Steven, please take a look.

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

[Steven Rostedt]

On Tue, 10 Dec 2019 08:19:42 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Sun, Dec 8, 2019 at 4:03 PM Alexei Starovoitov <ast@kernel.org> wrote:
> >
> > Depending on type of BPF programs served by BPF trampoline it can call original
> > function. In such case the trampoline will skip one stack frame while
> > returning. That will confuse function_graph tracer and will cause crashes with
> > bad RIP. Teach graph tracer to skip functions that have BPF trampoline attached.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>  
> 
> Steven, please take a look.

I'll try to get to it today or tomorrow. I have some other work to get
done that my job requires I do ;-)

-- Steve

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

[Steven Rostedt]

On Sun, 8 Dec 2019 16:01:12 -0800
Alexei Starovoitov <ast@kernel.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.

> +		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?

-- Steve

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

[Alexei Starovoitov]

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@kernel.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 :)

[PATCH bpf 2/3] bpf: Make BPF trampoline use register_ftrace_direct() API

[Alexei Starovoitov]

Make BPF trampoline attach its generated assembly code to kernel functions via
register_ftrace_direct() API. It helps ftrace-based tracers co-exist with BPF
trampoline on the same kernel function. It also switches attaching logic from
arch specific text_poke to generic ftrace that is available on many
architectures. text_poke is still necessary for bpf-to-bpf attach and for
bpf_tail_call optimization.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h     |  1 +
 kernel/bpf/trampoline.c | 64 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 35903f148be5..ac7de5291509 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -461,6 +461,7 @@ struct bpf_trampoline {
 	struct {
 		struct btf_func_model model;
 		void *addr;
+		bool ftrace_managed;
 	} func;
 	/* list of BPF programs using this trampoline */
 	struct hlist_head progs_hlist[BPF_TRAMP_MAX];
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 7e89f1f49d77..23b0d5cfd47e 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -3,6 +3,7 @@
 #include <linux/hash.h>
 #include <linux/bpf.h>
 #include <linux/filter.h>
+#include <linux/ftrace.h>
 
 /* btf_vmlinux has ~22k attachable functions. 1k htab is enough. */
 #define TRAMPOLINE_HASH_BITS 10
@@ -59,6 +60,60 @@ struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
 	return tr;
 }
 
+static int is_ftrace_location(void *ip)
+{
+	long addr;
+
+	addr = ftrace_location((long)ip);
+	if (!addr)
+		return 0;
+	if (WARN_ON_ONCE(addr != (long)ip))
+		return -EFAULT;
+	return 1;
+}
+
+static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
+{
+	void *ip = tr->func.addr;
+	int ret;
+
+	if (tr->func.ftrace_managed)
+		ret = unregister_ftrace_direct((long)ip, (long)old_addr);
+	else
+		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, NULL);
+	return ret;
+}
+
+static int modify_fentry(struct bpf_trampoline *tr, void *old_addr, void *new_addr)
+{
+	void *ip = tr->func.addr;
+	int ret;
+
+	if (tr->func.ftrace_managed)
+		ret = modify_ftrace_direct((long)ip, (long)old_addr, (long)new_addr);
+	else
+		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, new_addr);
+	return ret;
+}
+
+/* first time registering */
+static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
+{
+	void *ip = tr->func.addr;
+	int ret;
+
+	ret = is_ftrace_location(ip);
+	if (ret < 0)
+		return ret;
+	tr->func.ftrace_managed = ret;
+
+	if (tr->func.ftrace_managed)
+		ret = register_ftrace_direct((long)ip, (long)new_addr);
+	else
+		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, NULL, new_addr);
+	return ret;
+}
+
 /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
  * bytes on x86.  Pick a number to fit into PAGE_SIZE / 2
  */
@@ -77,8 +132,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	int err;
 
 	if (fentry_cnt + fexit_cnt == 0) {
-		err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_CALL,
-					 old_image, NULL);
+		err = unregister_fentry(tr, old_image);
 		tr->selector = 0;
 		goto out;
 	}
@@ -105,12 +159,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
 
 	if (tr->selector)
 		/* progs already running at this address */
-		err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_CALL,
-					 old_image, new_image);
+		err = modify_fentry(tr, old_image, new_image);
 	else
 		/* first time registering */
-		err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_CALL, NULL,
-					 new_image);
+		err = register_fentry(tr, new_image);
 	if (err)
 		goto out;
 	tr->selector++;
-- 
2.23.0

[PATCH bpf 3/3] selftests/bpf: test function_graph tracer and bpf trampoline together

[Alexei Starovoitov]

Add simple test script to execute funciton graph tracer while BPF trampoline
attaches and detaches from the functions being graph traced.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/testing/selftests/bpf/test_ftrace.sh | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100755 tools/testing/selftests/bpf/test_ftrace.sh

diff --git a/tools/testing/selftests/bpf/test_ftrace.sh b/tools/testing/selftests/bpf/test_ftrace.sh
new file mode 100755
index 000000000000..20de7bb873bc
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_ftrace.sh
@@ -0,0 +1,39 @@
+#!/bin/bash
+
+TR=/sys/kernel/debug/tracing/
+clear_trace() { # reset trace output
+    echo > $TR/trace
+}
+
+disable_tracing() { # stop trace recording
+    echo 0 > $TR/tracing_on
+}
+
+enable_tracing() { # start trace recording
+    echo 1 > $TR/tracing_on
+}
+
+reset_tracer() { # reset the current tracer
+    echo nop > $TR/current_tracer
+}
+
+disable_tracing
+clear_trace
+
+echo "" > $TR/set_ftrace_filter
+echo '*printk* *console* *wake* *serial* *lock*' > $TR/set_ftrace_notrace
+
+echo "bpf_prog_test*" > $TR/set_graph_function
+echo "" > $TR/set_graph_notrace
+
+echo function_graph > $TR/current_tracer
+
+enable_tracing
+./test_progs -t fentry
+./test_progs -t fexit
+disable_tracing
+clear_trace
+
+reset_tracer
+
+exit 0
-- 
2.23.0

Re: [PATCH bpf 0/3] bpf: Make BPF trampoline friendly to ftrace

[Martin Lau]

On Sun, Dec 08, 2019 at 04:01:11PM -0800, Alexei Starovoitov wrote:
> Patch 1 - fix crash function_graph tracer encounters BPF trampoline
> Patch 2 - use register_ftrace_direct() API to attach BPF trampoline
> Patch 3 - simple test
> 
> Alexei Starovoitov (3):
>   ftrace: Fix function_graph tracer interaction with BPF trampoline
>   bpf: Make BPF trampoline use register_ftrace_direct() API
>   selftests/bpf: test function_graph tracer and bpf trampoline together
Acked-by: Martin KaFai Lau <kafai@fb.com>