Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1426098 > unrolled thread

Re: [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc  from using short jumps to it Namhyung Kim <namhyung@kernel.org> - 2016-06-20 03:20 +0200
    Re: [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc  from using short jumps to it Steven Rostedt <rostedt@goodmis.org> - 2016-06-20 15:50 +0200

#1426098 — Re: [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it

Hi Steve,

On Sun, May 22, 2016 at 04:28:49PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> 
> Matt Fleming reported seeing crashes when enabling and disabling
> function profiling which uses function graph tracer. Later Namhyung Kim
> hit a similar issue and he found that the issue was due to the jmp to
> ftrace_stub in ftrace_graph_call was only two bytes, and when it was
> changed to jump to the tracing code, it overwrote the ftrace_stub that
> was after it.
> 
> Masami Hiramatsu bisected this down to a binutils change:
> 
> 8dcea93252a9ea7dff57e85220a719e2a5e8ab41 is the first bad commit
> commit 8dcea93252a9ea7dff57e85220a719e2a5e8ab41
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date:   Fri May 15 03:17:31 2015 -0700
> 
>     Add -mshared option to x86 ELF assembler
> 
>     This patch adds -mshared option to x86 ELF assembler.  By default,
>     assembler will optimize out non-PLT relocations against defined non-weak
>     global branch targets with default visibility.  The -mshared option tells
>     the assembler to generate code which may go into a shared library
>     where all non-weak global branch targets with default visibility can
>     be preempted.  The resulting code is slightly bigger.  This option
>     only affects the handling of branch instructions.
> 
> Declaring ftrace_stub as a weak call prevents gas from using two byte
> jumps to it, which would be converted to a jump to the function graph
> code.
> 
> Link: http://lkml.kernel.org/r/20160516230035.1dbae571@gandalf.local.home

Shouldn't it go to the stable tree?

Thanks,
Namhyung


> 
> Reported-by: Matt Fleming <matt@codeblueprint.co.uk>
> Reported-by: Namhyung Kim <namhyung@kernel.org>
> Tested-by: Matt Fleming <matt@codeblueprint.co.uk>
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  arch/x86/kernel/mcount_64.S | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
> index ed48a9f465f8..61924222a9e1 100644
> --- a/arch/x86/kernel/mcount_64.S
> +++ b/arch/x86/kernel/mcount_64.S
> @@ -182,7 +182,8 @@ GLOBAL(ftrace_graph_call)
>  	jmp ftrace_stub
>  #endif
>  
> -GLOBAL(ftrace_stub)
> +/* This is weak to keep gas from relaxing the jumps */
> +WEAK(ftrace_stub)
>  	retq
>  END(ftrace_caller)
>  
> -- 
> 2.8.0.rc3
> 
> 

[toc] | [next] | [standalone]

#1426619

On Mon, 20 Jun 2016 09:46:02 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> Hi Steve,
> 
> On Sun, May 22, 2016 at 04:28:49PM -0400, Steven Rostedt wrote:
> > From: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Matt Fleming reported seeing crashes when enabling and disabling
> > function profiling which uses function graph tracer. Later Namhyung Kim
> > hit a similar issue and he found that the issue was due to the jmp to
> > ftrace_stub in ftrace_graph_call was only two bytes, and when it was
> > changed to jump to the tracing code, it overwrote the ftrace_stub that
> > was after it.
> > 
> > Masami Hiramatsu bisected this down to a binutils change:
> > 
> > 8dcea93252a9ea7dff57e85220a719e2a5e8ab41 is the first bad commit
> > commit 8dcea93252a9ea7dff57e85220a719e2a5e8ab41
> > Author: H.J. Lu <hjl.tools@gmail.com>
> > Date:   Fri May 15 03:17:31 2015 -0700
> > 
> >     Add -mshared option to x86 ELF assembler
> > 
> >     This patch adds -mshared option to x86 ELF assembler.  By default,
> >     assembler will optimize out non-PLT relocations against defined non-weak
> >     global branch targets with default visibility.  The -mshared option tells
> >     the assembler to generate code which may go into a shared library
> >     where all non-weak global branch targets with default visibility can
> >     be preempted.  The resulting code is slightly bigger.  This option
> >     only affects the handling of branch instructions.
> > 
> > Declaring ftrace_stub as a weak call prevents gas from using two byte
> > jumps to it, which would be converted to a jump to the function graph
> > code.
> > 
> > Link: http://lkml.kernel.org/r/20160516230035.1dbae571@gandalf.local.home  
> 
> Shouldn't it go to the stable tree?

I had this discussion with Matt too. If you want to request this for
stable, feel free to do so. It's an issue with new compilers. My
thought was, if you are using an older kernel, you are probably using
an older compiler too. ;-)

-- Steve


> 
> 
> > 
> > Reported-by: Matt Fleming <matt@codeblueprint.co.uk>
> > Reported-by: Namhyung Kim <namhyung@kernel.org>
> > Tested-by: Matt Fleming <matt@codeblueprint.co.uk>
> > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> >  arch/x86/kernel/mcount_64.S | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
> > index ed48a9f465f8..61924222a9e1 100644
> > --- a/arch/x86/kernel/mcount_64.S
> > +++ b/arch/x86/kernel/mcount_64.S
> > @@ -182,7 +182,8 @@ GLOBAL(ftrace_graph_call)
> >  	jmp ftrace_stub
> >  #endif
> >  
> > -GLOBAL(ftrace_stub)
> > +/* This is weak to keep gas from relaxing the jumps */
> > +WEAK(ftrace_stub)
> >  	retq
> >  END(ftrace_caller)
> >  
> > -- 
> > 2.8.0.rc3
> > 
> >   

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web