| Message ID | 20210316195815.34714-1-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | [v3,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 | fail | Errors and warnings before: 96 this patch: 98 |
| 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, 77 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 96 this patch: 98 |
| netdev/header_inline | success | Link |
On Tue, 16 Mar 2021 12:58:15 -0700 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> > --- > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve
On 3/16/21 9:16 PM, Steven Rostedt wrote: > On Tue, 16 Mar 2021 12:58:15 -0700 > 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> > > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Applied, thanks!
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 4d8e35575549..40dd24db1708 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -5045,6 +5045,20 @@ struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr) return NULL; } +struct ftrace_direct_func *ftrace_alloc_direct_func(unsigned long addr) +{ + struct ftrace_direct_func *direct; + + direct = kmalloc(sizeof(*direct), GFP_KERNEL); + if (!direct) + return NULL; + direct->addr = addr; + direct->count = 0; + list_add_rcu(&direct->next, &ftrace_direct_funcs); + ftrace_direct_func_count++; + return direct; +} + /** * register_ftrace_direct - Call a custom trampoline directly * @ip: The address of the nop at the beginning of a function @@ -5120,15 +5134,11 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr) direct = ftrace_find_direct_func(addr); if (!direct) { - direct = kmalloc(sizeof(*direct), GFP_KERNEL); + direct = ftrace_alloc_direct_func(addr); if (!direct) { kfree(entry); goto out_unlock; } - direct->addr = addr; - direct->count = 0; - list_add_rcu(&direct->next, &ftrace_direct_funcs); - ftrace_direct_func_count++; } entry->ip = ip; @@ -5329,6 +5339,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, *new_direct = NULL; struct ftrace_func_entry *entry; struct dyn_ftrace *rec; int ret = -ENODEV; @@ -5344,6 +5355,20 @@ 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; + if (direct->count > 1) { + ret = -ENOMEM; + new_direct = ftrace_alloc_direct_func(new_addr); + if (!new_direct) + goto out_unlock; + direct->count--; + new_direct->count++; + } else { + 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. @@ -5357,6 +5382,14 @@ int modify_ftrace_direct(unsigned long ip, ret = 0; } + if (unlikely(ret && new_direct)) { + direct->count++; + list_del_rcu(&new_direct->next); + synchronize_rcu_tasks(); + kfree(new_direct); + ftrace_direct_func_count--; + } + out_unlock: mutex_unlock(&ftrace_lock); mutex_unlock(&direct_mutex);