On Fri, 12 Mar 2021 14:42:37 -0800
Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> From: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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.
