diff mbox series

[v2,bpf] ftrace: Fix modify_ftrace_direct.

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

Checks

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

Commit Message

Alexei Starovoitov March 16, 2021, 7:10 p.m. UTC
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.

 kernel/trace/ftrace.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

Comments

Steven Rostedt March 16, 2021, 7:45 p.m. UTC | #1
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
Alexei Starovoitov March 16, 2021, 7:50 p.m. UTC | #2
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 mbox series

Patch

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.