[tip:,x86/asm] x86/asm/ftrace: Mark function_hook as function
diff mbox series

Message ID 157141622788.29376.4016565749507481510.tip-bot2@tip-bot2
State In Next
Commit f13ad88a984e8090226a8f62d75e87b770eefdf4
Headers show
Series
  • [tip:,x86/asm] x86/asm/ftrace: Mark function_hook as function
Related show

Commit Message

tip-bot2 for Xiaochen Shen Oct. 18, 2019, 4:30 p.m. UTC
The following commit has been merged into the x86/asm branch of tip:

Commit-ID:     f13ad88a984e8090226a8f62d75e87b770eefdf4
Gitweb:        https://git.kernel.org/tip/f13ad88a984e8090226a8f62d75e87b770eefdf4
Author:        Jiri Slaby <jslaby@suse.cz>
AuthorDate:    Fri, 11 Oct 2019 13:51:01 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 18 Oct 2019 11:35:41 +02:00

x86/asm/ftrace: Mark function_hook as function

Relabel function_hook to be marked really as a function. It is called
from C and has the same expectations towards the stack etc.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: linux-arch@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20191011115108.12392-22-jslaby@suse.cz
---
 arch/x86/kernel/ftrace_32.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Steven Rostedt Oct. 18, 2019, 4:48 p.m. UTC | #1
On Fri, 18 Oct 2019 16:30:27 -0000
"tip-bot2 for Jiri Slaby" <tip-bot2@linutronix.de> wrote:

> The following commit has been merged into the x86/asm branch of tip:
> 
> Commit-ID:     f13ad88a984e8090226a8f62d75e87b770eefdf4
> Gitweb:        https://git.kernel.org/tip/f13ad88a984e8090226a8f62d75e87b770eefdf4
> Author:        Jiri Slaby <jslaby@suse.cz>
> AuthorDate:    Fri, 11 Oct 2019 13:51:01 +02:00
> Committer:     Borislav Petkov <bp@suse.de>
> CommitterDate: Fri, 18 Oct 2019 11:35:41 +02:00

I just noticed this (sorry missed the original patch).

> 
> x86/asm/ftrace: Mark function_hook as function
> 
> Relabel function_hook to be marked really as a function. It is called
> from C and has the same expectations towards the stack etc.

This is wrong, function_hook is never called from C. It's called via
fentry (use to be mcount), and does not have the same semantics as a C
function. In fact, that's why it exists in assembly. Because it has to
save and restore registers to make it possible to call a C function!

-- Steve


> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: linux-arch@vger.kernel.org
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: x86-ml <x86@kernel.org>
> Link: https://lkml.kernel.org/r/20191011115108.12392-22-jslaby@suse.cz
> ---
>  arch/x86/kernel/ftrace_32.S | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
> index e0061dc..219be13 100644
> --- a/arch/x86/kernel/ftrace_32.S
> +++ b/arch/x86/kernel/ftrace_32.S
> @@ -21,9 +21,9 @@ EXPORT_SYMBOL(__fentry__)
>  # define MCOUNT_FRAME			0	/* using frame = false */
>  #endif
>  
> -ENTRY(function_hook)
> +SYM_FUNC_START(function_hook)
>  	ret
> -END(function_hook)
> +SYM_FUNC_END(function_hook)
>  
>  ENTRY(ftrace_caller)
>
Steven Rostedt Oct. 18, 2019, 4:49 p.m. UTC | #2
On Fri, 18 Oct 2019 12:48:00 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > Relabel function_hook to be marked really as a function. It is called
> > from C and has the same expectations towards the stack etc.  
> 

And to go even further, it *does not* have the same expectations
towards the stack.

I think this patch should not be applied.

-- Steve
Borislav Petkov Oct. 18, 2019, 5:13 p.m. UTC | #3
On Fri, Oct 18, 2019 at 12:49:56PM -0400, Steven Rostedt wrote:
> On Fri, 18 Oct 2019 12:48:00 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > Relabel function_hook to be marked really as a function. It is called
> > > from C and has the same expectations towards the stack etc.  
> > 
> 
> And to go even further, it *does not* have the same expectations
> towards the stack.
> 
> I think this patch should not be applied.

There are a couple more markings like that now:

$ git grep function_hook
Documentation/asm-annotations.rst:120:    SYM_FUNC_START(function_hook)
Documentation/asm-annotations.rst:122:    SYM_FUNC_END(function_hook)
arch/x86/kernel/ftrace_32.S:15:# define function_hook   __fentry__
arch/x86/kernel/ftrace_32.S:24:SYM_FUNC_START(function_hook)
arch/x86/kernel/ftrace_32.S:26:SYM_FUNC_END(function_hook)
arch/x86/kernel/ftrace_64.S:17:# define function_hook   __fentry__
arch/x86/kernel/ftrace_64.S:135:SYM_FUNC_START(function_hook)
arch/x86/kernel/ftrace_64.S:137:SYM_FUNC_END(function_hook)
arch/x86/kernel/ftrace_64.S:251:SYM_FUNC_START(function_hook)
arch/x86/kernel/ftrace_64.S:282:SYM_FUNC_END(function_hook)

