Re: [tip: x86/asm] x86/asm/ftrace: Mark function_hook as function

From: Steven Rostedt <rostedt@goodmis.org>
To: Borislav Petkov <bp@suse.de>
Cc: tip-bot2 for Jiri Slaby <tip-bot2@linutronix.de>,
	linux-kernel@vger.kernel.org, linux-tip-commits@vger.kernel.org,
	Jiri Slaby <jslaby@suse.cz>, "H. Peter Anvin" <hpa@zytor.com>,
	Ingo Molnar <mingo@redhat.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	linux-arch@vger.kernel.org,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>, x86-ml <x86@kernel.org>,
	Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@alien8.de>
Subject: Re: [tip: x86/asm] x86/asm/ftrace: Mark function_hook as function
Date: Fri, 18 Oct 2019 13:37:35 -0400	[thread overview]
Message-ID: <20191018133735.77e90e36@gandalf.local.home> (raw)
In-Reply-To: <20191018171354.GB20368@zn.tnic>

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