* [PATCH v2 bpf] ftrace: Fix modify_ftrace_direct.
@ 2021-03-16 19:10 Alexei Starovoitov
2021-03-16 19:45 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Alexei Starovoitov @ 2021-03-16 19:10 UTC (permalink / raw)
To: davem; +Cc: daniel, rostedt, andrii, paulmck, bpf, kernel-team
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(-)
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.
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 bpf] ftrace: Fix modify_ftrace_direct.
2021-03-16 19:10 [PATCH v2 bpf] ftrace: Fix modify_ftrace_direct Alexei Starovoitov
@ 2021-03-16 19:45 ` Steven Rostedt
2021-03-16 19:50 ` Alexei Starovoitov
0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2021-03-16 19:45 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: davem, daniel, andrii, paulmck, bpf, kernel-team
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 bpf] ftrace: Fix modify_ftrace_direct.
2021-03-16 19:45 ` Steven Rostedt
@ 2021-03-16 19:50 ` Alexei Starovoitov
0 siblings, 0 replies; 3+ messages in thread
From: Alexei Starovoitov @ 2021-03-16 19:50 UTC (permalink / raw)
To: Steven Rostedt, Alexei Starovoitov
Cc: davem, daniel, andrii, paulmck, bpf, kernel-team
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-16 19:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-16 19:10 [PATCH v2 bpf] ftrace: Fix modify_ftrace_direct Alexei Starovoitov
2021-03-16 19:45 ` Steven Rostedt
2021-03-16 19:50 ` Alexei Starovoitov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).