| Message ID | 20210312224237.75061-1-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] ftrace: Fix modify_ftrace_direct. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 8 maintainers not CCed: netdev@vger.kernel.org yhs@fb.com kpsingh@kernel.org mingo@redhat.com kafai@fb.com ast@kernel.org john.fastabend@gmail.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 96 this patch: 96 |
| netdev/kdoc | success | Errors and warnings before: 31 this patch: 31 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 18 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 96 this patch: 96 |
| netdev/header_inline | success | Link |
On Fri, 12 Mar 2021 14:42:37 -0800 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > From: Alexei Starovoitov <ast@kernel.org> > > The following sequence of commands: > register_ftrace_direct(ip, addr1); > modify_ftrace_direct(ip, addr1, addr2); > unregister_ftrace_direct(ip, addr2); > will cause the kernel to warn: > [ 30.179191] WARNING: CPU: 2 PID: 1961 at kernel/trace/ftrace.c:5223 unregister_ftrace_direct+0x130/0x150 > [ 30.180556] CPU: 2 PID: 1961 Comm: test_progs W O 5.12.0-rc2-00378-g86bc10a0a711-dirty #3246 > [ 30.182453] RIP: 0010:unregister_ftrace_direct+0x130/0x150 > > When modify_ftrace_direct() changes the addr from old to new it should update > the addr stored in ftrace_direct_funcs. Otherwise the final > unregister_ftrace_direct() won't find the address and will cause the splat. > > Fixes: 0567d6809182 ("ftrace: Add modify_ftrace_direct()") > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- > Steven, > > I was fixing bpf trampoline and realized that modify_ftrace_direct() was > broken from the beginning. bpf trampoline was lucky that it was > reusing the same page and the final unregister_ftrace_direct() always > happened with original addr. > Pls ack if the fix looks good to you. > I'd like to cary this patch through bpf tree with the other fixes > I'm working on. > > Thanks! > > kernel/trace/ftrace.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 4d8e35575549..510e1c1050a1 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -5329,6 +5329,7 @@ int __weak ftrace_modify_direct_caller(struct ftrace_func_entry *entry, > int modify_ftrace_direct(unsigned long ip, > unsigned long old_addr, unsigned long new_addr) > { > + struct ftrace_direct_func *direct; > struct ftrace_func_entry *entry; > struct dyn_ftrace *rec; > int ret = -ENODEV; > @@ -5344,6 +5345,11 @@ int modify_ftrace_direct(unsigned long ip, > if (entry->direct != old_addr) > goto out_unlock; > > + direct = ftrace_find_direct_func(old_addr); > + if (WARN_ON(!direct)) > + goto out_unlock; > + direct->addr = new_addr; > + I think this needs a bit more, as a ftrace_direct_func could be called by more than one function, which is why the ftrace_direct_func has a ref count. You'll need something like: if (direct->count > 1) { ret = -ENOMEM; new_direct = kmalloc(sizeof(*direct), GFP_KERNEL); if (!new_direct) goto out_unlock; direct->count--; new_direct->count = 1; list_add_rcu(&new_direct->next, &ftrace_direct_funcs); ftrace_direct_func_count++; direct = new_direct; } direct->addr = new_addr; A helper function should probably be made to add a new direct instead of just copying the code, but you get the idea. -- Steve > /* > * If there's no other ftrace callback on the rec->ip location, > * then it can be changed directly by the architecture.
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 4d8e35575549..510e1c1050a1 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -5329,6 +5329,7 @@ int __weak ftrace_modify_direct_caller(struct ftrace_func_entry *entry, int modify_ftrace_direct(unsigned long ip, unsigned long old_addr, unsigned long new_addr) { + struct ftrace_direct_func *direct; struct ftrace_func_entry *entry; struct dyn_ftrace *rec; int ret = -ENODEV; @@ -5344,6 +5345,11 @@ int modify_ftrace_direct(unsigned long ip, if (entry->direct != old_addr) goto out_unlock; + direct = ftrace_find_direct_func(old_addr); + if (WARN_ON(!direct)) + goto out_unlock; + direct->addr = new_addr; + /* * If there's no other ftrace callback on the rec->ip location, * then it can be changed directly by the architecture.