| Message ID | 1316422804-32562-1-git-send-email-avik.sil@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
* Avik Sil (avik.sil@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 > > v1: > - include wrapper function kallsyms_lookup_funcptr as suggested > by Dave Martin > > Signed-off-by: Avik Sil <avik.sil@linaro.org> Merged, thanks! Mathieu > --- > lttng-context-prio.c | 3 ++- > wrapper/ftrace.h | 5 +++-- > wrapper/kallsyms.h | 28 ++++++++++++++++++++++++++++ > wrapper/splice.c | 3 ++- > wrapper/vmalloc.h | 3 ++- > 5 files changed, 37 insertions(+), 5 deletions(-) > create mode 100644 wrapper/kallsyms.h > > diff --git a/lttng-context-prio.c b/lttng-context-prio.c > index ad1c42f..1ee3a54 100644 > --- a/lttng-context-prio.c > +++ b/lttng-context-prio.c > @@ -13,6 +13,7 @@ > #include "ltt-events.h" > #include "wrapper/ringbuffer/frontend_types.h" > #include "wrapper/vmalloc.h" > +#include "wrapper/kallsyms.h" > #include "ltt-tracer.h" > > static > @@ -20,7 +21,7 @@ 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"); > + wrapper_task_prio_sym = (void *) kallsyms_lookup_funcptr("task_prio"); > 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..ace33c5 100644 > --- a/wrapper/ftrace.h > +++ b/wrapper/ftrace.h > @@ -16,6 +16,7 @@ > #ifdef CONFIG_KALLSYMS > > #include <linux/kallsyms.h> > +#include "kallsyms.h" > > static inline > int wrapper_register_ftrace_function_probe(char *glob, > @@ -24,7 +25,7 @@ int wrapper_register_ftrace_function_probe(char *glob, > 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"); > + register_ftrace_function_probe_sym = (void *) kallsyms_lookup_funcptr("register_ftrace_function_probe"); > if (register_ftrace_function_probe_sym) { > return register_ftrace_function_probe_sym(glob, ops, data); > } else { > @@ -40,7 +41,7 @@ void wrapper_unregister_ftrace_function_probe(char *glob, > 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"); > + unregister_ftrace_function_probe_sym = (void *) kallsyms_lookup_funcptr("unregister_ftrace_function_probe"); > if (unregister_ftrace_function_probe_sym) { > unregister_ftrace_function_probe_sym(glob, ops, data); > } else { > diff --git a/wrapper/kallsyms.h b/wrapper/kallsyms.h > new file mode 100644 > index 0000000..bb45f38 > --- /dev/null > +++ b/wrapper/kallsyms.h > @@ -0,0 +1,28 @@ > +#ifndef _LTT_WRAPPER_KALLSYMS_H > +#define _LTT_WRAPPER_KALLSYMS_H > + > +/* > + * Copyright (C) 2011 Avik Sil (avik.sil@linaro.org) > + * > + * wrapper around kallsyms_lookup_name. Implements arch-dependent code for > + * arches where the address of the start of the function body is different > + * from the pointer which can be used to call the function, e.g. ARM THUMB2. > + * > + * Dual LGPL v2.1/GPL v2 license. > + */ > + > +static inline > +unsigned long kallsyms_lookup_funcptr(const char *name) > +{ > + unsigned long addr; > + > + addr = kallsyms_lookup_name(name); > +#ifdef CONFIG_ARM > +#ifdef CONFIG_THUMB2_KERNEL > + if (addr) > + addr |= 1; /* set bit 0 in address for thumb mode */ > +#endif > +#endif > + return addr; > +} > +#endif /* _LTT_WRAPPER_KALLSYMS_H */ > diff --git a/wrapper/splice.c b/wrapper/splice.c > index edc499c..ba224ee 100644 > --- a/wrapper/splice.c > +++ b/wrapper/splice.c > @@ -13,6 +13,7 @@ > #include <linux/kallsyms.h> > #include <linux/fs.h> > #include <linux/splice.h> > +#include "kallsyms.h" > > static > ssize_t (*splice_to_pipe_sym)(struct pipe_inode_info *pipe, > @@ -22,7 +23,7 @@ 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"); > + splice_to_pipe_sym = (void *) kallsyms_lookup_funcptr("splice_to_pipe"); > 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..765f2ad 100644 > --- a/wrapper/vmalloc.h > +++ b/wrapper/vmalloc.h > @@ -14,13 +14,14 @@ > #ifdef CONFIG_KALLSYMS > > #include <linux/kallsyms.h> > +#include "kallsyms.h" > > 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"); > + vmalloc_sync_all_sym = (void *) kallsyms_lookup_funcptr("vmalloc_sync_all"); > if (vmalloc_sync_all_sym) { > vmalloc_sync_all_sym(); > } else { > -- > 1.7.0.4 > > > _______________________________________________ > ltt-dev mailing list > ltt-dev@lists.casi.polymtl.ca > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev >
On Mon, Sep 19, 2011 at 10:00 AM, Avik Sil <avik.sil@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 > > v1: > - include wrapper function kallsyms_lookup_funcptr as suggested > by Dave Martin > > Signed-off-by: Avik Sil <avik.sil@linaro.org> Looks reasonable. Tixy, could it make sense for that definition of kallsyms_lookup_funcptr to migrate into the kernel headers? I had the impression that you might also have used this in some places, if it had been available. Cheers ---Dave > --- > lttng-context-prio.c | 3 ++- > wrapper/ftrace.h | 5 +++-- > wrapper/kallsyms.h | 28 ++++++++++++++++++++++++++++ > wrapper/splice.c | 3 ++- > wrapper/vmalloc.h | 3 ++- > 5 files changed, 37 insertions(+), 5 deletions(-) > create mode 100644 wrapper/kallsyms.h > > diff --git a/lttng-context-prio.c b/lttng-context-prio.c > index ad1c42f..1ee3a54 100644 > --- a/lttng-context-prio.c > +++ b/lttng-context-prio.c > @@ -13,6 +13,7 @@ > #include "ltt-events.h" > #include "wrapper/ringbuffer/frontend_types.h" > #include "wrapper/vmalloc.h" > +#include "wrapper/kallsyms.h" > #include "ltt-tracer.h" > > static > @@ -20,7 +21,7 @@ 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"); > + wrapper_task_prio_sym = (void *) kallsyms_lookup_funcptr("task_prio"); > 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..ace33c5 100644 > --- a/wrapper/ftrace.h > +++ b/wrapper/ftrace.h > @@ -16,6 +16,7 @@ > #ifdef CONFIG_KALLSYMS > > #include <linux/kallsyms.h> > +#include "kallsyms.h" > > static inline > int wrapper_register_ftrace_function_probe(char *glob, > @@ -24,7 +25,7 @@ int wrapper_register_ftrace_function_probe(char *glob, > 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"); > + register_ftrace_function_probe_sym = (void *) kallsyms_lookup_funcptr("register_ftrace_function_probe"); > if (register_ftrace_function_probe_sym) { > return register_ftrace_function_probe_sym(glob, ops, data); > } else { > @@ -40,7 +41,7 @@ void wrapper_unregister_ftrace_function_probe(char *glob, > 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"); > + unregister_ftrace_function_probe_sym = (void *) kallsyms_lookup_funcptr("unregister_ftrace_function_probe"); > if (unregister_ftrace_function_probe_sym) { > unregister_ftrace_function_probe_sym(glob, ops, data); > } else { > diff --git a/wrapper/kallsyms.h b/wrapper/kallsyms.h > new file mode 100644 > index 0000000..bb45f38 > --- /dev/null > +++ b/wrapper/kallsyms.h > @@ -0,0 +1,28 @@ > +#ifndef _LTT_WRAPPER_KALLSYMS_H > +#define _LTT_WRAPPER_KALLSYMS_H > + > +/* > + * Copyright (C) 2011 Avik Sil (avik.sil@linaro.org) > + * > + * wrapper around kallsyms_lookup_name. Implements arch-dependent code for > + * arches where the address of the start of the function body is different > + * from the pointer which can be used to call the function, e.g. ARM THUMB2. > + * > + * Dual LGPL v2.1/GPL v2 license. > + */ > + > +static inline > +unsigned long kallsyms_lookup_funcptr(const char *name) > +{ > + unsigned long addr; > + > + addr = kallsyms_lookup_name(name); > +#ifdef CONFIG_ARM > +#ifdef CONFIG_THUMB2_KERNEL > + if (addr) > + addr |= 1; /* set bit 0 in address for thumb mode */ > +#endif > +#endif > + return addr; > +} > +#endif /* _LTT_WRAPPER_KALLSYMS_H */ > diff --git a/wrapper/splice.c b/wrapper/splice.c > index edc499c..ba224ee 100644 > --- a/wrapper/splice.c > +++ b/wrapper/splice.c > @@ -13,6 +13,7 @@ > #include <linux/kallsyms.h> > #include <linux/fs.h> > #include <linux/splice.h> > +#include "kallsyms.h" > > static > ssize_t (*splice_to_pipe_sym)(struct pipe_inode_info *pipe, > @@ -22,7 +23,7 @@ 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"); > + splice_to_pipe_sym = (void *) kallsyms_lookup_funcptr("splice_to_pipe"); > 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..765f2ad 100644 > --- a/wrapper/vmalloc.h > +++ b/wrapper/vmalloc.h > @@ -14,13 +14,14 @@ > #ifdef CONFIG_KALLSYMS > > #include <linux/kallsyms.h> > +#include "kallsyms.h" > > 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"); > + vmalloc_sync_all_sym = (void *) kallsyms_lookup_funcptr("vmalloc_sync_all"); > if (vmalloc_sync_all_sym) { > vmalloc_sync_all_sym(); > } else { > -- > 1.7.0.4 > >
On Mon, 2011-09-19 at 16:10 +0100, Dave Martin wrote: > On Mon, Sep 19, 2011 at 10:00 AM, Avik Sil <avik.sil@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 > > > > v1: > > - include wrapper function kallsyms_lookup_funcptr as suggested > > by Dave Martin > > > > Signed-off-by: Avik Sil <avik.sil@linaro.org> > > Looks reasonable. > > Tixy, could it make sense for that definition of > kallsyms_lookup_funcptr to migrate into the kernel headers? I had the > impression that you might also have used this in some places, if it > had been available. > I don't think so, the problems with kprobes are a bit more involved. The only candidate for using this new function is in kprobes.c ... /* * Some oddball architectures like 64bit powerpc have function descriptors * so this must be overridable. */ #ifndef kprobe_lookup_name #define kprobe_lookup_name(name, addr) \ addr = ((kprobe_opcode_t *)(kallsyms_lookup_name(name))) #endif Replacing the kallsyms_lookup_name() here would make kprobes 'better' for Thumb (avoid a small runtime overhead and some inconsistencies). But I don't think that the semantics of the new function match this use, and if it were desirable, we could overload the above kprobes function from ARM arch code anyway. In fact, I think I'll propose such a patch...
diff --git a/lttng-context-prio.c b/lttng-context-prio.c index ad1c42f..1ee3a54 100644 --- a/lttng-context-prio.c +++ b/lttng-context-prio.c @@ -13,6 +13,7 @@ #include "ltt-events.h" #include "wrapper/ringbuffer/frontend_types.h" #include "wrapper/vmalloc.h" +#include "wrapper/kallsyms.h" #include "ltt-tracer.h" static @@ -20,7 +21,7 @@ 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"); + wrapper_task_prio_sym = (void *) kallsyms_lookup_funcptr("task_prio"); 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..ace33c5 100644 --- a/wrapper/ftrace.h +++ b/wrapper/ftrace.h @@ -16,6 +16,7 @@ #ifdef CONFIG_KALLSYMS #include <linux/kallsyms.h> +#include "kallsyms.h" static inline int wrapper_register_ftrace_function_probe(char *glob, @@ -24,7 +25,7 @@ int wrapper_register_ftrace_function_probe(char *glob, 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"); + register_ftrace_function_probe_sym = (void *) kallsyms_lookup_funcptr("register_ftrace_function_probe"); if (register_ftrace_function_probe_sym) { return register_ftrace_function_probe_sym(glob, ops, data); } else { @@ -40,7 +41,7 @@ void wrapper_unregister_ftrace_function_probe(char *glob, 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"); + unregister_ftrace_function_probe_sym = (void *) kallsyms_lookup_funcptr("unregister_ftrace_function_probe"); if (unregister_ftrace_function_probe_sym) { unregister_ftrace_function_probe_sym(glob, ops, data); } else { diff --git a/wrapper/kallsyms.h b/wrapper/kallsyms.h new file mode 100644 index 0000000..bb45f38 --- /dev/null +++ b/wrapper/kallsyms.h @@ -0,0 +1,28 @@ +#ifndef _LTT_WRAPPER_KALLSYMS_H +#define _LTT_WRAPPER_KALLSYMS_H + +/* + * Copyright (C) 2011 Avik Sil (avik.sil@linaro.org) + * + * wrapper around kallsyms_lookup_name. Implements arch-dependent code for + * arches where the address of the start of the function body is different + * from the pointer which can be used to call the function, e.g. ARM THUMB2. + * + * Dual LGPL v2.1/GPL v2 license. + */ + +static inline +unsigned long kallsyms_lookup_funcptr(const char *name) +{ + unsigned long addr; + + addr = kallsyms_lookup_name(name); +#ifdef CONFIG_ARM +#ifdef CONFIG_THUMB2_KERNEL + if (addr) + addr |= 1; /* set bit 0 in address for thumb mode */ +#endif +#endif + return addr; +} +#endif /* _LTT_WRAPPER_KALLSYMS_H */ diff --git a/wrapper/splice.c b/wrapper/splice.c index edc499c..ba224ee 100644 --- a/wrapper/splice.c +++ b/wrapper/splice.c @@ -13,6 +13,7 @@ #include <linux/kallsyms.h> #include <linux/fs.h> #include <linux/splice.h> +#include "kallsyms.h" static ssize_t (*splice_to_pipe_sym)(struct pipe_inode_info *pipe, @@ -22,7 +23,7 @@ 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"); + splice_to_pipe_sym = (void *) kallsyms_lookup_funcptr("splice_to_pipe"); 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..765f2ad 100644 --- a/wrapper/vmalloc.h +++ b/wrapper/vmalloc.h @@ -14,13 +14,14 @@ #ifdef CONFIG_KALLSYMS #include <linux/kallsyms.h> +#include "kallsyms.h" 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"); + vmalloc_sync_all_sym = (void *) kallsyms_lookup_funcptr("vmalloc_sync_all"); if (vmalloc_sync_all_sym) { vmalloc_sync_all_sym(); } else {
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 v1: - include wrapper function kallsyms_lookup_funcptr as suggested by Dave Martin Signed-off-by: Avik Sil <avik.sil@linaro.org> --- lttng-context-prio.c | 3 ++- wrapper/ftrace.h | 5 +++-- wrapper/kallsyms.h | 28 ++++++++++++++++++++++++++++ wrapper/splice.c | 3 ++- wrapper/vmalloc.h | 3 ++- 5 files changed, 37 insertions(+), 5 deletions(-) create mode 100644 wrapper/kallsyms.h