From: rostedt at goodmis.org (Steven Rostedt) Subject: [RFC][PATCH 1/2] x86: Allow breakpoints to emulate call functions Date: Fri, 3 May 2019 18:49:19 -0400 [thread overview] Message-ID: <20190503184919.2b7ef242@gandalf.local.home> (raw) In-Reply-To: <CAHk-=wiA-WbrFrDs-kOfJZMXy4zMo9-SZfk=7B-GfmBJ866naw@mail.gmail.com> On Fri, 3 May 2019 14:46:11 -0700 Linus Torvalds <torvalds at linux-foundation.org> wrote: > On Fri, May 3, 2019 at 12:24 PM Steven Rostedt <rostedt at goodmis.org> wrote: > > > > The problem with this approach is that it would require doing the same > > for x86_64, as the int3 C code is the same for both. And that may be a > > bit more difficult on the x86_64 side because it's all done with a > > simple flag in the idtentry macro to add the gap. > > That argument is weakened by the fact that we have to do _other_ > things differently on 32-bit and 64-bit anyway. > > So we might as well have a "on 32-bit, the call emulation needs to > move the pt_regs to make space" special case in the call emulation > code. It's very easy to explain why. So if I understand correctly what you are implying, is to have the int3 code be different for 32 bit and 64 bit? This would require handling text_poke, ftrace and kprobes differently for the two. Or perhaps we can build hacks on top. > > And then we'd limit the special case to where it matters (with a big > comment about what's going on), rather than adding random special case > handling to random _other_ places. > > Having to add s magic special case to "kernel_stack_pointer() is > certainly not obvious. Neither is adding magic special cases to system > call exit paths etc. Honestly, this sounds more of an argument for doing the buffered space for all exceptions on 32 bit, because it removes one of the special cases. If we were to move the frame and give it a full frame like x86_64, then we can remove kernel_stack_pointer() altogether. I've hated that function for some time, as I tripped over it in the past too, and it keeps coming up. It's sad that regs->sp is unreliable as is. > > This has been why I've been arguing against the entry code changes. > Exactly because they tend to have these kind of odd cascading effects. > The entry code is fragile not just because it's a complex hardware > interface, but also because we know about those complex hardware > interfaces in random other places. IMO, getting rid of the kernel_stack_pointer() function is a positive side effect of these changes. > > I'd much rather have the code that does special things be in one > place, and be the place that *needs* to do the special thing. If we > copy the pt_regs around when we do the "call" emulation, it's *really* > easy to explain *exactly* what we're doing and why in *exactly* that > one context where we are doing it. And it won't affect anything else, > and our existing code that looks at pt_regs will work both before and > after. > > Would it need a #ifdef CONFIG_X86_32 around it because it's not needed > on x86-64? Sure. But that #ifdef would be right there, and the comment > that explains why the pt_regs need to be moved would also make it very > obvious why it is only needed for x86-32. > > There's a lot of advantages to keeping your problems localized, > instead of letting your random hacks escape and become problems for > other, entirely unrelated, code. But is it localize? It would definitely affect do_int3(). You are saying that we have a do_int3() for user space int3, and do_kernel_int3() for kernel space. That would need to be done in asm for both, because having x86_64 call do_int3() for kernel and user would be interesting. Looking at the do_int3() code, I'm not sure how to safely separate kernel and user int3 handlers if 64bit doesn't call do_kernel_int3() directly. It may end up looking something like this: dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code) { #ifdef CONFIG_X86_64 do_kernel_int3(®s); #endif /* * Use ist_enter despite the fact that we don't use an IST stack. * We can be called from a kprobe in non-CONTEXT_KERNEL kernel * mode or even during context tracking state changes. * * This means that we can't schedule. That's okay. */ ist_enter(regs); RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); #ifdef CONFIG_X86_64 #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP, SIGTRAP) == NOTIFY_STOP) goto exit; #endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */ #ifdef CONFIG_KPROBES if (kprobe_int3_handler(®s)) goto exit; #endif #endif if (notify_die(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP, SIGTRAP) == NOTIFY_STOP) goto exit; cond_local_irq_enable(regs); do_trap(X86_TRAP_BP, SIGTRAP, "int3", regs, error_code, 0, NULL); cond_local_irq_disable(regs); exit: ist_exit(regs); } dotraplinkage void notrace do_kernel_int3(struct pt_regs **regs) { #ifdef CONFIG_DYNAMIC_FTRACE /* * ftrace must be first, everything else may cause a recursive crash. * See note by declaration of modifying_ftrace_code in ftrace.c */ if (unlikely(atomic_read(&modifying_ftrace_code)) && ftrace_int3_handler(regs)) return; #endif if (poke_int3_handler(regs)) return; #ifdef CONFIG_X86_64 return; #endif /* * Use ist_enter despite the fact that we don't use an IST stack. * We can be called from a kprobe in non-CONTEXT_KERNEL kernel * mode or even during context tracking state changes. * * This means that we can't schedule. That's okay. */ ist_enter(*regs); RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP if (kgdb_ll_trap(DIE_INT3, "int3", *regs, error_code, X86_TRAP_BP, SIGTRAP) == NOTIFY_STOP) goto exit; #endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */ #ifdef CONFIG_KPROBES if (kprobe_int3_handler(regs)) goto exit; #endif notify_die(DIE_INT3, "int3", *regs, error_code, X86_TRAP_BP, SIGTRAP); ist_exit(*regs); } Or maybe I misunderstood what you envision :-/ -- 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: Fri, 3 May 2019 18:49:19 -0400 [thread overview] Message-ID: <20190503184919.2b7ef242@gandalf.local.home> (raw) Message-ID: <20190503224919.98oBV2ifPYMhWVWZiRS_TEMCtUuTA5IXBBXaBoIkEIo@z> (raw) In-Reply-To: <CAHk-=wiA-WbrFrDs-kOfJZMXy4zMo9-SZfk=7B-GfmBJ866naw@mail.gmail.com> On Fri, 3 May 2019 14:46:11 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, May 3, 2019@12:24 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > The problem with this approach is that it would require doing the same > > for x86_64, as the int3 C code is the same for both. And that may be a > > bit more difficult on the x86_64 side because it's all done with a > > simple flag in the idtentry macro to add the gap. > > That argument is weakened by the fact that we have to do _other_ > things differently on 32-bit and 64-bit anyway. > > So we might as well have a "on 32-bit, the call emulation needs to > move the pt_regs to make space" special case in the call emulation > code. It's very easy to explain why. So if I understand correctly what you are implying, is to have the int3 code be different for 32 bit and 64 bit? This would require handling text_poke, ftrace and kprobes differently for the two. Or perhaps we can build hacks on top. > > And then we'd limit the special case to where it matters (with a big > comment about what's going on), rather than adding random special case > handling to random _other_ places. > > Having to add s magic special case to "kernel_stack_pointer() is > certainly not obvious. Neither is adding magic special cases to system > call exit paths etc. Honestly, this sounds more of an argument for doing the buffered space for all exceptions on 32 bit, because it removes one of the special cases. If we were to move the frame and give it a full frame like x86_64, then we can remove kernel_stack_pointer() altogether. I've hated that function for some time, as I tripped over it in the past too, and it keeps coming up. It's sad that regs->sp is unreliable as is. > > This has been why I've been arguing against the entry code changes. > Exactly because they tend to have these kind of odd cascading effects. > The entry code is fragile not just because it's a complex hardware > interface, but also because we know about those complex hardware > interfaces in random other places. IMO, getting rid of the kernel_stack_pointer() function is a positive side effect of these changes. > > I'd much rather have the code that does special things be in one > place, and be the place that *needs* to do the special thing. If we > copy the pt_regs around when we do the "call" emulation, it's *really* > easy to explain *exactly* what we're doing and why in *exactly* that > one context where we are doing it. And it won't affect anything else, > and our existing code that looks at pt_regs will work both before and > after. > > Would it need a #ifdef CONFIG_X86_32 around it because it's not needed > on x86-64? Sure. But that #ifdef would be right there, and the comment > that explains why the pt_regs need to be moved would also make it very > obvious why it is only needed for x86-32. > > There's a lot of advantages to keeping your problems localized, > instead of letting your random hacks escape and become problems for > other, entirely unrelated, code. But is it localize? It would definitely affect do_int3(). You are saying that we have a do_int3() for user space int3, and do_kernel_int3() for kernel space. That would need to be done in asm for both, because having x86_64 call do_int3() for kernel and user would be interesting. Looking at the do_int3() code, I'm not sure how to safely separate kernel and user int3 handlers if 64bit doesn't call do_kernel_int3() directly. It may end up looking something like this: dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code) { #ifdef CONFIG_X86_64 do_kernel_int3(®s); #endif /* * Use ist_enter despite the fact that we don't use an IST stack. * We can be called from a kprobe in non-CONTEXT_KERNEL kernel * mode or even during context tracking state changes. * * This means that we can't schedule. That's okay. */ ist_enter(regs); RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); #ifdef CONFIG_X86_64 #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP, SIGTRAP) == NOTIFY_STOP) goto exit; #endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */ #ifdef CONFIG_KPROBES if (kprobe_int3_handler(®s)) goto exit; #endif #endif if (notify_die(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP, SIGTRAP) == NOTIFY_STOP) goto exit; cond_local_irq_enable(regs); do_trap(X86_TRAP_BP, SIGTRAP, "int3", regs, error_code, 0, NULL); cond_local_irq_disable(regs); exit: ist_exit(regs); } dotraplinkage void notrace do_kernel_int3(struct pt_regs **regs) { #ifdef CONFIG_DYNAMIC_FTRACE /* * ftrace must be first, everything else may cause a recursive crash. * See note by declaration of modifying_ftrace_code in ftrace.c */ if (unlikely(atomic_read(&modifying_ftrace_code)) && ftrace_int3_handler(regs)) return; #endif if (poke_int3_handler(regs)) return; #ifdef CONFIG_X86_64 return; #endif /* * Use ist_enter despite the fact that we don't use an IST stack. * We can be called from a kprobe in non-CONTEXT_KERNEL kernel * mode or even during context tracking state changes. * * This means that we can't schedule. That's okay. */ ist_enter(*regs); RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP if (kgdb_ll_trap(DIE_INT3, "int3", *regs, error_code, X86_TRAP_BP, SIGTRAP) == NOTIFY_STOP) goto exit; #endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */ #ifdef CONFIG_KPROBES if (kprobe_int3_handler(regs)) goto exit; #endif notify_die(DIE_INT3, "int3", *regs, error_code, X86_TRAP_BP, SIGTRAP); ist_exit(*regs); } Or maybe I misunderstood what you envision :-/ -- Steve
next prev parent reply other threads:[~2019-05-03 22:49 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 [this message] 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 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=20190503184919.2b7ef242@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).