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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline")
> > > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > > Acked-by: Paul E. McKenney <paulmck@xxxxxxxxxx> # for RCU
> > > ---
> > > Without ftrace fix:
> > > https://patchwork.kernel.org/project/netdevbpf/patch/20210316195815.34714-1-alexei.starovoitov@xxxxxxxxx/
> > > 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);
