Re: [PATCH bpf] bpf: Fix fexit trampoline.

[Jiri Olsa <jolsa@redhat.com>]

On Mon, Mar 22, 2021 at 05:24:26PM +0100, Jiri Olsa wrote:
> On Mon, Mar 22, 2021 at 12:32:05AM +0100, Jiri Olsa wrote:
> > On Tue, Mar 16, 2021 at 02:00:07PM -0700, Alexei Starovoitov wrote:
> > > From: Alexei Starovoitov <ast@kernel.org>
> > > 
> > > The fexit/fmod_ret programs can be attached to kernel functions that can sleep.
> > > The synchronize_rcu_tasks() will not wait for such tasks to complete.
> > > In such case the trampoline image will be freed and when the task
> > > wakes up the return IP will point to freed memory causing the crash.
> > > Solve this by adding percpu_ref_get/put for the duration of trampoline
> > > and separate trampoline vs its image life times.
> > > The "half page" optimization has to be removed, since
> > > first_half->second_half->first_half transition cannot be guaranteed to
> > > complete in deterministic time. Every trampoline update becomes a new image.
> > > The image with fmod_ret or fexit progs will be freed via percpu_ref_kill and
> > > call_rcu_tasks. Together they will wait for the original function and
> > > trampoline asm to complete. The trampoline is patched from nop to jmp to skip
> > > fexit progs. They are freed independently from the trampoline. The image with
> > > fentry progs only will be freed via call_rcu_tasks_trace+call_rcu_tasks which
> > > will wait for both sleepable and non-sleepable progs to complete.
> > > 
> > > Reported-by: Andrii Nakryiko <andrii@kernel.org>
> > > Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline")
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > Acked-by: Paul E. McKenney <paulmck@kernel.org>  # for RCU
> > > ---
> > > Without ftrace fix:
> > > https://patchwork.kernel.org/project/netdevbpf/patch/20210316195815.34714-1-alexei.starovoitov@gmail.com/
> > > this patch will trigger warn in ftrace.
> > > 
> > >  arch/x86/net/bpf_jit_comp.c |  26 ++++-
> > >  include/linux/bpf.h         |  24 +++-
> > >  kernel/bpf/bpf_struct_ops.c |   2 +-
> > >  kernel/bpf/core.c           |   4 +-
> > >  kernel/bpf/trampoline.c     | 218 +++++++++++++++++++++++++++---------
> > >  5 files changed, 213 insertions(+), 61 deletions(-)
> > > 
> > 
> > hi,
> > I'm on bpf/master and I'm triggering warnings below when running together:
> > 
> >   # while :; do ./test_progs -t fentry_test ; done
> >   # while :; do ./test_progs -t module_attach ; done
> 
> hum, is it possible that we don't take module ref and it can get
> unloaded even if there's trampoline attach to it..? I can't see
> that in the code.. ftrace_release_mod can't fail ;-)

when I get the module for each module trampoline,
I can no longer see those warnings (link for Steven):
  https://lore.kernel.org/bpf/YFfXcqnksPsSe0Bv@krava/

Steven,
I might be missing something, but it looks like module
can be unloaded even if the trampoline (direct function)
is registered in it.. is that right?

thanks,
jirka


---
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index b7e29db127fa..ab0b2c8df283 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5059,6 +5059,28 @@ static struct ftrace_direct_func *ftrace_alloc_direct_func(unsigned long addr)
 	return direct;
 }
 
+static struct module *ftrace_direct_module_get(unsigned long ip)
+{
+	struct module *mod;
+	int err = 0;
+
+	preempt_disable();
+	mod = __module_text_address(ip);
+	if (mod && !try_module_get(mod))
+		err = -ENOENT;
+	preempt_enable();
+	return err ? ERR_PTR(err) : mod;
+}
+
+static void ftrace_direct_module_put(unsigned long ip)
+{
+	struct module *mod;
+
+	mod = __module_text_address(ip);
+	if (mod)
+		module_put(mod);
+}
+
 /**
  * register_ftrace_direct - Call a custom trampoline directly
  * @ip: The address of the nop at the beginning of a function
@@ -5081,6 +5103,7 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 	struct ftrace_direct_func *direct;
 	struct ftrace_func_entry *entry;
 	struct ftrace_hash *free_hash = NULL;
+	struct module *mod = NULL;
 	struct dyn_ftrace *rec;
 	int ret = -EBUSY;
 
@@ -5095,6 +5118,13 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 	if (!rec)
 		goto out_unlock;
 
+	mod = ftrace_direct_module_get(ip);
+	if (IS_ERR(mod)) {
+		ret = -ENOENT;
+		mod = NULL;
+		goto out_unlock;
+	}
+
 	/*
 	 * Check if the rec says it has a direct call but we didn't
 	 * find one earlier?
@@ -5172,6 +5202,8 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
  out_unlock:
 	mutex_unlock(&direct_mutex);
 
+	if (ret)
+		module_put(mod);
 	if (free_hash) {
 		synchronize_rcu_tasks();
 		free_ftrace_hash(free_hash);
@@ -5242,6 +5274,8 @@ int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
 			ftrace_direct_func_count--;
 		}
 	}
+	ftrace_direct_module_put(ip);
+
  out_unlock:
 	mutex_unlock(&direct_mutex);