Re: git pull request for tip/tracing/urgent

[Frederic Weisbecker <fweisbec@gmail.com>]

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)
>