Frankly, I wouldn't mark this function at all as it is special and I see
a little sense to have it in stack traces but maybe Jiri has another
angle here. I'll let him comment.

I guess with the new nomenclature that can be SYM_CODE_* now...

Then, this magic "function" or a global symbol with an address or
whatever that is (oh, there's #define trickery too) definitely deserves
a comment above it to explain what it is. I even have to build the .s
file to see what it turns into:

.globl __fentry__ ; .p2align 4, 0x90 ; __fentry__:
 retq
.type __fentry__, @function ; .size __fentry__, .-__fentry__

Yeah, it is called on every function entry:

callq  ffffffff81a01760 <__fentry__>

but can we please explain with a comment above it what it is?

Thx.
Steven Rostedt Oct. 18, 2019, 5:37 p.m. UTC | #4
On Fri, 18 Oct 2019 19:13:54 +0200
Borislav Petkov <bp@suse.de> wrote:

> On Fri, Oct 18, 2019 at 12:49:56PM -0400, Steven Rostedt wrote:
> > On Fri, 18 Oct 2019 12:48:00 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >   
> > > > Relabel function_hook to be marked really as a function. It is called
> > > > from C and has the same expectations towards the stack etc.    
> > >   
> > 
> > And to go even further, it *does not* have the same expectations
> > towards the stack.
> > 
> > I think this patch should not be applied.  
> 
> There are a couple more markings like that now:
> 
> $ git grep function_hook
> Documentation/asm-annotations.rst:120:    SYM_FUNC_START(function_hook)
> Documentation/asm-annotations.rst:122:    SYM_FUNC_END(function_hook)
> arch/x86/kernel/ftrace_32.S:15:# define function_hook   __fentry__
> arch/x86/kernel/ftrace_32.S:24:SYM_FUNC_START(function_hook)
> arch/x86/kernel/ftrace_32.S:26:SYM_FUNC_END(function_hook)
> arch/x86/kernel/ftrace_64.S:17:# define function_hook   __fentry__
> arch/x86/kernel/ftrace_64.S:135:SYM_FUNC_START(function_hook)
> arch/x86/kernel/ftrace_64.S:137:SYM_FUNC_END(function_hook)
> arch/x86/kernel/ftrace_64.S:251:SYM_FUNC_START(function_hook)
> arch/x86/kernel/ftrace_64.S:282:SYM_FUNC_END(function_hook)
> 
> Frankly, I wouldn't mark this function at all as it is special and I see
> a little sense to have it in stack traces but maybe Jiri has another
> angle here. I'll let him comment.

It just needs to be visible by modules and what not, otherwise linking
will fail.

> 
> I guess with the new nomenclature that can be SYM_CODE_* now...
> 
> Then, this magic "function" or a global symbol with an address or
> whatever that is (oh, there's #define trickery too) definitely deserves
> a comment above it to explain what it is. I even have to build the .s
> file to see what it turns into:

The #define was because we use to support mcount or __fentry__, now we
just support __fentry__, and function_hook describes it better ;-)

> 
> .globl __fentry__ ; .p2align 4, 0x90 ; __fentry__:
>  retq
> .type __fentry__, @function ; .size __fentry__, .-__fentry__
> 
> Yeah, it is called on every function entry:
> 
> callq  ffffffff81a01760 <__fentry__>
> 
> but can we please explain with a comment above it what it is?

Heh, I guess we could, which would probably be quite a long comment as
it's the key behind ftrace itself.

-- Steve
Borislav Petkov Oct. 18, 2019, 7:48 p.m. UTC | #5
On Fri, Oct 18, 2019 at 01:37:35PM -0400, Steven Rostedt wrote:
> It just needs to be visible by modules and what not, otherwise linking
> will fail.

And I assume all of them?

> The #define was because we use to support mcount or __fentry__, now we
> just support __fentry__, and function_hook describes it better ;-)

Well sorry but gcc documentation talks about __fentry__. I'd keep the
same name to avoid confusion and explain above it what it is. Much
better.

> Heh, I guess we could, which would probably be quite a long comment as
> it's the key behind ftrace itself.

Well, you can explain with a couple of sentences what it is and
then point at the bigger document explaining ftrace. Provided, Mr.
Rostedt, you'll stop doing talks and finally sit down and write that
documentation!

:-P
Steven Rostedt Oct. 18, 2019, 8:31 p.m. UTC | #6
On Fri, 18 Oct 2019 21:48:56 +0200
Borislav Petkov <bp@suse.de> wrote:

