| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 24 Nov 2014 20:56:56 -0500 From: Steven Rostedt <rostedt@...dmis.org> To: Linus Torvalds <torvalds@...ux-foundation.org> Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>, "H. Peter Anvin" <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>, Masami Hiramatsu <masami.hiramatsu.pt@...achi.com> Subject: Re: [RFC][PATCH 0/7] ftrace/x86: Clean up of mcount.S code On Mon, 24 Nov 2014 17:34:04 -0800 Linus Torvalds <torvalds@...ux-foundation.org> wrote: > On Mon, Nov 24, 2014 at 4:42 PM, Steven Rostedt <rostedt@...dmis.org> wrote: > > > > Let me know if these changes have mcount.S give you less heebie-jeebies. > > So I haven't looked at the individual patches, I just looked at the > rolled-up final patch in this email. > > And yes, from that final patch, I certainly like this much more. At > least it now creates the frame in the obvious place, and the comments > explain the layout. > > However, explain this (in the ftrace_caller_setup macro): > > #ifdef CC_USING_FENTRY > - movq SS+16(%rsp), %rsi > + movq MCOUNT_REG_SIZE+8+\added(%rsp), %rsi > #else > - movq 8(%rbp), %rsi > + /* Need to grab the original %rbp */ > + movq RBP(%rsp), %rsi > + /* Now parent address is 8 above original %rbp */ > + movq 8(%rsi), %rsi > #endif > > Why isn't that "follow rbp" approach now *always* the right thing to > do, regardless of fentry-vs-not? And in particular, couldn't you have > made '%rsi' already contain that old rbp address in save_mcount_regs, > the same way %rdi contains the RIP value? > > (Yes, you can only do that after you've saved the old %rsi, but that's > easy enough to do by just delaying the second > > mov %rsp,%rbp > > until after the save area, and replacing it with > > mov %rbp,%rsi > lea MCOUNT_REG_SIZE - MCOUNT_FRAME_SIZE(%rsp),%rbp > > after the saving of the frame. And now you have that RBP(%rsp) in %rsi > already, so regardless of whether you have CC_USING_FENTRY or not, the > above code becomes just > > /* Now parent address is 8 above original %rbp */ > movq 8(%rsi), %rsi > > No? > > Ok, so I didn't write it all out, and maybe I made some mistake while > writing this email. but it *looks* like your ftrace_caller_setup macro > is just unnecessarily complicated, and again, it's because you have > two different macros and they aren't taking advantage of each other > very well. > > Hmm? > Actually, I just wrote a patch to move the MCOUNT_INSN_SIZE update into save_mcount_regs, and after reading this, I think something like this may work. Note, I didn't even compile test it. But this removes that ftrace_caller_setup completely. (Written on top of this series). I have to look to at how this affects function_graph tracing. But I'm sure we can make that work too. -- Steve diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S index 003b22df1d87..438747ca994f 100644 --- a/arch/x86/kernel/mcount_64.S +++ b/arch/x86/kernel/mcount_64.S @@ -101,9 +101,19 @@ movq MCOUNT_REG_SIZE-8(%rsp), %rdx movq %rdx, RBP(%rsp) + /* Copy the parent address into %rsi (second parameter) */ + movq 8(%rdx), %rsi + /* Move RIP to its proper location */ movq MCOUNT_REG_SIZE+\added(%rsp), %rdi movq %rdi, RIP(%rsp) + + /* + * Now %rdi (the first parameter) has the return address of + * where ftrace_call returns. But the callbacks expect the + * the address of the call itself. + */ + subq $MCOUNT_INSN_SIZE, %rdi .endm .macro restore_mcount_regs @@ -122,28 +132,6 @@ .endm -/* skip is set if stack has been adjusted */ -.macro ftrace_caller_setup trace_label added=0 - save_mcount_regs \added - - /* Save this location */ -GLOBAL(\trace_label) - /* Load the ftrace_ops into the 3rd parameter */ - movq function_trace_op(%rip), %rdx - - /* %rdi already has %rip from the save_mcount_regs macro */ - subq $MCOUNT_INSN_SIZE, %rdi - /* Load the parent_ip into the second parameter */ -#ifdef CC_USING_FENTRY - movq MCOUNT_REG_SIZE+8+\added(%rsp), %rsi -#else - /* Need to grab the original %rbp */ - movq RBP(%rsp), %rsi - /* Now parent address is 8 above original %rbp */ - movq 8(%rsi), %rsi -#endif -.endm - #ifdef CONFIG_DYNAMIC_FTRACE ENTRY(function_hook) @@ -151,7 +139,13 @@ ENTRY(function_hook) END(function_hook) ENTRY(ftrace_caller) - ftrace_caller_setup ftrace_caller_op_ptr + /* save_mcount_regs fills in first two parameters */ + save_mcount_regs + +GLOBAL(ftrace_caller_op_ptr) + /* Load the ftrace_ops into the 3rd parameter */ + movq function_trace_op(%rip), %rdx + /* regs go into 4th parameter (but make it NULL) */ movq $0, %rcx @@ -181,8 +175,12 @@ ENTRY(ftrace_regs_caller) /* Save the current flags before any operations that can change them */ pushfq - /* added 8 bytes to save flags */ - ftrace_caller_setup ftrace_regs_caller_op_ptr 8 + /* save_mcount_regs fills in first two parameters */ + save_mcount_regs 8 + +GLOBAL(ftrace_regs_caller_op_ptr) + /* Load the ftrace_ops into the 3rd parameter */ + movq function_trace_op(%rip), %rdx /* Save the rest of pt_regs */ movq %r15, R15(%rsp) @@ -263,7 +261,8 @@ GLOBAL(ftrace_stub) retq trace: - ftrace_caller_setup ftrace_caller_op_ptr + /* save_mcount_regs fills in first two parameters */ + save_mcount_regs call *ftrace_trace_function -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists