| Message ID | 20160520140513.dsgfb5sei52qge3w@treble (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, May 20, 2016 at 7:05 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Thu, May 19, 2016 at 04:39:51PM -0700, Andy Lutomirski wrote: >> On Thu, May 19, 2016 at 4:15 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: >> > Note this example is with today's unwinder. It could be made smarter to >> > get the RIP from the pt_regs so the '?' could be removed from >> > copy_page_to_iter(). >> > >> > Thoughts? >> >> I think we should do that. The silly sample patch I sent you (or at >> least that I think I sent you) did that, and it worked nicely. > > Yeah, we can certainly do something similar to make the unwinder > smarter. It should be very simple with this approach: if it finds the > pt_regs() function on the stack, the (struct pt_regs *) pointer will be > right after it. That seems barely easier than checking if it finds a function in .entry that's marked on the stack, and the latter has no runtime cost. > >> > diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h >> > index 9a9e588..f54886a 100644 >> > --- a/arch/x86/entry/calling.h >> > +++ b/arch/x86/entry/calling.h >> > @@ -201,6 +201,32 @@ For 32-bit we have the following conventions - kernel is built with >> > .byte 0xf1 >> > .endm >> > >> > + /* >> > + * Create a stack frame for the saved pt_regs. This allows frame >> > + * pointer based unwinders to find pt_regs on the stack. >> > + */ >> > + .macro CREATE_PT_REGS_FRAME regs=%rsp >> > +#ifdef CONFIG_FRAME_POINTER >> > + pushq \regs >> > + pushq $pt_regs+1 >> > + pushq %rbp >> > + movq %rsp, %rbp >> > +#endif >> > + .endm >> >> I don't love this part. It's going to hurt performance, and, given >> that we need to change the unwinder anyway to make it useful, let's >> just emit a table somewhere in .rodata and use it directly. > > I'm not sure about the idea of a table. I get the feeling it would add > more complexity to both the entry code and the unwinder. How would you > specify the pt_regs location when it's on a different stack? (See the > interrupt macro: non-nested interrupts will place pt_regs on the task > stack before switching to the irq stack.) Hmm. I need to think about the interrupt stack case a bit. Although the actual top of the interrupt stack has a nearly fixed format, and I have old patches to clean it up and make it actually be fixed. I'll try to dust those off and resend them soon. > > If you're worried about performance, I can remove the syscall > annotations. They're optional anyway, since the pt_regs is always at > the same place on the stack for syscalls. > > I think three extra pushes wouldn't be a performance issue for > interrupts/exceptions. And they'll go away when we finally bury > CONFIG_FRAME_POINTER. I bet we'll always need to support CONFIG_FRAME_POINTER for some embedded systems. I'll play with this a bit. --Andy
On Fri, May 20, 2016 at 08:41:00AM -0700, Andy Lutomirski wrote: > On Fri, May 20, 2016 at 7:05 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Thu, May 19, 2016 at 04:39:51PM -0700, Andy Lutomirski wrote: > >> On Thu, May 19, 2016 at 4:15 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > >> > Note this example is with today's unwinder. It could be made smarter to > >> > get the RIP from the pt_regs so the '?' could be removed from > >> > copy_page_to_iter(). > >> > > >> > Thoughts? > >> > >> I think we should do that. The silly sample patch I sent you (or at > >> least that I think I sent you) did that, and it worked nicely. > > > > Yeah, we can certainly do something similar to make the unwinder > > smarter. It should be very simple with this approach: if it finds the > > pt_regs() function on the stack, the (struct pt_regs *) pointer will be > > right after it. > > That seems barely easier than checking if it finds a function in > .entry that's marked on the stack, Don't forget you'd also have to look up the function's pt_regs offset in the table. > and the latter has no runtime cost. Well, I had been assuming the three extra pushes and one extra pop for interrupts would be negligible, but I'm definitely no expert there. Any idea how I can measure it? > > I'm not sure about the idea of a table. I get the feeling it would add > > more complexity to both the entry code and the unwinder. How would you > > specify the pt_regs location when it's on a different stack? (See the > > interrupt macro: non-nested interrupts will place pt_regs on the task > > stack before switching to the irq stack.) > > Hmm. I need to think about the interrupt stack case a bit. Although > the actual top of the interrupt stack has a nearly fixed format, and I > have old patches to clean it up and make it actually be fixed. I'll > try to dust those off and resend them soon. How would that solve the problem? Would pt_regs be moved or copied to the irq stack? > > If you're worried about performance, I can remove the syscall > > annotations. They're optional anyway, since the pt_regs is always at > > the same place on the stack for syscalls. > > > > I think three extra pushes wouldn't be a performance issue for > > interrupts/exceptions. And they'll go away when we finally bury > > CONFIG_FRAME_POINTER. > > I bet we'll always need to support CONFIG_FRAME_POINTER for some > embedded systems. Yeah, probably. > I'll play with this a bit. Thanks, looking forward to seeing what you come up with.
On Fri, May 20, 2016 at 9:41 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Fri, May 20, 2016 at 08:41:00AM -0700, Andy Lutomirski wrote: >> On Fri, May 20, 2016 at 7:05 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: >> > On Thu, May 19, 2016 at 04:39:51PM -0700, Andy Lutomirski wrote: >> >> On Thu, May 19, 2016 at 4:15 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: >> >> > Note this example is with today's unwinder. It could be made smarter to >> >> > get the RIP from the pt_regs so the '?' could be removed from >> >> > copy_page_to_iter(). >> >> > >> >> > Thoughts? >> >> >> >> I think we should do that. The silly sample patch I sent you (or at >> >> least that I think I sent you) did that, and it worked nicely. >> > >> > Yeah, we can certainly do something similar to make the unwinder >> > smarter. It should be very simple with this approach: if it finds the >> > pt_regs() function on the stack, the (struct pt_regs *) pointer will be >> > right after it. >> >> That seems barely easier than checking if it finds a function in >> .entry that's marked on the stack, > > Don't forget you'd also have to look up the function's pt_regs offset in > the table. > >> and the latter has no runtime cost. > > Well, I had been assuming the three extra pushes and one extra pop for > interrupts would be negligible, but I'm definitely no expert there. Any > idea how I can measure it? I think it would be negligible, at least for interrupts, since interrupts are already extremely expensive. But I don't love adding assembly code that makes them even slower. The real thing I dislike about this approach is that it's not a normal stack frame, so you need code in the unwinder to unwind through it correctly, which makes me think that you're not saving much complexity by adding the pushes. But maybe I'm wrong. > >> > I'm not sure about the idea of a table. I get the feeling it would add >> > more complexity to both the entry code and the unwinder. How would you >> > specify the pt_regs location when it's on a different stack? (See the >> > interrupt macro: non-nested interrupts will place pt_regs on the task >> > stack before switching to the irq stack.) >> >> Hmm. I need to think about the interrupt stack case a bit. Although >> the actual top of the interrupt stack has a nearly fixed format, and I >> have old patches to clean it up and make it actually be fixed. I'll >> try to dust those off and resend them soon. > > How would that solve the problem? Would pt_regs be moved or copied to > the irq stack? Hmm. Maybe the right way would be to unwind off the IRQ stack in two steps. Step one would be to figure out that you're on the IRQ stack and pop your way off it. Step two would be to find pt_regs. But that could be rather nasty to implement. Maybe what we actually want to do is this: First, apply this thing: https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=x86/entry_ist&id=2231ec7e0bcc1a2bc94a17081511ab54cc6badd1 that gives us a common format for the IRQ stack. Second, use my idea of making a table of offsets to pt_regs, so we'd have, roughly: ENTER_IRQ_STACK old_rsp=%r11 call __do_softirq ANNOTATE_IRQSTACK_PTREGS_CALL offset=0 LEAVE_IRQ_STACK the idea here is that offset=0 means that there is no offset beyond that implied by ENTER_IRQ_STACK. What actually gets written to the table is offset 8, because ENTER_IRQ_STACK itself does one push. So far, this has no runtime overhead at all. Then, in the unwinder, the logic becomes: If the return address is annotated in the ptregs entry table, look up the offset. If the offset is in bounds, then use it. If the offset is out of bounds and we're on the IRQ stack, then unwind the ENTER_IRQ_STACK as well. Does that seem reasonable? I can try to implement it and see what it looks like. --Andy > >> > If you're worried about performance, I can remove the syscall >> > annotations. They're optional anyway, since the pt_regs is always at >> > the same place on the stack for syscalls. >> > >> > I think three extra pushes wouldn't be a performance issue for >> > interrupts/exceptions. And they'll go away when we finally bury >> > CONFIG_FRAME_POINTER. >> >> I bet we'll always need to support CONFIG_FRAME_POINTER for some >> embedded systems. > > Yeah, probably. > >> I'll play with this a bit. > > Thanks, looking forward to seeing what you come up with. > > -- > Josh
On Fri, May 20, 2016 at 09:59:38AM -0700, Andy Lutomirski wrote: > On Fri, May 20, 2016 at 9:41 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Fri, May 20, 2016 at 08:41:00AM -0700, Andy Lutomirski wrote: > >> On Fri, May 20, 2016 at 7:05 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > >> > On Thu, May 19, 2016 at 04:39:51PM -0700, Andy Lutomirski wrote: > >> >> On Thu, May 19, 2016 at 4:15 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > >> >> > Note this example is with today's unwinder. It could be made smarter to > >> >> > get the RIP from the pt_regs so the '?' could be removed from > >> >> > copy_page_to_iter(). > >> >> > > >> >> > Thoughts? > >> >> > >> >> I think we should do that. The silly sample patch I sent you (or at > >> >> least that I think I sent you) did that, and it worked nicely. > >> > > >> > Yeah, we can certainly do something similar to make the unwinder > >> > smarter. It should be very simple with this approach: if it finds the > >> > pt_regs() function on the stack, the (struct pt_regs *) pointer will be > >> > right after it. > >> > >> That seems barely easier than checking if it finds a function in > >> .entry that's marked on the stack, > > > > Don't forget you'd also have to look up the function's pt_regs offset in > > the table. > > > >> and the latter has no runtime cost. > > > > Well, I had been assuming the three extra pushes and one extra pop for > > interrupts would be negligible, but I'm definitely no expert there. Any > > idea how I can measure it? > > I think it would be negligible, at least for interrupts, since > interrupts are already extremely expensive. But I don't love adding > assembly code that makes them even slower. I just don't find that very convincing :-) If the performance impact is negligible, we should ignore it. We should instead be focusing on finding the simplest solution. > The real thing I dislike about this approach is that it's not a normal > stack frame, so you need code in the unwinder to unwind through it > correctly, which makes me think that you're not saving much complexity > by adding the pushes. But maybe I'm wrong. Hm, I view it differently. Sure the stack frame is a bit unusual, but as far as unwinders go, it _is_ normal. So even a stock unwinder can show the user that a pt_regs() -- or interrupt_frame() or whatever we call it -- function was called. Just knowing that an interrupt occurred could be useful information, even without the contents of RIP. > >> > I'm not sure about the idea of a table. I get the feeling it would add > >> > more complexity to both the entry code and the unwinder. How would you > >> > specify the pt_regs location when it's on a different stack? (See the > >> > interrupt macro: non-nested interrupts will place pt_regs on the task > >> > stack before switching to the irq stack.) > >> > >> Hmm. I need to think about the interrupt stack case a bit. Although > >> the actual top of the interrupt stack has a nearly fixed format, and I > >> have old patches to clean it up and make it actually be fixed. I'll > >> try to dust those off and resend them soon. > > > > How would that solve the problem? Would pt_regs be moved or copied to > > the irq stack? > > Hmm. > > Maybe the right way would be to unwind off the IRQ stack in two steps. > Step one would be to figure out that you're on the IRQ stack and pop > your way off it. Step two would be to find pt_regs. > > But that could be rather nasty to implement. Maybe what we actually > want to do is this: > > First, apply this thing: > > https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=x86/entry_ist&id=2231ec7e0bcc1a2bc94a17081511ab54cc6badd1 > > that gives us a common format for the IRQ stack. > > Second, use my idea of making a table of offsets to pt_regs, so we'd > have, roughly: > > ENTER_IRQ_STACK old_rsp=%r11 > call __do_softirq > ANNOTATE_IRQSTACK_PTREGS_CALL offset=0 > LEAVE_IRQ_STACK > > the idea here is that offset=0 means that there is no offset beyond > that implied by ENTER_IRQ_STACK. What actually gets written to the > table is offset 8, because ENTER_IRQ_STACK itself does one push. > > So far, this has no runtime overhead at all. > > Then, in the unwinder, the logic becomes: > > If the return address is annotated in the ptregs entry table, look up > the offset. If the offset is in bounds, then use it. If the offset > is out of bounds and we're on the IRQ stack, then unwind the > ENTER_IRQ_STACK as well. > > Does that seem reasonable? I can try to implement it and see what it > looks like. To be honest I'm not crazy about it. The ANNOTATE_IRQSTACK_PTREGS_CALL macro needs knowledge about the implementation of ENTER_IRQ_STACK and how many pushes it does. I think that makes the entry code more complex and harder to understand than my patch. Also the unwinders would need to be quite a bit more complex, and would need to know more of the intimate details of the irq stack. That's probably a less important consideration than entry code complexity, but it's still significant when you consider the fact that the kernel has many unwinders, both in-kernel and out-of-kernel.
On Fri, 20 May 2016, Andy Lutomirski wrote: > I think it would be negligible, at least for interrupts, since > interrupts are already extremely expensive. But I don't love adding > assembly code that makes them even slower. The real thing I dislike > about this approach is that it's not a normal stack frame, so you need > code in the unwinder to unwind through it correctly, which makes me > think that you're not saving much complexity by adding the pushes. I fail to see what is so special about the stack frame; it's in fact pretty normal. It has added semantic value for "those who know", but the others will (pretty much correctly) consider it to be a stackframe from a function call, and be done with it. What am I missing? Thanks,
On Mon, May 23, 2016 at 4:02 PM, Jiri Kosina <jikos@kernel.org> wrote: > On Fri, 20 May 2016, Andy Lutomirski wrote: > >> I think it would be negligible, at least for interrupts, since >> interrupts are already extremely expensive. But I don't love adding >> assembly code that makes them even slower. The real thing I dislike >> about this approach is that it's not a normal stack frame, so you need >> code in the unwinder to unwind through it correctly, which makes me >> think that you're not saving much complexity by adding the pushes. > > I fail to see what is so special about the stack frame; it's in fact > pretty normal. > > It has added semantic value for "those who know", but the others will > (pretty much correctly) consider it to be a stackframe from a function > call, and be done with it. > > What am I missing? In Josh's code, the stack looks like: ... interrupted frame pt_regs pointer to pt_regs address of pt_regs dummy function rbp handler frame A naive unwinder won't unwind this correctly, as there's no link back to the interrupted frame's RIP. If the layout changed to: ... interrupted frame pt_regs interrupted RIP rbp handler frame then I think it would unwind correctly, but the pt_regs would be invisible, which is IMO a bit unfortunate. It could also be (I think): ... interrupted frame pt_regs interrupted rbp interrupted RIP pointer to pt_regs address of pt_regs dummy function pointer to "interrupted RIP" stack slot handler frame but now this is *five* pushes for the dummy frame, which I think is getting a bit out of hand. --Andy
diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h index 9a9e588..f54886a 100644 --- a/arch/x86/entry/calling.h +++ b/arch/x86/entry/calling.h @@ -201,6 +201,32 @@ For 32-bit we have the following conventions - kernel is built with .byte 0xf1 .endm + /* + * Create a stack frame for the saved pt_regs. This allows frame + * pointer based unwinders to find pt_regs on the stack. + */ + .macro CREATE_PT_REGS_FRAME regs=%rsp +#ifdef CONFIG_FRAME_POINTER + pushq \regs + pushq $pt_regs+1 + pushq %rbp + movq %rsp, %rbp +#endif + .endm + + .macro REMOVE_PT_REGS_FRAME +#ifdef CONFIG_FRAME_POINTER + popq %rbp + addq $0x10, %rsp +#endif + .endm + + .macro CALL_HANDLER handler regs=%rsp + CREATE_PT_REGS_FRAME \regs + call \handler + REMOVE_PT_REGS_FRAME + .endm + #endif /* CONFIG_X86_64 */ /* diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index 9ee0da1..93bc8f0 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -468,7 +468,7 @@ END(irq_entries_start) /* We entered an interrupt context - irqs are off: */ TRACE_IRQS_OFF - call \func /* rdi points to pt_regs */ + CALL_HANDLER \func regs=%rdi .endm /* @@ -495,7 +495,7 @@ ret_from_intr: /* Interrupt came from user space */ GLOBAL(retint_user) mov %rsp,%rdi - call prepare_exit_to_usermode + CALL_HANDLER prepare_exit_to_usermode TRACE_IRQS_IRETQ SWAPGS jmp restore_regs_and_iret @@ -509,7 +509,7 @@ retint_kernel: jnc 1f 0: cmpl $0, PER_CPU_VAR(__preempt_count) jnz 1f - call preempt_schedule_irq + CALL_HANDLER preempt_schedule_irq jmp 0b 1: #endif @@ -688,8 +688,6 @@ ENTRY(\sym) .endif .endif - movq %rsp, %rdi /* pt_regs pointer */ - .if \has_error_code movq ORIG_RAX(%rsp), %rsi /* get error code */ movq $-1, ORIG_RAX(%rsp) /* no syscall to restart */ @@ -701,7 +699,8 @@ ENTRY(\sym) subq $EXCEPTION_STKSZ, CPU_TSS_IST(\shift_ist) .endif - call \do_sym + movq %rsp, %rdi /* pt_regs pointer */ + CALL_HANDLER \do_sym .if \shift_ist != -1 addq $EXCEPTION_STKSZ, CPU_TSS_IST(\shift_ist) @@ -728,8 +727,6 @@ ENTRY(\sym) call sync_regs movq %rax, %rsp /* switch stack */ - movq %rsp, %rdi /* pt_regs pointer */ - .if \has_error_code movq ORIG_RAX(%rsp), %rsi /* get error code */ movq $-1, ORIG_RAX(%rsp) /* no syscall to restart */ @@ -737,7 +734,8 @@ ENTRY(\sym) xorl %esi, %esi /* no error code */ .endif - call \do_sym + movq %rsp, %rdi /* pt_regs pointer */ + CALL_HANDLER \do_sym jmp error_exit /* %ebx: no swapgs flag */ .endif @@ -1174,7 +1172,7 @@ ENTRY(nmi) movq %rsp, %rdi movq $-1, %rsi - call do_nmi + CALL_HANDLER do_nmi /* * Return back to user mode. We must *not* do the normal exit @@ -1387,7 +1385,7 @@ end_repeat_nmi: /* paranoidentry do_nmi, 0; without TRACE_IRQS_OFF */ movq %rsp, %rdi movq $-1, %rsi - call do_nmi + CALL_HANDLER do_nmi testl %ebx, %ebx /* swapgs needed? */ jnz nmi_restore @@ -1423,3 +1421,11 @@ ENTRY(ignore_sysret) mov $-ENOSYS, %eax sysret END(ignore_sysret) + +/* fake function which allows stack unwinders to detect pt_regs frames */ +#ifdef CONFIG_FRAME_POINTER +ENTRY(pt_regs) + nop + nop +ENDPROC(pt_regs) +#endif /* CONFIG_FRAME_POINTER */