[ltt-dev] [PATCH] ARM: Set bit 0 for thumb mode in kallsyms_lookup_name returned address

[ltt-dev] [PATCH] ARM: Set bit 0 for thumb mode in kallsyms_lookup_name returned address

[Avik Sil]

This patch fixes the undefined instruction oops due to execution
of thumb-2 code in ARM mode. The zero bit in the symbol address
returned by kallsyms_lookup_name is not set, leading to switching
to ARM mode that generates oops while executing thumb-2 code. For
detailed discussion, see [1].
[1] http://lists.casi.polymtl.ca/pipermail/ltt-dev/2011-September/005176.html

Signed-off-by: Avik Sil <avik.sil at linaro.org>
---
 lttng-context-prio.c |   11 ++++++++++-
 wrapper/ftrace.h     |   28 ++++++++++++++++++++++------
 wrapper/splice.c     |   14 ++++++++++++--
 wrapper/vmalloc.h    |   14 +++++++++++---
 4 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/lttng-context-prio.c b/lttng-context-prio.c
index ad1c42f..5e8c0d2 100644
--- a/lttng-context-prio.c
+++ b/lttng-context-prio.c
@@ -20,7 +20,16 @@ int (*wrapper_task_prio_sym)(struct task_struct *t);
 
 int wrapper_task_prio_init(void)
 {
-	wrapper_task_prio_sym = (void *) kallsyms_lookup_name("task_prio");
+        unsigned long addr;
+
+        addr = kallsyms_lookup_name("task_prio");
+#ifdef CONFIG_ARM 
+#ifdef CONFIG_THUMB2_KERNEL
+        if (addr)
+                addr |= 1; /* set bit 0 in address for thumb mode */
+#endif
+#endif
+        wrapper_task_prio_sym = (void *) addr;
 	if (!wrapper_task_prio_sym) {
 		printk(KERN_WARNING "LTTng: task_prio symbol lookup failed.\n");
 		return -EINVAL;
diff --git a/wrapper/ftrace.h b/wrapper/ftrace.h
index 9c18cc5..d79aa5e 100644
--- a/wrapper/ftrace.h
+++ b/wrapper/ftrace.h
@@ -21,10 +21,18 @@ static inline
 int wrapper_register_ftrace_function_probe(char *glob,
 		struct ftrace_probe_ops *ops, void *data)
 {
-	int (*register_ftrace_function_probe_sym)(char *glob,
-			struct ftrace_probe_ops *ops, void *data);
+        unsigned long addr;
+        int (*register_ftrace_function_probe_sym)(char *glob,
+                        struct ftrace_probe_ops *ops, void *data);
 
-	register_ftrace_function_probe_sym = (void *) kallsyms_lookup_name("register_ftrace_function_probe");
+        addr = kallsyms_lookup_name("register_ftrace_function_probe");
+#ifdef CONFIG_ARM 
+#ifdef CONFIG_THUMB2_KERNEL
+        if (addr)
+                addr |= 1; /* set bit 0 in address for thumb mode */
+#endif
+#endif
+        register_ftrace_function_probe_sym = (void *) addr;
 	if (register_ftrace_function_probe_sym) {
 		return register_ftrace_function_probe_sym(glob, ops, data);
 	} else {
@@ -37,10 +45,18 @@ static inline
 void wrapper_unregister_ftrace_function_probe(char *glob,
 		struct ftrace_probe_ops *ops, void *data)
 {
-	void (*unregister_ftrace_function_probe_sym)(char *glob,
-			struct ftrace_probe_ops *ops, void *data);
+        unsigned long addr;
+        void (*unregister_ftrace_function_probe_sym)(char *glob,
+                        struct ftrace_probe_ops *ops, void *data);
 
-	unregister_ftrace_function_probe_sym = (void *) kallsyms_lookup_name("unregister_ftrace_function_probe");
+        addr = kallsyms_lookup_name("unregister_ftrace_function_probe");
+#ifdef CONFIG_ARM 
+#ifdef CONFIG_THUMB2_KERNEL
+        if (addr)
+                addr |= 1; /* set bit 0 in address for thumb mode */
+#endif
+#endif
+        unregister_ftrace_function_probe_sym = (void *) addr;
 	if (unregister_ftrace_function_probe_sym) {
 		unregister_ftrace_function_probe_sym(glob, ops, data);
 	} else {
diff --git a/wrapper/splice.c b/wrapper/splice.c
index edc499c..c2c275b 100644
--- a/wrapper/splice.c
+++ b/wrapper/splice.c
@@ -21,8 +21,18 @@ ssize_t (*splice_to_pipe_sym)(struct pipe_inode_info *pipe,
 ssize_t wrapper_splice_to_pipe(struct pipe_inode_info *pipe,
 			       struct splice_pipe_desc *spd)
 {
-	if (!splice_to_pipe_sym)
-		splice_to_pipe_sym = (void *) kallsyms_lookup_name("splice_to_pipe");
+        unsigned long addr;
+
+        if (!splice_to_pipe_sym) {
+                addr = kallsyms_lookup_name("splice_to_pipe");
+#ifdef CONFIG_ARM 
+#ifdef CONFIG_THUMB2_KERNEL
+                if (addr)
+                        addr |= 1; /* set bit 0 in address for thumb mode */
+#endif
+#endif
+                splice_to_pipe_sym = (void *) addr;
+        }
 	if (splice_to_pipe_sym) {
 		return splice_to_pipe_sym(pipe, spd);
 	} else {
diff --git a/wrapper/vmalloc.h b/wrapper/vmalloc.h
index 7d87855..d512c89 100644
--- a/wrapper/vmalloc.h
+++ b/wrapper/vmalloc.h
@@ -18,9 +18,17 @@
 static inline
 void wrapper_vmalloc_sync_all(void)
 {
-	void (*vmalloc_sync_all_sym)(void);
-
-	vmalloc_sync_all_sym = (void *) kallsyms_lookup_name("vmalloc_sync_all");
+        unsigned long addr;
+        void (*vmalloc_sync_all_sym)(void);
+
+        addr = kallsyms_lookup_name("vmalloc_sync_all");
+#ifdef CONFIG_ARM 
+#ifdef CONFIG_THUMB2_KERNEL
+        if (addr)
+                addr |= 1; /* set bit 0 in address for thumb mode */
+#endif
+#endif
+        vmalloc_sync_all_sym = (void *) addr;
 	if (vmalloc_sync_all_sym) {
 		vmalloc_sync_all_sym();
 	} else {
-- 
1.7.0.4

[ltt-dev] [PATCH] ARM: Set bit 0 for thumb mode in kallsyms_lookup_name returned address

[Mathieu Desnoyers]

* Avik Sil (avik.sil at linaro.org) wrote:
> This patch fixes the undefined instruction oops due to execution
> of thumb-2 code in ARM mode. The zero bit in the symbol address
> returned by kallsyms_lookup_name is not set, leading to switching
> to ARM mode that generates oops while executing thumb-2 code. For
> detailed discussion, see [1].
> [1] http://lists.casi.polymtl.ca/pipermail/ltt-dev/2011-September/005176.html

Hi Avik,

Instead of modifying all those wrappers individually, can you create a
wrapper/kallsyms.h and create a wrapper for kallsyms_lookup_name ? This
way, the ifdef for the ARM special-case would only appear once.

Thanks!

Mathieu

> 
> Signed-off-by: Avik Sil <avik.sil at linaro.org>
> ---
>  lttng-context-prio.c |   11 ++++++++++-
>  wrapper/ftrace.h     |   28 ++++++++++++++++++++++------
>  wrapper/splice.c     |   14 ++++++++++++--
>  wrapper/vmalloc.h    |   14 +++++++++++---
>  4 files changed, 55 insertions(+), 12 deletions(-)
> 
> diff --git a/lttng-context-prio.c b/lttng-context-prio.c
> index ad1c42f..5e8c0d2 100644
> --- a/lttng-context-prio.c
> +++ b/lttng-context-prio.c
> @@ -20,7 +20,16 @@ int (*wrapper_task_prio_sym)(struct task_struct *t);
>  
>  int wrapper_task_prio_init(void)
>  {
> -	wrapper_task_prio_sym = (void *) kallsyms_lookup_name("task_prio");
> +        unsigned long addr;
> +
> +        addr = kallsyms_lookup_name("task_prio");
> +#ifdef CONFIG_ARM 
> +#ifdef CONFIG_THUMB2_KERNEL
> +        if (addr)
> +                addr |= 1; /* set bit 0 in address for thumb mode */
> +#endif
> +#endif
> +        wrapper_task_prio_sym = (void *) addr;
>  	if (!wrapper_task_prio_sym) {
>  		printk(KERN_WARNING "LTTng: task_prio symbol lookup failed.\n");
>  		return -EINVAL;
> diff --git a/wrapper/ftrace.h b/wrapper/ftrace.h
> index 9c18cc5..d79aa5e 100644
> --- a/wrapper/ftrace.h
> +++ b/wrapper/ftrace.h
> @@ -21,10 +21,18 @@ static inline
>  int wrapper_register_ftrace_function_probe(char *glob,
>  		struct ftrace_probe_ops *ops, void *data)
>  {
> -	int (*register_ftrace_function_probe_sym)(char *glob,
> -			struct ftrace_probe_ops *ops, void *data);
> +        unsigned long addr;
> +        int (*register_ftrace_function_probe_sym)(char *glob,
> +                        struct ftrace_probe_ops *ops, void *data);
>  
> -	register_ftrace_function_probe_sym = (void *) kallsyms_lookup_name("register_ftrace_function_probe");
> +        addr = kallsyms_lookup_name("register_ftrace_function_probe");
> +#ifdef CONFIG_ARM 
> +#ifdef CONFIG_THUMB2_KERNEL
> +        if (addr)
> +                addr |= 1; /* set bit 0 in address for thumb mode */
> +#endif
> +#endif
> +        register_ftrace_function_probe_sym = (void *) addr;
>  	if (register_ftrace_function_probe_sym) {
>  		return register_ftrace_function_probe_sym(glob, ops, data);
>  	} else {
> @@ -37,10 +45,18 @@ static inline
>  void wrapper_unregister_ftrace_function_probe(char *glob,
>  		struct ftrace_probe_ops *ops, void *data)
>  {
> -	void (*unregister_ftrace_function_probe_sym)(char *glob,
> -			struct ftrace_probe_ops *ops, void *data);
> +        unsigned long addr;
> +        void (*unregister_ftrace_function_probe_sym)(char *glob,
> +                        struct ftrace_probe_ops *ops, void *data);
>  
> -	unregister_ftrace_function_probe_sym = (void *) kallsyms_lookup_name("unregister_ftrace_function_probe");
> +        addr = kallsyms_lookup_name("unregister_ftrace_function_probe");
> +#ifdef CONFIG_ARM 
> +#ifdef CONFIG_THUMB2_KERNEL
> +        if (addr)
> +                addr |= 1; /* set bit 0 in address for thumb mode */
> +#endif
> +#endif
> +        unregister_ftrace_function_probe_sym = (void *) addr;
>  	if (unregister_ftrace_function_probe_sym) {
>  		unregister_ftrace_function_probe_sym(glob, ops, data);
>  	} else {
> diff --git a/wrapper/splice.c b/wrapper/splice.c
> index edc499c..c2c275b 100644
> --- a/wrapper/splice.c
> +++ b/wrapper/splice.c
> @@ -21,8 +21,18 @@ ssize_t (*splice_to_pipe_sym)(struct pipe_inode_info *pipe,
>  ssize_t wrapper_splice_to_pipe(struct pipe_inode_info *pipe,
>  			       struct splice_pipe_desc *spd)
>  {
> -	if (!splice_to_pipe_sym)
> -		splice_to_pipe_sym = (void *) kallsyms_lookup_name("splice_to_pipe");
> +        unsigned long addr;
> +
> +        if (!splice_to_pipe_sym) {
> +                addr = kallsyms_lookup_name("splice_to_pipe");
> +#ifdef CONFIG_ARM 
> +#ifdef CONFIG_THUMB2_KERNEL
> +                if (addr)
> +                        addr |= 1; /* set bit 0 in address for thumb mode */
> +#endif
> +#endif
> +                splice_to_pipe_sym = (void *) addr;
> +        }
>  	if (splice_to_pipe_sym) {
>  		return splice_to_pipe_sym(pipe, spd);
>  	} else {
> diff --git a/wrapper/vmalloc.h b/wrapper/vmalloc.h
> index 7d87855..d512c89 100644
> --- a/wrapper/vmalloc.h
> +++ b/wrapper/vmalloc.h
> @@ -18,9 +18,17 @@
>  static inline
>  void wrapper_vmalloc_sync_all(void)
>  {
> -	void (*vmalloc_sync_all_sym)(void);
> -
> -	vmalloc_sync_all_sym = (void *) kallsyms_lookup_name("vmalloc_sync_all");
> +        unsigned long addr;
> +        void (*vmalloc_sync_all_sym)(void);
> +
> +        addr = kallsyms_lookup_name("vmalloc_sync_all");
> +#ifdef CONFIG_ARM 
> +#ifdef CONFIG_THUMB2_KERNEL
> +        if (addr)
> +                addr |= 1; /* set bit 0 in address for thumb mode */
> +#endif
> +#endif
> +        vmalloc_sync_all_sym = (void *) addr;
>  	if (vmalloc_sync_all_sym) {
>  		vmalloc_sync_all_sym();
>  	} else {
> -- 
> 1.7.0.4
> 
> 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH] ARM: Set bit 0 for thumb mode in kallsyms_lookup_name returned address

[Dave Martin]

On Fri, Sep 16, 2011 at 2:53 PM, Mathieu Desnoyers
<compudj at krystal.dyndns.org> wrote:
> * Avik Sil (avik.sil at linaro.org) wrote:
>> This patch fixes the undefined instruction oops due to execution
>> of thumb-2 code in ARM mode. The zero bit in the symbol address
>> returned by kallsyms_lookup_name is not set, leading to switching
>> to ARM mode that generates oops while executing thumb-2 code. For
>> detailed discussion, see [1].
>> [1] http://lists.casi.polymtl.ca/pipermail/ltt-dev/2011-September/005176.html
>
> Hi Avik,
>
> Instead of modifying all those wrappers individually, can you create a
> wrapper/kallsyms.h and create a wrapper for kallsyms_lookup_name ? This
> way, the ifdef for the ARM special-case would only appear once.

kallsyms_lookup_name() makes no distinction between function and data
symbols, so we should be wary of changing the behaviour of this
function globally.  I suspect that indiscriminately setting bit 0 of
the return of kallsyms_lookup_name() may lead to some wrong behaviour.

[ Aside: this problem is copounded by the fact that the kallsyms table
is built by parsing the output of nm, so the recorded symbol type
information is somewhat wrong: nm seems to classify symbols according
to what section they're in, _not_ according to the actual symbol type
declared in the ELF symbol table.  This means that if
kallsyms_lookup_name() is used to get the address of a data symbol
defined in the text segment of an assembler file, it's hard to
guarantee to return a sensible result.  (There are also plenty of
symbols in assembler files with no declared ELF symbol type at all,
but that's arguably a fixable coding bug in those files) ]

I think we do need an arch-specific wrapper with a separate, explicit
name such as kallsyms_lookup_funcptr(), and all code which really
wants a callable function pointer to be returned should use this
wrapper instead of kallsyms_lookup_name(), at least for all
arch-independent code, and arch-dependent code for arches where it
makes a difference.

In particular, I really think we shouldn't be pasting multiple
arch-specific #ifdefs like

+                addr = kallsyms_lookup_name("splice_to_pipe");
+#ifdef CONFIG_ARM
+#ifdef CONFIG_THUMB2_KERNEL
+        if (addr)
+                addr |= 1; /* set bit 0 in address for thumb mode */
+#endif
+#endif

into arch-independent code -- this will cerate a lot of patch noise
and frustrate maintenance.


Personally, I would much prefer

-               splice_to_pipe_sym = (void *)
kallsyms_lookup_name("splice_to_pipe");
+               splice_to_pipe_sym = (void *)
kallsyms_lookup_funcptr("splice_to_pipe");

...which also makes the code clearer IMHO.  kallsyms_lookup_funcptr()
must necessarily come from arch-specific headers for many arches,
including ARM.

Cheers
---Dave