[PATCH bpf] ftrace: Fix modify_ftrace_direct.

[PATCH bpf] ftrace: Fix modify_ftrace_direct.

[Alexei Starovoitov]

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;
+
 	/*
 	 * If there's no other ftrace callback on the rec->ip location,
 	 * then it can be changed directly by the architecture.
-- 
2.24.1

Re: [PATCH bpf] ftrace: Fix modify_ftrace_direct.

[Steven Rostedt]

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.