git pull request for tip/tracing/urgent

git pull request for tip/tracing/urgent

[Steven Rostedt]

Ingo,

The bug that is fixed by this change can affect users. Most likely it will 
not, since the fault should never happen. But this is a protective 
mechanism, where if it does, that means there is a bug in the tracer.

As you have previously told me, a bug in the tracer should never crash the 
kernel. Since the detection of a fault in the function graph tracer can 
lead to a kernel crash (without this change) I think this qualifies as 
something for 29.

-- Steve


The following patch is in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: tip/tracing/urgent


Steven Rostedt (1):
      tracing, x86: fix fixup section to return to original code

----
 arch/x86/kernel/ftrace.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
---------------------------
commit e3944bfac961cd7fc82f3b3143c55dc375748569
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Tue Feb 10 13:07:13 2009 -0500

    tracing, x86: fix fixup section to return to original code
    
    Impact: fix to prevent a kernel crash on fault
    
    If for some reason the pointer to the parent function on the
    stack takes a fault, the fix up code will not return back to
    the original faulting code. This can lead to unpredictable
    results and perhaps even a kernel panic.
    
    A fault should not happen, but if it does, we should simply
    disable the tracer, warn, and continue running the kernel.
    It should not lead to a kernel crash.
    
    Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 1b43086..9d549e4 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -491,13 +491,15 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 		"1: " _ASM_MOV " (%[parent_old]), %[old]\n"
 		"2: " _ASM_MOV " %[return_hooker], (%[parent_replaced])\n"
 		"   movl $0, %[faulted]\n"
+		"3:\n"
 
 		".section .fixup, \"ax\"\n"
-		"3: movl $1, %[faulted]\n"
+		"4: movl $1, %[faulted]\n"
+		"   jmp 3b\n"
 		".previous\n"
 
-		_ASM_EXTABLE(1b, 3b)
-		_ASM_EXTABLE(2b, 3b)
+		_ASM_EXTABLE(1b, 4b)
+		_ASM_EXTABLE(2b, 4b)
 
 		: [parent_replaced] "=r" (parent), [old] "=r" (old),
 		  [faulted] "=r" (faulted)

Re: git pull request for tip/tracing/urgent

[Frederic Weisbecker]

On Tue, Feb 10, 2009 at 01:19:31PM -0500, Steven Rostedt wrote:
> 
> Ingo,
> 
> The bug that is fixed by this change can affect users. Most likely it will 
> not, since the fault should never happen. But this is a protective 
> mechanism, where if it does, that means there is a bug in the tracer.
> 
> As you have previously told me, a bug in the tracer should never crash the 
> kernel. Since the detection of a fault in the function graph tracer can 
> lead to a kernel crash (without this change) I think this qualifies as 
> something for 29.
> 
> -- Steve
> 
> 
> The following patch is in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: tip/tracing/urgent
> 
> 
> Steven Rostedt (1):
>       tracing, x86: fix fixup section to return to original code
> 
> ----
>  arch/x86/kernel/ftrace.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> ---------------------------
> commit e3944bfac961cd7fc82f3b3143c55dc375748569
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Tue Feb 10 13:07:13 2009 -0500
> 
>     tracing, x86: fix fixup section to return to original code
>     
>     Impact: fix to prevent a kernel crash on fault
>     
>     If for some reason the pointer to the parent function on the
>     stack takes a fault, the fix up code will not return back to
>     the original faulting code. This can lead to unpredictable
>     results and perhaps even a kernel panic.
>     
>     A fault should not happen, but if it does, we should simply
>     disable the tracer, warn, and continue running the kernel.
>     It should not lead to a kernel crash.
>     
>     Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 1b43086..9d549e4 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -491,13 +491,15 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
>  		"1: " _ASM_MOV " (%[parent_old]), %[old]\n"
>  		"2: " _ASM_MOV " %[return_hooker], (%[parent_replaced])\n"
>  		"   movl $0, %[faulted]\n"
> +		"3:\n"
>  
>  		".section .fixup, \"ax\"\n"
> -		"3: movl $1, %[faulted]\n"
> +		"4: movl $1, %[faulted]\n"
> +		"   jmp 3b\n"
>  		".previous\n"


It thought after the fixup section, the code would continue to rest of the C code.
Where would it go without the jmp?

Thanks.


> -		_ASM_EXTABLE(1b, 3b)
> -		_ASM_EXTABLE(2b, 3b)
> +		_ASM_EXTABLE(1b, 4b)
> +		_ASM_EXTABLE(2b, 4b)
>  
>  		: [parent_replaced] "=r" (parent), [old] "=r" (old),
>  		  [faulted] "=r" (faulted)
> 

