From: rostedt at goodmis.org (Steven Rostedt) Subject: [RFC][PATCH 1/2] x86: Allow breakpoints to emulate call functions Date: Mon, 6 May 2019 16:29:15 -0400 [thread overview] Message-ID: <20190506162915.380993f9@gandalf.local.home> (raw) In-Reply-To: <CAHk-=witfFBW2O5v6g--FmqnAFsMkKNLosTFfWyaoJ7euQF8kQ@mail.gmail.com> On Mon, 6 May 2019 12:46:11 -0700 Linus Torvalds <torvalds at linux-foundation.org> wrote: > On Mon, May 6, 2019 at 11:57 AM Steven Rostedt <rostedt at goodmis.org> wrote: > > > > You should have waited another week to open that merge window ;-) > > Hmm. I'm looking at it while the test builds happen, and since I don't > see what's wrong in the low-level entry code, I'm looking at the > ftrace code instead. > > What's going on here? > > *pregs = int3_emulate_call(regs, (unsigned > long)ftrace_regs_caller); > > that line makes no sense. Why would we emulate a call to > ftrace_regs_caller()? That function sets up a pt_regs, and then calls > ftrace_stub(). Because that call to ftrace_stub is also dynamic. In entry_32.S .globl ftrace_call ftrace_call: call ftrace_stub update_ftrace_func() { [..] } else if (is_ftrace_caller(ip)) { if (!ftrace_update_func_call) { int3_emulate_jmp(regs, ip + CALL_INSN_SIZE); return 1; } *pregs = int3_emulate_call(regs, ftrace_update_func_call); return 1; } Part of the code will change it to call the function needed directly. struct ftrace_ops my_ops { .func = my_handler }; register_ftrace_function(&my_ops); Will change "call ftrace_stub" into "call my_handler" If you register another ftrace_ops, it will change that to call ftrace_ops_list_func Which will iterate over all registered ftrace_ops, and depending on the hashs in ftrace_ops, will call the registered handler for them. > > But we *have* pt_regs here already with the right values. Why isn't > this just a direct call to ftrace_stub() from within the int3 handler? > > And the thing is, calling ftrace_regs_caller() looks wrong, because > that's not what happens for *real* mcount handling, which uses that > "create_trampoline()" to create the thing we're supposed to really > use? The ftrace_regs_caller() is what is called if there's two or more callbacks registered to a single function. For example, you have a function that is being lived patch (it uses the ftrace_regs_caller copy of the trampoline). But if you enable function tracing (which doesn't need a copy of regs), it will call the ftrace_regs_caller, which will call a ftrace_ops_list_func() which will look at the ftrace_ops (which is the descriptor representing registered callbacks to ftrace), and based on the hash value in them, will call their handler if the ftrace_ops hashes match the function being called. > > Anyway, I simply don't understand the code, so I'm confused. But why > is the int3 emulation creating a call that doesn't seem to match what > the instruction that we're actually rewriting is supposed to do? > > IOW, it looks to me like ftrace_int3_handler() is actually emulating > something different than what ftrace_modify_code() is actually > modifying the code to do! > > Since the only caller of ftrace_modify_code() is update_ftrace_func(), > why is that function not just saving the target and we'd emulate the > call to that? Using anything else looks crazy? > > But as mentioned, I just don't understand the ftrace logic. It looks > insane to me, and much more likely to be buggy than the very simple > entry code. Let's go an example. Let's say we live patched do_IRQ() and __migrate_task(). We would have this: live_patch_trampoline: (which is a modified copy of the ftrace_regs_caller) pushl $__KERNEL_CS pushl 4(%esp) pushl $0 pushl %gs pushl %fs pushl %es pushl %ds pushl %eax pushf popl %eax movl %eax, 8*4(%esp) pushl %ebp pushl %edi pushl %esi pushl %edx pushl %ecx pushl %ebx movl 12*4(%esp), %eax subl $MCOUNT_INSN_SIZE, %eax movl 15*4(%esp), %edx /* Load parent ip (2nd parameter) */ movl function_trace_op, %ecx /* Save ftrace_pos in 3rd parameter */ pushl %esp /* Save pt_regs as 4th parameter */ call live_kernel_patch_func addl $4, %esp /* Skip pt_regs */ push 14*4(%esp) popf movl 12*4(%esp), %eax movl %eax, 14*4(%esp) popl %ebx popl %ecx popl %edx popl %esi popl %edi popl %ebp popl %eax popl %ds popl %es popl %fs popl %gs lea 3*4(%esp), %esp /* Skip orig_ax, ip and cs */ jmp .Lftrace_ret <do_IRQ>: call live_patch_trampoline [..] <__migrate_task>: call_live_patch_trampoline Now we enable function tracing on all functions that can be traced, and this includes do_IRQ() and __migrate_task(). Thus, we first modify that call to ftrace_stub in the ftrace_regs_caller to point to the ftrace_ops_list_func() as that will iterate over the ftrace_ops for live kernel patching, and the ftrace_ops for the function tracer. That iterator will check the hashes against the called functions, and for live kernel patching, it will it will call its handler if the passed in ip matches either do_IRQ() or __migrate_task(). It will see that the ftrace_ops for function tracing is set to trace all functions and just call its handler in that loop too. Today, when we place an int3 on those functions, we basically turn them into nops. <do_IRQ>: <int3>(convert from call live_patch_trampoline to call ftrace_regs_caller) [..] But that int3 handler, doesn't call either the live_patch_trampoline or ftrace_regs_caller, which means, the live kernel patching doesn't get to make that function call something different. We basically, just disabled tracing completely for those functions during that transition. Remember that ftrace_regs_caller gets updated to not call ftrace_stub, but to the list iterator if there's more than one handler registered with ftrace (and so does ftrace_caller). By making the int3 handler call it, will do the iteration over all registered ftrace_ops and nothing will be missed. Does that help explain what's going on? -- Steve
WARNING: multiple messages have this Message-ID (diff)
From: rostedt@goodmis.org (Steven Rostedt) Subject: [RFC][PATCH 1/2] x86: Allow breakpoints to emulate call functions Date: Mon, 6 May 2019 16:29:15 -0400 [thread overview] Message-ID: <20190506162915.380993f9@gandalf.local.home> (raw) Message-ID: <20190506202915.ubiP04r5HML2YS_G7xNIvXLCMPPkl7K3o01SyNPnOTw@z> (raw) In-Reply-To: <CAHk-=witfFBW2O5v6g--FmqnAFsMkKNLosTFfWyaoJ7euQF8kQ@mail.gmail.com> On Mon, 6 May 2019 12:46:11 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Mon, May 6, 2019@11:57 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > You should have waited another week to open that merge window ;-) > > Hmm. I'm looking at it while the test builds happen, and since I don't > see what's wrong in the low-level entry code, I'm looking at the > ftrace code instead. > > What's going on here? > > *pregs = int3_emulate_call(regs, (unsigned > long)ftrace_regs_caller); > > that line makes no sense. Why would we emulate a call to > ftrace_regs_caller()? That function sets up a pt_regs, and then calls > ftrace_stub(). Because that call to ftrace_stub is also dynamic. In entry_32.S .globl ftrace_call ftrace_call: call ftrace_stub update_ftrace_func() { [..] } else if (is_ftrace_caller(ip)) { if (!ftrace_update_func_call) { int3_emulate_jmp(regs, ip + CALL_INSN_SIZE); return 1; } *pregs = int3_emulate_call(regs, ftrace_update_func_call); return 1; } Part of the code will change it to call the function needed directly. struct ftrace_ops my_ops { .func = my_handler }; register_ftrace_function(&my_ops); Will change "call ftrace_stub" into "call my_handler" If you register another ftrace_ops, it will change that to call ftrace_ops_list_func Which will iterate over all registered ftrace_ops, and depending on the hashs in ftrace_ops, will call the registered handler for them. > > But we *have* pt_regs here already with the right values. Why isn't > this just a direct call to ftrace_stub() from within the int3 handler? > > And the thing is, calling ftrace_regs_caller() looks wrong, because > that's not what happens for *real* mcount handling, which uses that > "create_trampoline()" to create the thing we're supposed to really > use? The ftrace_regs_caller() is what is called if there's two or more callbacks registered to a single function. For example, you have a function that is being lived patch (it uses the ftrace_regs_caller copy of the trampoline). But if you enable function tracing (which doesn't need a copy of regs), it will call the ftrace_regs_caller, which will call a ftrace_ops_list_func() which will look at the ftrace_ops (which is the descriptor representing registered callbacks to ftrace), and based on the hash value in them, will call their handler if the ftrace_ops hashes match the function being called. > > Anyway, I simply don't understand the code, so I'm confused. But why > is the int3 emulation creating a call that doesn't seem to match what > the instruction that we're actually rewriting is supposed to do? > > IOW, it looks to me like ftrace_int3_handler() is actually emulating > something different than what ftrace_modify_code() is actually > modifying the code to do! > > Since the only caller of ftrace_modify_code() is update_ftrace_func(), > why is that function not just saving the target and we'd emulate the > call to that? Using anything else looks crazy? > > But as mentioned, I just don't understand the ftrace logic. It looks > insane to me, and much more likely to be buggy than the very simple > entry code. Let's go an example. Let's say we live patched do_IRQ() and __migrate_task(). We would have this: live_patch_trampoline: (which is a modified copy of the ftrace_regs_caller) pushl $__KERNEL_CS pushl 4(%esp) pushl $0 pushl %gs pushl %fs pushl %es pushl %ds pushl %eax pushf popl %eax movl %eax, 8*4(%esp) pushl %ebp pushl %edi pushl %esi pushl %edx pushl %ecx pushl %ebx movl 12*4(%esp), %eax subl $MCOUNT_INSN_SIZE, %eax movl 15*4(%esp), %edx /* Load parent ip (2nd parameter) */ movl function_trace_op, %ecx /* Save ftrace_pos in 3rd parameter */ pushl %esp /* Save pt_regs as 4th parameter */ call live_kernel_patch_func addl $4, %esp /* Skip pt_regs */ push 14*4(%esp) popf movl 12*4(%esp), %eax movl %eax, 14*4(%esp) popl %ebx popl %ecx popl %edx popl %esi popl %edi popl %ebp popl %eax popl %ds popl %es popl %fs popl %gs lea 3*4(%esp), %esp /* Skip orig_ax, ip and cs */ jmp .Lftrace_ret <do_IRQ>: call live_patch_trampoline [..] <__migrate_task>: call_live_patch_trampoline Now we enable function tracing on all functions that can be traced, and this includes do_IRQ() and __migrate_task(). Thus, we first modify that call to ftrace_stub in the ftrace_regs_caller to point to the ftrace_ops_list_func() as that will iterate over the ftrace_ops for live kernel patching, and the ftrace_ops for the function tracer. That iterator will check the hashes against the called functions, and for live kernel patching, it will it will call its handler if the passed in ip matches either do_IRQ() or __migrate_task(). It will see that the ftrace_ops for function tracing is set to trace all functions and just call its handler in that loop too. Today, when we place an int3 on those functions, we basically turn them into nops. <do_IRQ>: <int3>(convert from call live_patch_trampoline to call ftrace_regs_caller) [..] But that int3 handler, doesn't call either the live_patch_trampoline or ftrace_regs_caller, which means, the live kernel patching doesn't get to make that function call something different. We basically, just disabled tracing completely for those functions during that transition. Remember that ftrace_regs_caller gets updated to not call ftrace_stub, but to the list iterator if there's more than one handler registered with ftrace (and so does ftrace_caller). By making the int3 handler call it, will do the iteration over all registered ftrace_ops and nothing will be missed. Does that help explain what's going on? -- Steve
next prev parent reply other threads:[~2019-05-06 20:29 UTC|newest] Thread overview: 204+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <20190501202830.347656894@goodmis.org> 2019-05-01 20:28 ` [RFC][PATCH 1/2] x86: Allow breakpoints to emulate call functions rostedt 2019-05-01 20:28 ` Steven Rostedt 2019-05-02 3:24 ` rostedt 2019-05-02 3:24 ` Steven Rostedt 2019-05-02 16:21 ` peterz 2019-05-02 16:21 ` Peter Zijlstra 2019-05-02 16:29 ` peterz 2019-05-02 16:29 ` Peter Zijlstra 2019-05-02 18:02 ` torvalds 2019-05-02 18:02 ` Linus Torvalds 2019-05-02 18:18 ` peterz 2019-05-02 18:18 ` Peter Zijlstra 2019-05-02 18:30 ` peterz 2019-05-02 18:30 ` Peter Zijlstra 2019-05-02 18:43 ` torvalds 2019-05-02 18:43 ` Linus Torvalds 2019-05-02 19:28 ` jikos 2019-05-02 19:28 ` Jiri Kosina 2019-05-02 20:25 ` luto 2019-05-02 20:25 ` Andy Lutomirski 2019-05-02 20:21 ` peterz 2019-05-02 20:21 ` Peter Zijlstra 2019-05-02 20:49 ` torvalds 2019-05-02 20:49 ` Linus Torvalds 2019-05-02 21:32 ` peterz 2019-05-02 21:32 ` Peter Zijlstra 2019-05-03 19:24 ` rostedt 2019-05-03 19:24 ` Steven Rostedt 2019-05-03 21:46 ` torvalds 2019-05-03 21:46 ` Linus Torvalds 2019-05-03 22:49 ` rostedt 2019-05-03 22:49 ` Steven Rostedt 2019-05-03 23:07 ` torvalds 2019-05-03 23:07 ` Linus Torvalds 2019-05-04 4:17 ` rostedt 2019-05-04 4:17 ` Steven Rostedt [not found] ` <CAHk-=wiuSFbv_rELND-BLWcP0GSZ0yF=xOAEcf61GE3bU9d=yg@mail.gmail.com> 2019-05-04 18:59 ` torvalds 2019-05-04 18:59 ` Linus Torvalds 2019-05-04 20:12 ` luto 2019-05-04 20:12 ` Andy Lutomirski 2019-05-04 20:28 ` torvalds 2019-05-04 20:28 ` Linus Torvalds 2019-05-04 20:36 ` torvalds 2019-05-04 20:36 ` Linus Torvalds 2019-05-03 22:55 ` luto 2019-05-03 22:55 ` Andy Lutomirski 2019-05-03 23:16 ` torvalds 2019-05-03 23:16 ` Linus Torvalds 2019-05-03 23:32 ` luto 2019-05-03 23:32 ` Andy Lutomirski 2019-05-02 22:52 ` rostedt 2019-05-02 22:52 ` Steven Rostedt 2019-05-02 23:31 ` rostedt 2019-05-02 23:31 ` Steven Rostedt 2019-05-02 23:50 ` rostedt 2019-05-02 23:50 ` Steven Rostedt 2019-05-03 1:51 ` [RFC][PATCH 1/2 v2] " rostedt 2019-05-03 1:51 ` Steven Rostedt 2019-05-03 9:29 ` [RFC][PATCH 1/2] " peterz 2019-05-03 9:29 ` Peter Zijlstra 2019-05-03 13:22 ` rostedt 2019-05-03 13:22 ` Steven Rostedt 2019-05-03 16:20 ` luto 2019-05-03 16:20 ` Andy Lutomirski 2019-05-03 16:31 ` rostedt 2019-05-03 16:31 ` Steven Rostedt 2019-05-03 16:35 ` peterz 2019-05-03 16:35 ` Peter Zijlstra 2019-05-03 16:44 ` luto 2019-05-03 16:44 ` Andy Lutomirski 2019-05-03 16:49 ` rostedt 2019-05-03 16:49 ` Steven Rostedt 2019-05-03 16:32 ` peterz 2019-05-03 16:32 ` Peter Zijlstra 2019-05-03 18:57 ` torvalds 2019-05-03 18:57 ` Linus Torvalds 2019-05-06 8:19 ` peterz 2019-05-06 8:19 ` Peter Zijlstra 2019-05-06 13:56 ` rostedt 2019-05-06 13:56 ` Steven Rostedt 2019-05-06 16:17 ` torvalds 2019-05-06 16:17 ` Linus Torvalds 2019-05-06 16:19 ` torvalds 2019-05-06 16:19 ` Linus Torvalds 2019-05-06 17:06 ` rostedt 2019-05-06 17:06 ` Steven Rostedt 2019-05-06 18:06 ` torvalds 2019-05-06 18:06 ` Linus Torvalds 2019-05-06 18:57 ` rostedt 2019-05-06 18:57 ` Steven Rostedt 2019-05-06 19:46 ` torvalds 2019-05-06 19:46 ` Linus Torvalds 2019-05-06 20:29 ` rostedt [this message] 2019-05-06 20:29 ` Steven Rostedt 2019-05-06 20:42 ` torvalds 2019-05-06 20:42 ` Linus Torvalds 2019-05-06 20:44 ` torvalds 2019-05-06 20:44 ` Linus Torvalds 2019-05-06 21:45 ` rostedt 2019-05-06 21:45 ` Steven Rostedt 2019-05-06 22:06 ` torvalds 2019-05-06 22:06 ` Linus Torvalds 2019-05-06 22:31 ` torvalds 2019-05-06 22:31 ` Linus Torvalds 2019-05-07 0:10 ` rostedt 2019-05-07 0:10 ` Steven Rostedt 2019-05-07 1:06 ` torvalds 2019-05-07 1:06 ` Linus Torvalds 2019-05-07 1:04 ` rostedt 2019-05-07 1:04 ` Steven Rostedt 2019-05-07 1:34 ` rostedt 2019-05-07 1:34 ` Steven Rostedt 2019-05-07 1:34 ` torvalds 2019-05-07 1:34 ` Linus Torvalds 2019-05-07 1:53 ` rostedt 2019-05-07 1:53 ` Steven Rostedt 2019-05-07 2:22 ` torvalds 2019-05-07 2:22 ` Linus Torvalds 2019-05-07 2:58 ` rostedt 2019-05-07 2:58 ` Steven Rostedt 2019-05-07 3:05 ` torvalds 2019-05-07 3:05 ` Linus Torvalds 2019-05-07 3:21 ` rostedt 2019-05-07 3:21 ` Steven Rostedt 2019-05-07 3:28 ` torvalds 2019-05-07 3:28 ` Linus Torvalds 2019-05-07 14:54 ` torvalds 2019-05-07 14:54 ` Linus Torvalds 2019-05-07 15:12 ` rostedt 2019-05-07 15:12 ` Steven Rostedt 2019-05-07 15:25 ` rostedt 2019-05-07 15:25 ` Steven Rostedt 2019-05-07 16:25 ` rostedt 2019-05-07 16:25 ` Steven Rostedt 2019-05-07 15:31 ` torvalds 2019-05-07 15:31 ` Linus Torvalds 2019-05-07 15:45 ` rostedt 2019-05-07 15:45 ` Steven Rostedt 2019-05-07 16:34 ` peterz 2019-05-07 16:34 ` Peter Zijlstra 2019-05-07 17:08 ` torvalds 2019-05-07 17:08 ` Linus Torvalds 2019-05-07 17:21 ` jpoimboe 2019-05-07 17:21 ` Josh Poimboeuf 2019-05-07 21:24 ` rostedt 2019-05-07 21:24 ` Steven Rostedt 2019-05-08 4:50 ` torvalds 2019-05-08 4:50 ` Linus Torvalds 2019-05-08 16:37 ` rostedt 2019-05-08 16:37 ` Steven Rostedt 2019-05-07 17:38 ` peterz 2019-05-07 17:38 ` Peter Zijlstra 2019-05-07 9:51 ` peterz 2019-05-07 9:51 ` Peter Zijlstra 2019-05-07 14:48 ` luto 2019-05-07 14:48 ` Andy Lutomirski 2019-05-07 14:57 ` torvalds 2019-05-07 14:57 ` Linus Torvalds 2019-05-07 14:13 ` mhiramat 2019-05-07 14:13 ` Masami Hiramatsu 2019-05-07 17:15 ` mhiramat 2019-05-07 17:15 ` Masami Hiramatsu 2019-05-06 14:22 ` peterz 2019-05-06 14:22 ` Peter Zijlstra 2019-05-07 8:57 ` peterz 2019-05-07 8:57 ` Peter Zijlstra 2019-05-07 9:18 ` David.Laight 2019-05-07 9:18 ` David Laight 2019-05-07 11:30 ` peterz 2019-05-07 11:30 ` Peter Zijlstra 2019-05-07 12:57 ` David.Laight 2019-05-07 12:57 ` David Laight 2019-05-07 13:14 ` rostedt 2019-05-07 13:14 ` Steven Rostedt 2019-05-07 14:50 ` David.Laight 2019-05-07 14:50 ` David Laight 2019-05-07 14:57 ` rostedt 2019-05-07 14:57 ` Steven Rostedt 2019-05-07 15:46 ` David.Laight 2019-05-07 15:46 ` David Laight 2019-05-07 13:32 ` peterz 2019-05-07 13:32 ` Peter Zijlstra 2019-05-07 9:27 ` peterz 2019-05-07 9:27 ` Peter Zijlstra 2019-05-07 12:27 ` rostedt 2019-05-07 12:27 ` Steven Rostedt 2019-05-07 12:41 ` peterz 2019-05-07 12:41 ` Peter Zijlstra 2019-05-07 12:54 ` rostedt 2019-05-07 12:54 ` Steven Rostedt 2019-05-07 17:22 ` masami.hiramatsu 2019-05-07 17:22 ` Masami Hiramatsu 2019-05-07 14:28 ` peterz 2019-05-07 14:28 ` Peter Zijlstra 2019-05-02 20:48 ` rostedt 2019-05-02 20:48 ` Steven Rostedt 2019-05-06 15:14 ` jpoimboe 2019-05-06 15:14 ` Josh Poimboeuf 2019-05-01 20:28 ` [RFC][PATCH 2/2] ftrace/x86: Emulate call function while updating in breakpoint handler rostedt 2019-05-01 20:28 ` Steven Rostedt 2019-05-03 10:22 ` [RFC][PATCH 1.5/2] x86: Add int3_emulate_call() selftest peterz 2019-05-03 10:22 ` Peter Zijlstra 2019-05-03 18:46 ` rostedt 2019-05-03 18:46 ` Steven Rostedt
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190506162915.380993f9@gandalf.local.home \ --to=linux-kselftest@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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).