> > The #define was because we use to support mcount or __fentry__, now we
> > just support __fentry__, and function_hook describes it better ;-)  
> 
> Well sorry but gcc documentation talks about __fentry__. I'd keep the
> same name to avoid confusion and explain above it what it is. Much
> better.

Still looks ugly ;-)

> 
> > Heh, I guess we could, which would probably be quite a long comment as
> > it's the key behind ftrace itself.  
> 
> Well, you can explain with a couple of sentences what it is and
> then point at the bigger document explaining ftrace. Provided, Mr.
> Rostedt, you'll stop doing talks and finally sit down and write that
> documentation!

I do the talks hoping someone else will finally sit down and write the
documentation!

-- Steve
Borislav Petkov Oct. 19, 2019, 7:34 a.m. UTC | #7
On Fri, Oct 18, 2019 at 04:31:25PM -0400, Steven Rostedt wrote:
> Still looks ugly ;-)

See below. I think it's not so bad. It is only built-tested on 64-bit and
objtool complains about something again:

arch/x86/kernel/ftrace_64.o: warning: objtool: .entry.text+0x0: unreachable instruction

but I think it is the better thing to do.

> I do the talks hoping someone else will finally sit down and write the
> documentation!

Well, you can put down the outline of the doc and flesh out section by
section gradually. Besides, you have all the text in your slides so it
is actually more or less a copy+paste.

---
diff --git a/Documentation/asm-annotations.rst b/Documentation/asm-annotations.rst
index 29ccd6e61fe5..f55c2bb74d00 100644
--- a/Documentation/asm-annotations.rst
+++ b/Documentation/asm-annotations.rst
@@ -117,9 +117,9 @@ This section covers ``SYM_FUNC_*`` and ``SYM_CODE_*`` enumerated above.
   So in most cases, developers should write something like in the following
   example, having some asm instructions in between the macros, of course::
 
-    SYM_FUNC_START(function_hook)
+    SYM_FUNC_START(memset)
         ... asm insns ...
-    SYM_FUNC_END(function_hook)
+    SYM_FUNC_END(memset)
 
   In fact, this kind of annotation corresponds to the now deprecated ``ENTRY``
   and ``ENDPROC`` macros.
diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 8ed1f5d371f0..77be7e7e5e59 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -12,18 +12,16 @@
 #include <asm/frame.h>
 #include <asm/asm-offsets.h>
 
-# define function_hook	__fentry__
-EXPORT_SYMBOL(__fentry__)
-
 #ifdef CONFIG_FRAME_POINTER
 # define MCOUNT_FRAME			1	/* using frame = true  */
 #else
 # define MCOUNT_FRAME			0	/* using frame = false */
 #endif
 
-SYM_FUNC_START(function_hook)
+SYM_CODE_START(__fentry__)
 	ret
-SYM_FUNC_END(function_hook)
+SYM_CODE_END(__fentry__)
+EXPORT_SYMBOL(__fentry__)
 
 SYM_CODE_START(ftrace_caller)
 
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 69c8d1b9119e..3029fe4f8547 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -14,9 +14,6 @@
 	.code64
 	.section .entry.text, "ax"
 
-# define function_hook	__fentry__
-EXPORT_SYMBOL(__fentry__)
-
 #ifdef CONFIG_FRAME_POINTER
 /* Save parent and function stack frames (rip and rbp) */
 #  define MCOUNT_FRAME_SIZE	(8+16*2)
@@ -132,9 +129,10 @@ EXPORT_SYMBOL(__fentry__)
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
-SYM_FUNC_START(function_hook)
+SYM_CODE_START(__fentry__)
 	retq
-SYM_FUNC_END(function_hook)
+SYM_CODE_END(__fentry__)
+EXPORT_SYMBOL(__fentry__)
 
 SYM_FUNC_START(ftrace_caller)
 	/* save_mcount_regs fills in first two parameters */
@@ -248,7 +246,7 @@ SYM_FUNC_END(ftrace_regs_caller)
 
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
-SYM_FUNC_START(function_hook)
+SYM_CODE_START(__fentry__)
 	cmpq $ftrace_stub, ftrace_trace_function
 	jnz trace
 
@@ -279,7 +277,8 @@ trace:
 	restore_mcount_regs
 
 	jmp fgraph_trace
-SYM_FUNC_END(function_hook)
+SYM_CODE_END(__fentry__)
+EXPORT_SYMBOL(__fentry__)
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER

Patch
diff mbox series

diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index e0061dc..219be13 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -21,9 +21,9 @@  EXPORT_SYMBOL(__fentry__)
 # define MCOUNT_FRAME			0	/* using frame = false */
 #endif
 
-ENTRY(function_hook)
+SYM_FUNC_START(function_hook)
 	ret
-END(function_hook)
+SYM_FUNC_END(function_hook)
 
 ENTRY(ftrace_caller)