Re: git pull request for tip/tracing/urgent

[Steven Rostedt]

On Tue, 10 Feb 2009, Frederic Weisbecker wrote:
> > 
> > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > index 1b43086..9d549e4 100644
> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > @@ -491,13 +491,15 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
> >  		"1: " _ASM_MOV " (%[parent_old]), %[old]\n"
> >  		"2: " _ASM_MOV " %[return_hooker], (%[parent_replaced])\n"
> >  		"   movl $0, %[faulted]\n"
> > +		"3:\n"
> >  
> >  		".section .fixup, \"ax\"\n"
> > -		"3: movl $1, %[faulted]\n"
> > +		"4: movl $1, %[faulted]\n"
> > +		"   jmp 3b\n"
> >  		".previous\n"
> 
> 
> It thought after the fixup section, the code would continue to rest of the C code.
> Where would it go without the jmp?

To the next item the linker placed into the .fixup section.  And that 
would jump back to the location for that fixup. Basically, what you have 
is this:

(just picking random and factitious registers)

.section .text
[...]
L1:	mov	%a, %b
L2:	cmp	%x, $1
<continue code>


<Someplace else>

.section .text
[...]
L3:	mov	%c, %d
L4:	cmp	%x, $22
[...]

.section .fixup
[...]
L5:	mov	$1, %x
	jmp L2
L6:	mov	$22, %x
	jmp L4
[...]


.section __ex_table
[...]
.long	L1, L5
.long	L3, L6
[...]


So when we take an exception at label L1, the page fault code will look 
to see if it is OK, by doing a binary search of the exception table.
When it finds the L1, L5 pair, it will then set up a return to the L5 
label.

When the fault returns to L5, it loads that reg %x with $1 and jumps back 
to L2, where it can see that it took a fault.

Now lets look at what happens when we do not have that jump back to L2. 
Instead of going back to the original code, it will load $22 into %x and 
jmp back to the wrong area. God knows what will happen then, since the 
stack pointer thinks it is from where the original fault occurred.

-- Steve

Re: git pull request for tip/tracing/urgent

[Frederic Weisbecker]

On Tue, Feb 10, 2009 at 06:00:14PM -0500, Steven Rostedt wrote:
> 
> 
> On Tue, 10 Feb 2009, Frederic Weisbecker wrote:
> > > 
> > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > > index 1b43086..9d549e4 100644
> > > --- a/arch/x86/kernel/ftrace.c
> > > +++ b/arch/x86/kernel/ftrace.c
> > > @@ -491,13 +491,15 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
> > >  		"1: " _ASM_MOV " (%[parent_old]), %[old]\n"
> > >  		"2: " _ASM_MOV " %[return_hooker], (%[parent_replaced])\n"
> > >  		"   movl $0, %[faulted]\n"
> > > +		"3:\n"
> > >  
> > >  		".section .fixup, \"ax\"\n"
> > > -		"3: movl $1, %[faulted]\n"
> > > +		"4: movl $1, %[faulted]\n"
> > > +		"   jmp 3b\n"
> > >  		".previous\n"
> > 
> > 
> > It thought after the fixup section, the code would continue to rest of the C code.
> > Where would it go without the jmp?
> 
> To the next item the linker placed into the .fixup section.  And that 
> would jump back to the location for that fixup. Basically, what you have 
> is this:
> 
> (just picking random and factitious registers)
> 
> .section .text
> [...]
> L1:	mov	%a, %b
> L2:	cmp	%x, $1
> <continue code>
> 
> 
> <Someplace else>
> 
> .section .text
> [...]
> L3:	mov	%c, %d
> L4:	cmp	%x, $22
> [...]
> 
> .section .fixup
> [...]
> L5:	mov	$1, %x
> 	jmp L2
> L6:	mov	$22, %x
> 	jmp L4
> [...]
> 
> 
> .section __ex_table
> [...]
> .long	L1, L5
> .long	L3, L6
> [...]
> 
> 
> So when we take an exception at label L1, the page fault code will look 
> to see if it is OK, by doing a binary search of the exception table.
> When it finds the L1, L5 pair, it will then set up a return to the L5 
> label.
> 
> When the fault returns to L5, it loads that reg %x with $1 and jumps back 
> to L2, where it can see that it took a fault.
> 
> Now lets look at what happens when we do not have that jump back to L2. 
> Instead of going back to the original code, it will load $22 into %x and 
> jmp back to the wrong area. God knows what will happen then, since the 
> stack pointer thinks it is from where the original fault occurred.


Heh, that's fairly logic. Don't ask me why, but I did not imagine each
part of .fixup unified in a separate contiguous section (but what else can it be?...).

