| Message ID | 20210316191046.28002-1-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2,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, 63 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:10:46 -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> > --- > Steven, > I think I've changed it the way you requested. Please ack if so. The changes look fine, but I just found another issue that needs to be handled as well. > @@ -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; > 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. Everything looks good above, but then looking below this code we have: if (ftrace_rec_count(rec) == 1) { ret = ftrace_modify_direct_caller(entry, rec, old_addr, new_addr); } else { entry->direct = new_addr; ret = 0; } Where if ftrace_modify_direct_caller() fails, you need to put back the direct descriptors to where they were. struct ftrace_direct_func *new_direct = NULL; [..] if (unlikely(ret && new_direct)) { direct->count++; list_del_rcu(&new_direct->next); synchronize_rcu_tasks(); kfree(new_direct); } The above is highly unlikely to happen, but it could. -- Steve
On 3/16/21 12:45 PM, Steven Rostedt wrote: > On Tue, 16 Mar 2021 12:10:46 -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> >> --- >> Steven, >> I think I've changed it the way you requested. Please ack if so. > > The changes look fine, but I just found another issue that needs to be > handled as well. > > >> @@ -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; >> 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. > > Everything looks good above, but then looking below this code we have: > > if (ftrace_rec_count(rec) == 1) { > ret = ftrace_modify_direct_caller(entry, rec, old_addr, new_addr); > } else { > entry->direct = new_addr; > ret = 0; > } > > Where if ftrace_modify_direct_caller() fails, you need to put back the > direct descriptors to where they were. > > struct ftrace_direct_func *new_direct = NULL; > > [..] > > if (unlikely(ret && new_direct)) { > direct->count++; > list_del_rcu(&new_direct->next); > synchronize_rcu_tasks(); > kfree(new_direct); > } > > The above is highly unlikely to happen, but it could. Sure. Will respin.
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 4d8e35575549..1f94a100e587 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; 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.