Re: [PATCH][GIT PULL][for 2.6.35] tracing: Add alignment tosyscall metadata declarations

[Sam Ravnborg]

On Fri, Jul 09, 2010 at 03:56:42PM -0400, Steven Rostedt wrote:
> 
> Ingo,
> 
> Please pull the latest tip/perf/urgent tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/perf/urgent
> 
> 
> Steven Rostedt (1):
>       tracing: Add alignment to syscall metadata declarations
> 
> ----
>  include/linux/syscalls.h |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> ---------------------------
> commit 44a54f787c0abcf75a2ed49b8ec8b2b512468f73
> Author: Steven Rostedt <srostedt@xxxxxxxxxx>
> Date:   Fri Jul 9 15:41:44 2010 -0400
> 
>     tracing: Add alignment to syscall metadata declarations
>     
>     For some reason if we declare a static variable and then assign it
>     later, and the assignment contains a __attribute__((__aligned__(#))),
>     some versions of gcc will ignore it.
>     
>     This caused the syscall meta data to not be compact in its section
>     and caused a kernel oops when the section was being read.
>     
>     The fix for these versions of gcc seems to be to add the aligned
>     attribute to the declaration as well.
>     
>     This fixes the BZ regression:
>     
>       https://bugzilla.kernel.org/show_bug.cgi?id=16353
>     
>     Reported-by: Zeev Tarantov <zeev.tarantov@xxxxxxxxx>
>     Tested-by: Zeev Tarantov <zeev.tarantov@xxxxxxxxx>
>     Acked-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>
>     LKML-Reference: <AANLkTinkKVmB0fpVeqUkMeqe3ZYeXJdI8xDuzJEOjYwh@xxxxxxxxxxxxxx>
>     Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> 
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 7f614ce..13ebb54 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -124,7 +124,8 @@ extern struct trace_event_functions enter_syscall_print_funcs;
>  extern struct trace_event_functions exit_syscall_print_funcs;
>  
>  #define SYSCALL_TRACE_ENTER_EVENT(sname)				\
> -	static struct syscall_metadata __syscall_meta_##sname;		\
> +	static struct syscall_metadata					\
> +	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
>  	static struct ftrace_event_call					\
>  	__attribute__((__aligned__(4))) event_enter_##sname;		\
>  	static struct ftrace_event_call __used				\
> @@ -138,7 +139,8 @@ extern struct trace_event_functions exit_syscall_print_funcs;
>  	}
>  
>  #define SYSCALL_TRACE_EXIT_EVENT(sname)					\
> -	static struct syscall_metadata __syscall_meta_##sname;		\
> +	static struct syscall_metadata					\
> +	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
>  	static struct ftrace_event_call					\
>  	__attribute__((__aligned__(4))) event_exit_##sname;		\
>  	static struct ftrace_event_call __used				\

This looks like a fix that just hide the real bug.
If I remember the original report correct the problem is
that the symbol:

    __start_syscalls_metadata

Does not point to a valid syscall entry.

The symbol is assigned in vmlinux.lds.h like this:
#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
                         *(__syscalls_metadata)                         \
                         VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;

Now consider what is happening if we have the following scanario:

. equals 0x1004 so __start_syscalls_metadata is set to 0x1004
But __syscall_metadata require 8 byte alignment so it starts at 0x1008.

Then __start_syscalls_metadata fails to point at the first entry and
this code will fail:

        start = (struct syscall_metadata *)__start_syscalls_metadata;
        for ( ; start < stop; start++) {
                if (start->name && !strcmp(start->name + 3, str + 3))
                        return start;
 


iThe right fix seems to me that we guarantee that __start_syscalls_metadata
is assinged a proper address.

Something like this:
(whitespace damaged)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 48c5299..64430d3 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -133,7 +133,8 @@
 #endif

 #ifdef CONFIG_FTRACE_SYSCALLS
-#define TRACE_SYSCALLS() VMLINUX_SYMBOL(__start_syscalls_metadata) = .;        \
+#define TRACE_SYSCALLS() . = ALIGN(8);                                 \
+                        VMLINUX_SYMBOL(__start_syscalls_metadata) = .; \
                         *(__syscalls_metadata)                         \
                         VMLINUX_SYMBOL(__stop_syscalls_metadata) = .;
 #else


But at the same time we should audit the other trace related symbols
in vmlinux.lds.h to see if they may suffer from the same potential bug.

__start_ftrace_events looks like it may have the same potential bug..

The reason why your patch works is that you force a smaller alignment
and this happens by pure luck to be the alignmnet of "." from the
previous section.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/