Thanks for your explanations :-)

 
> -- Steve
> 

Re: git pull request for tip/tracing/urgent

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Ingo,
> 
> The bug that is fixed by this change can affect users. Most likely it will 
> not, since the fault should never happen. But this is a protective 
> mechanism, where if it does, that means there is a bug in the tracer.
> 
> As you have previously told me, a bug in the tracer should never crash the 
> kernel. Since the detection of a fault in the function graph tracer can 
> lead to a kernel crash (without this change) I think this qualifies as 
> something for 29.

Yeah, definitely so.

> -- Steve
> 
> 
> The following patch is in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: tip/tracing/urgent
> 
> 
> Steven Rostedt (1):
>       tracing, x86: fix fixup section to return to original code
> 
> ----
>  arch/x86/kernel/ftrace.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)

Pulled into tip:tracing/urgent, thanks Steve!

	Ingo

Re: git pull request for tip/tracing/urgent

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> > > 
> > > 
> > > It thought after the fixup section, the code would continue to rest of the C code.
> > > Where would it go without the jmp?
> > 
> > To the next item the linker placed into the .fixup section.  And that 
> > would jump back to the location for that fixup. Basically, what you have 
> > is this:
> > 
> > (just picking random and factitious registers)
> > 
> > .section .text
> > [...]
> > L1:	mov	%a, %b
> > L2:	cmp	%x, $1
> > <continue code>
> > 
> > 
> > <Someplace else>
> > 
> > .section .text
> > [...]
> > L3:	mov	%c, %d
> > L4:	cmp	%x, $22
> > [...]
> > 
> > .section .fixup
> > [...]
> > L5:	mov	$1, %x
> > 	jmp L2
> > L6:	mov	$22, %x
> > 	jmp L4
> > [...]
> > 
> > 
> > .section __ex_table
> > [...]
> > .long	L1, L5
> > .long	L3, L6
> > [...]
> > 
> > 
> > So when we take an exception at label L1, the page fault code will look 
> > to see if it is OK, by doing a binary search of the exception table.
> > When it finds the L1, L5 pair, it will then set up a return to the L5 
> > label.
> > 
> > When the fault returns to L5, it loads that reg %x with $1 and jumps back 
> > to L2, where it can see that it took a fault.
> > 
> > Now lets look at what happens when we do not have that jump back to L2. 
> > Instead of going back to the original code, it will load $22 into %x and 
> > jmp back to the wrong area. God knows what will happen then, since the 
> > stack pointer thinks it is from where the original fault occurred.
> 
> 
> Heh, that's fairly logic. Don't ask me why, but I did not imagine each
> part of .fixup unified in a separate contiguous section (but what else can it be?...).
> 
> Thanks for your explanations :-)

This bit:

  	".section .fixup, \"ax\"\n"
 	"4: movl $1, %[faulted]\n"
 	"   jmp 3b\n"
 	".previous\n"

Can be thought of as an 'embedded' or 'nested' section - the '.previous'
directive jumps back to whatever section we were in before. This can be
nested multiple times too:

   .section A
   [...]
     .section B
     [...]
        .section C
        [...]
        .previous
     [...]
     .previous
   [...]
   .previous

For whatever reason the interaction of the assembler with the linker and
in particular linker scripts are one of the most undocumented areas of OSS.
Does anyone know any good reference to start with? 

Something that explains the principles, how it all works, what the various
section flags mean in practice, including details like dwarf2/CFI annotations.

I do not know about any coherent documentation in this area and as a result
many developers shy away from this area, frequently mess it up if they have to
touch it and generally treat it as voodoo.

	Ingo

Re: git pull request for tip/tracing/urgent

[Tejun Heo]

Hello,

Ingo Molnar wrote:
> This bit:
> 
>   	".section .fixup, \"ax\"\n"
>  	"4: movl $1, %[faulted]\n"
>  	"   jmp 3b\n"
>  	".previous\n"
> 
> Can be thought of as an 'embedded' or 'nested' section - the '.previous'
> directive jumps back to whatever section we were in before. This can be
> nested multiple times too:
> 
>    .section A
>    [...]
>      .section B
>      [...]
>         .section C
>         [...]
>         .previous
>      [...]
>      .previous
>    [...]
>    .previous

That will result in...

 A->B->C->B->C->B

.pushsection and .popsection should be used for nesting, right?

Thanks.

-- 
tejun

Re: git pull request for tip/tracing/urgent

[Ingo Molnar]

* Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> Ingo Molnar wrote:
> > This bit:
> > 
> >   	".section .fixup, \"ax\"\n"
> >  	"4: movl $1, %[faulted]\n"
> >  	"   jmp 3b\n"
> >  	".previous\n"
> > 
> > Can be thought of as an 'embedded' or 'nested' section - the '.previous'
> > directive jumps back to whatever section we were in before. This can be
> > nested multiple times too:
> > 
> >    .section A
> >    [...]
> >      .section B
> >      [...]
> >         .section C
> >         [...]
> >         .previous
> >      [...]
> >      .previous
> >    [...]
> >    .previous
> 
> That will result in...
> 
>  A->B->C->B->C->B
> 
> .pushsection and .popsection should be used for nesting, right?
> 
> Thanks.

hm ... doh, yes. I think we even had a bug in this area.

Why is it implemented like this? These semantics seem to make no sense.

	Ingo

Re: git pull request for tip/tracing/urgent

[Frederic Weisbecker]

On Wed, Feb 11, 2009 at 10:02:29AM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > > > 
> > > > 
> > > > It thought after the fixup section, the code would continue to rest of the C code.
> > > > Where would it go without the jmp?
> > > 
> > > To the next item the linker placed into the .fixup section.  And that 
> > > would jump back to the location for that fixup. Basically, what you have 
> > > is this:
> > > 
> > > (just picking random and factitious registers)
> > > 
> > > .section .text
> > > [...]
> > > L1:	mov	%a, %b
> > > L2:	cmp	%x, $1
> > > <continue code>
> > > 
> > > 
> > > <Someplace else>
> > > 
> > > .section .text
> > > [...]
> > > L3:	mov	%c, %d
> > > L4:	cmp	%x, $22
> > > [...]
> > > 
> > > .section .fixup
> > > [...]
> > > L5:	mov	$1, %x
> > > 	jmp L2
> > > L6:	mov	$22, %x
> > > 	jmp L4
> > > [...]
> > > 
> > > 
> > > .section __ex_table
> > > [...]
> > > .long	L1, L5
> > > .long	L3, L6
> > > [...]
> > > 
> > > 
> > > So when we take an exception at label L1, the page fault code will look 
> > > to see if it is OK, by doing a binary search of the exception table.
> > > When it finds the L1, L5 pair, it will then set up a return to the L5 
> > > label.
> > > 
> > > When the fault returns to L5, it loads that reg %x with $1 and jumps back 
> > > to L2, where it can see that it took a fault.
> > > 
> > > Now lets look at what happens when we do not have that jump back to L2. 
> > > Instead of going back to the original code, it will load $22 into %x and 
> > > jmp back to the wrong area. God knows what will happen then, since the 
> > > stack pointer thinks it is from where the original fault occurred.
> > 
> > 
> > Heh, that's fairly logic. Don't ask me why, but I did not imagine each
> > part of .fixup unified in a separate contiguous section (but what else can it be?...).
> > 
> > Thanks for your explanations :-)
> 
> This bit:
> 
>   	".section .fixup, \"ax\"\n"
>  	"4: movl $1, %[faulted]\n"
>  	"   jmp 3b\n"
>  	".previous\n"
> 
> Can be thought of as an 'embedded' or 'nested' section - the '.previous'
> directive jumps back to whatever section we were in before. This can be
> nested multiple times too:
> 
>    .section A
>    [...]
>      .section B
>      [...]
>         .section C
>         [...]
>         .previous
>      [...]
>      .previous
>    [...]
>    .previous
> 
> For whatever reason the interaction of the assembler with the linker and
> in particular linker scripts are one of the most undocumented areas of OSS.
> Does anyone know any good reference to start with? 
> 
> Something that explains the principles, how it all works, what the various
> section flags mean in practice, including details like dwarf2/CFI annotations.
> 
> I do not know about any coherent documentation in this area and as a result
> many developers shy away from this area, frequently mess it up if they have to
> touch it and generally treat it as voodoo.
> 
> 	Ingo

When I wrote this part, I used the following documentation:

http://tldp.org/LDP/khg/HyperNews/get/devices/exceptions.html

But yeah, the assembler/linker stuff documentation for gcc are not so much
documented.

There is the raw reference:
http://sourceware.org/binutils/docs/as/index.html

Concerning examples, practical cases, it is exploded on several parts on the web, some good
links can be found on http://asm.sourceforge.net/

Thanks for your explanation.
But I'm confused, if the .previous make a jump to the previous section, then it already
does what jmp 3b does right?

Re: git pull request for tip/tracing/urgent

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> hm ... doh, yes. I think we even had a bug in this area.
> 
> Why is it implemented like this? These semantics seem to make no sense.
> 

They don't, which is why .pushsection/.popsection now exists.

.section/.previous really are rather broken, but like many things with 
broken semantics there is too much that expect the broken semantics now...

	-hpa