We see the following GPF when register_ftrace_direct fails:
[ ] general protection fault, probably for non-canonical address \
0x200000000000010: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC PTI
[...]
[ ] RIP: 0010:ftrace_find_rec_direct+0x53/0x70
[ ] Code: 48 c1 e0 03 48 03 42 08 48 8b 10 31 c0 48 85 d2 74 [...]
[ ] RSP: 0018:ffffc9000138bc10 EFLAGS: 00010206
[ ] RAX: 0000000000000000 RBX: ffffffff813e0df0 RCX: 000000000000003b
[ ] RDX: 0200000000000000 RSI: 000000000000000c RDI: ffffffff813e0df0
[ ] RBP: ffffffffa00a3000 R08: ffffffff81180ce0 R09: 0000000000000001
[ ] R10: ffffc9000138bc18 R11: 0000000000000001 R12: ffffffff813e0df0
[ ] R13: ffffffff813e0df0 R14: ffff888171b56400 R15: 0000000000000000
[ ] FS: 00007fa9420c7780(0000) GS:ffff888ff6a00000(0000) knlGS:000000000
[ ] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ ] CR2: 000000000770d000 CR3: 0000000107d50003 CR4: 0000000000370ee0
[ ] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ ] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ ] Call Trace:
[ ] <TASK>
[ ] register_ftrace_direct+0x54/0x290
[ ] ? render_sigset_t+0xa0/0xa0
[ ] bpf_trampoline_update+0x3f5/0x4a0
[ ] ? 0xffffffffa00a3000
[ ] bpf_trampoline_link_prog+0xa9/0x140
[ ] bpf_tracing_prog_attach+0x1dc/0x450
[ ] bpf_raw_tracepoint_open+0x9a/0x1e0
[ ] ? find_held_lock+0x2d/0x90
[ ] ? lock_release+0x150/0x430
[ ] __sys_bpf+0xbd6/0x2700
[ ] ? lock_is_held_type+0xd8/0x130
[ ] __x64_sys_bpf+0x1c/0x20
[ ] do_syscall_64+0x3a/0x80
[ ] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ ] RIP: 0033:0x7fa9421defa9
[ ] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 9 f8 [...]
[ ] RSP: 002b:00007ffed743bd78 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
[ ] RAX: ffffffffffffffda RBX: 00000000069d2480 RCX: 00007fa9421defa9
[ ] RDX: 0000000000000078 RSI: 00007ffed743bd80 RDI: 0000000000000011
[ ] RBP: 00007ffed743be00 R08: 0000000000bb7270 R09: 0000000000000000
[ ] R10: 00000000069da210 R11: 0000000000000246 R12: 0000000000000001
[ ] R13: 00007ffed743c4b0 R14: 00000000069d2480 R15: 0000000000000001
[ ] </TASK>
[ ] Modules linked in: klp_vm(OK)
[ ] ---[ end trace 0000000000000000 ]---
One way to trigger this is:
1. load a livepatch that patches kernel function xxx;
2. run bpftrace -e 'kfunc:xxx {}', this will fail (expected for now);
3. repeat #2 => gpf.
This is because the entry is added to direct_functions, but not removed.
Fix this by remove the entry from direct_functions when
register_ftrace_direct fails.
Also remove the last trailing space from ftrace.c, so we don't have to
worry about it anymore.
Cc: [email protected]
Fixes: 763e34e74bb7 ("ftrace: Add register_ftrace_direct()")
Signed-off-by: Song Liu <[email protected]>
---
kernel/trace/ftrace.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 4f1d2f5e7263..375293c5f3e9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4465,7 +4465,7 @@ int ftrace_func_mapper_add_ip(struct ftrace_func_mapper *mapper,
* @ip: The instruction pointer address to remove the data from
*
* Returns the data if it is found, otherwise NULL.
- * Note, if the data pointer is used as the data itself, (see
+ * Note, if the data pointer is used as the data itself, (see
* ftrace_func_mapper_find_ip(), then the return value may be meaningless,
* if the data pointer was set to zero.
*/
@@ -5200,8 +5200,10 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
if (!ret && !(direct_ops.flags & FTRACE_OPS_FL_ENABLED)) {
ret = register_ftrace_function(&direct_ops);
- if (ret)
+ if (ret) {
ftrace_set_filter_ip(&direct_ops, ip, 1, 0);
+ remove_hash_entry(direct_functions, entry);
+ }
}
if (ret) {
--
2.30.2
On Thu, May 12, 2022 at 3:08 PM Song Liu <[email protected]> wrote:
>
> We see the following GPF when register_ftrace_direct fails:
>
> [ ] general protection fault, probably for non-canonical address \
> 0x200000000000010: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC PTI
> [...]
> [ ] RIP: 0010:ftrace_find_rec_direct+0x53/0x70
> [ ] Code: 48 c1 e0 03 48 03 42 08 48 8b 10 31 c0 48 85 d2 74 [...]
> [ ] RSP: 0018:ffffc9000138bc10 EFLAGS: 00010206
> [ ] RAX: 0000000000000000 RBX: ffffffff813e0df0 RCX: 000000000000003b
> [ ] RDX: 0200000000000000 RSI: 000000000000000c RDI: ffffffff813e0df0
> [ ] RBP: ffffffffa00a3000 R08: ffffffff81180ce0 R09: 0000000000000001
> [ ] R10: ffffc9000138bc18 R11: 0000000000000001 R12: ffffffff813e0df0
> [ ] R13: ffffffff813e0df0 R14: ffff888171b56400 R15: 0000000000000000
> [ ] FS: 00007fa9420c7780(0000) GS:ffff888ff6a00000(0000) knlGS:000000000
> [ ] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ ] CR2: 000000000770d000 CR3: 0000000107d50003 CR4: 0000000000370ee0
> [ ] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ ] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ ] Call Trace:
> [ ] <TASK>
> [ ] register_ftrace_direct+0x54/0x290
> [ ] ? render_sigset_t+0xa0/0xa0
> [ ] bpf_trampoline_update+0x3f5/0x4a0
> [ ] ? 0xffffffffa00a3000
> [ ] bpf_trampoline_link_prog+0xa9/0x140
> [ ] bpf_tracing_prog_attach+0x1dc/0x450
> [ ] bpf_raw_tracepoint_open+0x9a/0x1e0
> [ ] ? find_held_lock+0x2d/0x90
> [ ] ? lock_release+0x150/0x430
> [ ] __sys_bpf+0xbd6/0x2700
> [ ] ? lock_is_held_type+0xd8/0x130
> [ ] __x64_sys_bpf+0x1c/0x20
> [ ] do_syscall_64+0x3a/0x80
> [ ] entry_SYSCALL_64_after_hwframe+0x44/0xae
> [ ] RIP: 0033:0x7fa9421defa9
> [ ] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 9 f8 [...]
> [ ] RSP: 002b:00007ffed743bd78 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
> [ ] RAX: ffffffffffffffda RBX: 00000000069d2480 RCX: 00007fa9421defa9
> [ ] RDX: 0000000000000078 RSI: 00007ffed743bd80 RDI: 0000000000000011
> [ ] RBP: 00007ffed743be00 R08: 0000000000bb7270 R09: 0000000000000000
> [ ] R10: 00000000069da210 R11: 0000000000000246 R12: 0000000000000001
> [ ] R13: 00007ffed743c4b0 R14: 00000000069d2480 R15: 0000000000000001
> [ ] </TASK>
> [ ] Modules linked in: klp_vm(OK)
> [ ] ---[ end trace 0000000000000000 ]---
>
> One way to trigger this is:
> 1. load a livepatch that patches kernel function xxx;
> 2. run bpftrace -e 'kfunc:xxx {}', this will fail (expected for now);
> 3. repeat #2 => gpf.
>
> This is because the entry is added to direct_functions, but not removed.
> Fix this by remove the entry from direct_functions when
> register_ftrace_direct fails.
>
> Also remove the last trailing space from ftrace.c, so we don't have to
> worry about it anymore.
>
> Cc: [email protected]
> Fixes: 763e34e74bb7 ("ftrace: Add register_ftrace_direct()")
> Signed-off-by: Song Liu <[email protected]>
Hi Steven,
Could you please share your thoughts on this fix?
Thanks,
Song
> ---
> kernel/trace/ftrace.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 4f1d2f5e7263..375293c5f3e9 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -4465,7 +4465,7 @@ int ftrace_func_mapper_add_ip(struct ftrace_func_mapper *mapper,
> * @ip: The instruction pointer address to remove the data from
> *
> * Returns the data if it is found, otherwise NULL.
> - * Note, if the data pointer is used as the data itself, (see
> + * Note, if the data pointer is used as the data itself, (see
> * ftrace_func_mapper_find_ip(), then the return value may be meaningless,
> * if the data pointer was set to zero.
> */
> @@ -5200,8 +5200,10 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
>
> if (!ret && !(direct_ops.flags & FTRACE_OPS_FL_ENABLED)) {
> ret = register_ftrace_function(&direct_ops);
> - if (ret)
> + if (ret) {
> ftrace_set_filter_ip(&direct_ops, ip, 1, 0);
> + remove_hash_entry(direct_functions, entry);
> + }
> }
>
> if (ret) {
> --
> 2.30.2
>
On Mon, 23 May 2022 11:57:22 -0700
Song Liu <[email protected]> wrote:
> Hi Steven,
>
> Could you please share your thoughts on this fix?
I'll take a look at it tomorrow, when I look at all my past patches.
Sorry for the delay, I've been behind schedule on something that is
hopefully ready.
-- Steve
> On May 24, 2022, at 7:45 AM, Steven Rostedt <[email protected]> wrote:
>
> On Thu, 12 May 2022 15:08:08 -0700
> Song Liu <[email protected]> wrote:
>
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -4465,7 +4465,7 @@ int ftrace_func_mapper_add_ip(struct ftrace_func_mapper *mapper,
>> * @ip: The instruction pointer address to remove the data from
>> *
>> * Returns the data if it is found, otherwise NULL.
>> - * Note, if the data pointer is used as the data itself, (see
>> + * Note, if the data pointer is used as the data itself, (see
>> * ftrace_func_mapper_find_ip(), then the return value may be meaningless,
>> * if the data pointer was set to zero.
>> */
>> @@ -5200,8 +5200,10 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
>>
>> if (!ret && !(direct_ops.flags & FTRACE_OPS_FL_ENABLED)) {
>> ret = register_ftrace_function(&direct_ops);
>> - if (ret)
>> + if (ret) {
>> ftrace_set_filter_ip(&direct_ops, ip, 1, 0);
>> + remove_hash_entry(direct_functions, entry);
>> + }
>> }
>>
>> if (ret) {
>
> Perhaps something like this is more robust?
Yeah, this does look better. Do I need to send v2 with this version?
Thanks,
Song
>
> -- Steve
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 4f1d2f5e7263..cd38ad490174 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5195,8 +5195,6 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
> goto out_unlock;
>
> ret = ftrace_set_filter_ip(&direct_ops, ip, 0, 0);
> - if (ret)
> - remove_hash_entry(direct_functions, entry);
>
> if (!ret && !(direct_ops.flags & FTRACE_OPS_FL_ENABLED)) {
> ret = register_ftrace_function(&direct_ops);
> @@ -5205,6 +5203,7 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
> }
>
> if (ret) {
> + remove_hash_entry(direct_functions, entry);
> kfree(entry);
> if (!direct->count) {
> list_del_rcu(&direct->next);
>
On Thu, 12 May 2022 15:08:08 -0700
Song Liu <[email protected]> wrote:
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -4465,7 +4465,7 @@ int ftrace_func_mapper_add_ip(struct ftrace_func_mapper *mapper,
> * @ip: The instruction pointer address to remove the data from
> *
> * Returns the data if it is found, otherwise NULL.
> - * Note, if the data pointer is used as the data itself, (see
> + * Note, if the data pointer is used as the data itself, (see
> * ftrace_func_mapper_find_ip(), then the return value may be meaningless,
> * if the data pointer was set to zero.
> */
> @@ -5200,8 +5200,10 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
>
> if (!ret && !(direct_ops.flags & FTRACE_OPS_FL_ENABLED)) {
> ret = register_ftrace_function(&direct_ops);
> - if (ret)
> + if (ret) {
> ftrace_set_filter_ip(&direct_ops, ip, 1, 0);
> + remove_hash_entry(direct_functions, entry);
> + }
> }
>
> if (ret) {
Perhaps something like this is more robust?
-- Steve
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 4f1d2f5e7263..cd38ad490174 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5195,8 +5195,6 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
goto out_unlock;
ret = ftrace_set_filter_ip(&direct_ops, ip, 0, 0);
- if (ret)
- remove_hash_entry(direct_functions, entry);
if (!ret && !(direct_ops.flags & FTRACE_OPS_FL_ENABLED)) {
ret = register_ftrace_function(&direct_ops);
@@ -5205,6 +5203,7 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
}
if (ret) {
+ remove_hash_entry(direct_functions, entry);
kfree(entry);
if (!direct->count) {
list_del_rcu(&direct->next);
On Tue, 24 May 2022 15:33:36 +0000
Song Liu <[email protected]> wrote:
> Yeah, this does look better. Do I need to send v2 with this version?
Yes please. And as a new thread (not a reply to this one).
Thanks,
-- Steve