* [PATCH v5 1/2] module: Overwrite st_size instead of st_info @ 2018-12-04 14:14 Vincent Whitchurch 2018-12-04 14:14 ` [PATCH v5 2/2] ARM: module: Fix function kallsyms on Thumb-2 Vincent Whitchurch 2018-12-06 9:45 ` [PATCH v5 1/2] module: Overwrite st_size instead of st_info Miroslav Benes 0 siblings, 2 replies; 5+ messages in thread From: Vincent Whitchurch @ 2018-12-04 14:14 UTC (permalink / raw) To: linux, jeyu Cc: dave.martin, linux-arm-kernel, linux-kernel, Vincent Whitchurch st_info is currently overwritten after relocation and used to store the elf_type(). However, we're going to need it fix kallsyms on ARM's Thumb-2 kernels, so preserve st_info and overwrite the st_size field instead. st_size is neither used by the module core nor by any architecture. Reviewed-by: Dave Martin <Dave.Martin@arm.com> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> --- v5: Add Dave Martin's Reviewed-by v4: Split out to separate patch. Use st_size instead of st_other. v1-v3: See PATCH 2/2 kernel/module.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/module.c b/kernel/module.c index 49a405891587..3d86a38b580c 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2682,7 +2682,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info) /* Set types up while we still have access to sections. */ for (i = 0; i < mod->kallsyms->num_symtab; i++) - mod->kallsyms->symtab[i].st_info + mod->kallsyms->symtab[i].st_size = elf_type(&mod->kallsyms->symtab[i], info); /* Now populate the cut down core kallsyms for after init. */ @@ -4061,7 +4061,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type, kallsyms = rcu_dereference_sched(mod->kallsyms); if (symnum < kallsyms->num_symtab) { *value = kallsyms->symtab[symnum].st_value; - *type = kallsyms->symtab[symnum].st_info; + *type = kallsyms->symtab[symnum].st_size; strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN); strlcpy(module_name, mod->name, MODULE_NAME_LEN); *exported = is_exported(name, *value, mod); -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v5 2/2] ARM: module: Fix function kallsyms on Thumb-2 2018-12-04 14:14 [PATCH v5 1/2] module: Overwrite st_size instead of st_info Vincent Whitchurch @ 2018-12-04 14:14 ` Vincent Whitchurch 2018-12-06 17:29 ` Jessica Yu 2018-12-06 9:45 ` [PATCH v5 1/2] module: Overwrite st_size instead of st_info Miroslav Benes 1 sibling, 1 reply; 5+ messages in thread From: Vincent Whitchurch @ 2018-12-04 14:14 UTC (permalink / raw) To: linux, jeyu Cc: dave.martin, linux-arm-kernel, linux-kernel, Vincent Whitchurch Thumb-2 functions have the lowest bit set in the symbol value in the symtab. When kallsyms are generated for the vmlinux, the kallsyms are generated from the output of nm, and nm clears the lowest bit. $ arm-linux-gnueabihf-readelf -a vmlinux | grep show_interrupts 95947: 8015dc89 686 FUNC GLOBAL DEFAULT 2 show_interrupts $ arm-linux-gnueabihf-nm vmlinux | grep show_interrupts 8015dc88 T show_interrupts $ cat /proc/kallsyms | grep show_interrupts 8015dc88 T show_interrupts However, for modules, the kallsyms uses the values in the symbol table without modification, so for functions in modules, the lowest bit is set in kallsyms. $ arm-linux-gnueabihf-readelf -a drivers/net/tun.ko | grep tun_get_socket 333: 00002d4d 36 FUNC GLOBAL DEFAULT 1 tun_get_socket $ arm-linux-gnueabihf-nm drivers/net/tun.ko | grep tun_get_socket 00002d4c T tun_get_socket $ cat /proc/kallsyms | grep tun_get_socket 7f802d4d t tun_get_socket [tun] Because of this, the symbol+offset of the crashing instruction shown in oopses is incorrect when the crash is in a module. For example, given a tun_get_socket which starts like this, 00002d4c <tun_get_socket>: 2d4c: 6943 ldr r3, [r0, #20] 2d4e: 4a07 ldr r2, [pc, #28] 2d50: 4293 cmp r3, r2 a crash when tun_get_socket is called with NULL results in: PC is at tun_xdp+0xa3/0xa4 [tun] pc : [<7f802d4c>] As can be seen, the "PC is at" line reports the wrong symbol name, and the symbol+offset will point to the wrong source line if it is passed to gdb. To solve this, add a way for archs to fixup the reading of these module kallsyms values, and use that to clear the lowest bit for function symbols on Thumb-2. After the fix: # cat /proc/kallsyms | grep tun_get_socket 7f802d4c t tun_get_socket [tun] PC is at tun_get_socket+0x0/0x24 [tun] pc : [<7f802d4c>] Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> --- v5: Use/move local variables to reduce calls and keep lines short. Use const arg. v4: Split out st_value overwrite change. Add HAVE* macro to avoid function call. v3: Do not overwrite st_value v2: Fix build warning with !MODULES arch/arm/include/asm/module.h | 11 +++++++++++ include/linux/module.h | 7 +++++++ kernel/module.c | 45 +++++++++++++++++++++++++++---------------- 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h index 9e81b7c498d8..c7bcf0347801 100644 --- a/arch/arm/include/asm/module.h +++ b/arch/arm/include/asm/module.h @@ -61,4 +61,15 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val); MODULE_ARCH_VERMAGIC_ARMTHUMB \ MODULE_ARCH_VERMAGIC_P2V +#ifdef CONFIG_THUMB2_KERNEL +#define HAVE_ARCH_MODULE_KALLSYMS_SYMBOL_VALUE +static inline unsigned long module_kallsyms_symbol_value(const Elf_Sym *sym) +{ + if (ELF_ST_TYPE(sym->st_info) == STT_FUNC) + return sym->st_value & ~1; + + return sym->st_value; +} +#endif + #endif /* _ASM_ARM_MODULE_H */ diff --git a/include/linux/module.h b/include/linux/module.h index fce6b4335e36..12146257eb5d 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -486,6 +486,13 @@ struct module { #define MODULE_ARCH_INIT {} #endif +#ifndef HAVE_ARCH_MODULE_KALLSYMS_SYMBOL_VALUE +static inline unsigned long module_kallsyms_symbol_value(const Elf_Sym *sym) +{ + return sym->st_value; +} +#endif + extern struct mutex module_mutex; /* FIXME: It'd be nice to isolate modules during init, too, so they diff --git a/kernel/module.c b/kernel/module.c index 3d86a38b580c..9364017fdc21 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3922,7 +3922,7 @@ static const char *get_ksymbol(struct module *mod, unsigned long *offset) { unsigned int i, best = 0; - unsigned long nextval; + unsigned long nextval, bestval; struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms); /* At worse, next value is at end of module */ @@ -3931,10 +3931,15 @@ static const char *get_ksymbol(struct module *mod, else nextval = (unsigned long)mod->core_layout.base+mod->core_layout.text_size; + bestval = module_kallsyms_symbol_value(&kallsyms->symtab[best]); + /* Scan for closest preceding symbol, and next symbol. (ELF starts real symbols at 1). */ for (i = 1; i < kallsyms->num_symtab; i++) { - if (kallsyms->symtab[i].st_shndx == SHN_UNDEF) + const Elf_Sym *sym = &kallsyms->symtab[i]; + unsigned long thisval = module_kallsyms_symbol_value(sym); + + if (sym->st_shndx == SHN_UNDEF) continue; /* We ignore unnamed symbols: they're uninformative @@ -3943,21 +3948,21 @@ static const char *get_ksymbol(struct module *mod, || is_arm_mapping_symbol(symname(kallsyms, i))) continue; - if (kallsyms->symtab[i].st_value <= addr - && kallsyms->symtab[i].st_value > kallsyms->symtab[best].st_value) + if (thisval <= addr && thisval > bestval) { best = i; - if (kallsyms->symtab[i].st_value > addr - && kallsyms->symtab[i].st_value < nextval) - nextval = kallsyms->symtab[i].st_value; + bestval = thisval; + } + if (thisval > addr && thisval < nextval) + nextval = thisval; } if (!best) return NULL; if (size) - *size = nextval - kallsyms->symtab[best].st_value; + *size = nextval - bestval; if (offset) - *offset = addr - kallsyms->symtab[best].st_value; + *offset = addr - bestval; return symname(kallsyms, best); } @@ -4060,8 +4065,10 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type, continue; kallsyms = rcu_dereference_sched(mod->kallsyms); if (symnum < kallsyms->num_symtab) { - *value = kallsyms->symtab[symnum].st_value; - *type = kallsyms->symtab[symnum].st_size; + const Elf_Sym *sym = &kallsyms->symtab[symnum]; + + *value = module_kallsyms_symbol_value(sym); + *type = sym->st_size; strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN); strlcpy(module_name, mod->name, MODULE_NAME_LEN); *exported = is_exported(name, *value, mod); @@ -4079,10 +4086,13 @@ static unsigned long mod_find_symname(struct module *mod, const char *name) unsigned int i; struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms); - for (i = 0; i < kallsyms->num_symtab; i++) + for (i = 0; i < kallsyms->num_symtab; i++) { + const Elf_Sym *sym = &kallsyms->symtab[i]; + if (strcmp(name, symname(kallsyms, i)) == 0 && - kallsyms->symtab[i].st_shndx != SHN_UNDEF) - return kallsyms->symtab[i].st_value; + sym->st_shndx != SHN_UNDEF) + return module_kallsyms_symbol_value(sym); + } return 0; } @@ -4127,12 +4137,13 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, if (mod->state == MODULE_STATE_UNFORMED) continue; for (i = 0; i < kallsyms->num_symtab; i++) { + const Elf_Sym *sym = &kallsyms->symtab[i]; - if (kallsyms->symtab[i].st_shndx == SHN_UNDEF) + if (sym->st_shndx == SHN_UNDEF) continue; - ret = fn(data, symname(kallsyms, i), - mod, kallsyms->symtab[i].st_value); + ret = fn(data, symname(kallsyms, i), mod, + module_kallsyms_symbol_value(sym)); if (ret != 0) return ret; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v5 2/2] ARM: module: Fix function kallsyms on Thumb-2 2018-12-04 14:14 ` [PATCH v5 2/2] ARM: module: Fix function kallsyms on Thumb-2 Vincent Whitchurch @ 2018-12-06 17:29 ` Jessica Yu 2018-12-14 16:07 ` Vincent Whitchurch 0 siblings, 1 reply; 5+ messages in thread From: Jessica Yu @ 2018-12-06 17:29 UTC (permalink / raw) To: Vincent Whitchurch Cc: linux, dave.martin, linux-arm-kernel, linux-kernel, Vincent Whitchurch +++ Vincent Whitchurch [04/12/18 15:14 +0100]: >Thumb-2 functions have the lowest bit set in the symbol value in the >symtab. When kallsyms are generated for the vmlinux, the kallsyms are >generated from the output of nm, and nm clears the lowest bit. > > $ arm-linux-gnueabihf-readelf -a vmlinux | grep show_interrupts > 95947: 8015dc89 686 FUNC GLOBAL DEFAULT 2 show_interrupts > $ arm-linux-gnueabihf-nm vmlinux | grep show_interrupts > 8015dc88 T show_interrupts > $ cat /proc/kallsyms | grep show_interrupts > 8015dc88 T show_interrupts > >However, for modules, the kallsyms uses the values in the symbol table >without modification, so for functions in modules, the lowest bit is set >in kallsyms. > > $ arm-linux-gnueabihf-readelf -a drivers/net/tun.ko | grep tun_get_socket > 333: 00002d4d 36 FUNC GLOBAL DEFAULT 1 tun_get_socket > $ arm-linux-gnueabihf-nm drivers/net/tun.ko | grep tun_get_socket > 00002d4c T tun_get_socket > $ cat /proc/kallsyms | grep tun_get_socket > 7f802d4d t tun_get_socket [tun] > >Because of this, the symbol+offset of the crashing instruction shown in >oopses is incorrect when the crash is in a module. For example, given a >tun_get_socket which starts like this, > > 00002d4c <tun_get_socket>: > 2d4c: 6943 ldr r3, [r0, #20] > 2d4e: 4a07 ldr r2, [pc, #28] > 2d50: 4293 cmp r3, r2 > >a crash when tun_get_socket is called with NULL results in: > > PC is at tun_xdp+0xa3/0xa4 [tun] > pc : [<7f802d4c>] > >As can be seen, the "PC is at" line reports the wrong symbol name, and >the symbol+offset will point to the wrong source line if it is passed to >gdb. > >To solve this, add a way for archs to fixup the reading of these module >kallsyms values, and use that to clear the lowest bit for function >symbols on Thumb-2. > >After the fix: > > # cat /proc/kallsyms | grep tun_get_socket > 7f802d4c t tun_get_socket [tun] > > PC is at tun_get_socket+0x0/0x24 [tun] > pc : [<7f802d4c>] > >Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> Looks good, could I get an ACK from an ARM maintainer? (Russell?) Also, do you mind if I drop the module_ prefix from module_kallsyms_symbol_value()? I recently submitted some internal module kallsyms cleanups [1] and there we have the newly renamed kallsyms_symbol_name(), so I think it'd be nice if we kept the naming scheme consistent with just kallsyms_symbol_value(). I could just do this myself if you don't want to respin a v6. Thanks! Jessica [1] https://git.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git/commit/?h=modules-next&id=2d25bc55235314d869459c574be14e8faa73aca3 >--- >v5: Use/move local variables to reduce calls and keep lines short. Use const arg. >v4: Split out st_value overwrite change. Add HAVE* macro to avoid function call. >v3: Do not overwrite st_value >v2: Fix build warning with !MODULES > > arch/arm/include/asm/module.h | 11 +++++++++++ > include/linux/module.h | 7 +++++++ > kernel/module.c | 45 +++++++++++++++++++++++++++---------------- > 3 files changed, 46 insertions(+), 17 deletions(-) > >diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h >index 9e81b7c498d8..c7bcf0347801 100644 >--- a/arch/arm/include/asm/module.h >+++ b/arch/arm/include/asm/module.h >@@ -61,4 +61,15 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val); > MODULE_ARCH_VERMAGIC_ARMTHUMB \ > MODULE_ARCH_VERMAGIC_P2V > >+#ifdef CONFIG_THUMB2_KERNEL >+#define HAVE_ARCH_MODULE_KALLSYMS_SYMBOL_VALUE >+static inline unsigned long module_kallsyms_symbol_value(const Elf_Sym *sym) >+{ >+ if (ELF_ST_TYPE(sym->st_info) == STT_FUNC) >+ return sym->st_value & ~1; >+ >+ return sym->st_value; >+} >+#endif >+ > #endif /* _ASM_ARM_MODULE_H */ >diff --git a/include/linux/module.h b/include/linux/module.h >index fce6b4335e36..12146257eb5d 100644 >--- a/include/linux/module.h >+++ b/include/linux/module.h >@@ -486,6 +486,13 @@ struct module { > #define MODULE_ARCH_INIT {} > #endif > >+#ifndef HAVE_ARCH_MODULE_KALLSYMS_SYMBOL_VALUE >+static inline unsigned long module_kallsyms_symbol_value(const Elf_Sym *sym) >+{ >+ return sym->st_value; >+} >+#endif >+ > extern struct mutex module_mutex; > > /* FIXME: It'd be nice to isolate modules during init, too, so they >diff --git a/kernel/module.c b/kernel/module.c >index 3d86a38b580c..9364017fdc21 100644 >--- a/kernel/module.c >+++ b/kernel/module.c >@@ -3922,7 +3922,7 @@ static const char *get_ksymbol(struct module *mod, > unsigned long *offset) > { > unsigned int i, best = 0; >- unsigned long nextval; >+ unsigned long nextval, bestval; > struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms); > > /* At worse, next value is at end of module */ >@@ -3931,10 +3931,15 @@ static const char *get_ksymbol(struct module *mod, > else > nextval = (unsigned long)mod->core_layout.base+mod->core_layout.text_size; > >+ bestval = module_kallsyms_symbol_value(&kallsyms->symtab[best]); >+ > /* Scan for closest preceding symbol, and next symbol. (ELF > starts real symbols at 1). */ > for (i = 1; i < kallsyms->num_symtab; i++) { >- if (kallsyms->symtab[i].st_shndx == SHN_UNDEF) >+ const Elf_Sym *sym = &kallsyms->symtab[i]; >+ unsigned long thisval = module_kallsyms_symbol_value(sym); >+ >+ if (sym->st_shndx == SHN_UNDEF) > continue; > > /* We ignore unnamed symbols: they're uninformative >@@ -3943,21 +3948,21 @@ static const char *get_ksymbol(struct module *mod, > || is_arm_mapping_symbol(symname(kallsyms, i))) > continue; > >- if (kallsyms->symtab[i].st_value <= addr >- && kallsyms->symtab[i].st_value > kallsyms->symtab[best].st_value) >+ if (thisval <= addr && thisval > bestval) { > best = i; >- if (kallsyms->symtab[i].st_value > addr >- && kallsyms->symtab[i].st_value < nextval) >- nextval = kallsyms->symtab[i].st_value; >+ bestval = thisval; >+ } >+ if (thisval > addr && thisval < nextval) >+ nextval = thisval; > } > > if (!best) > return NULL; > > if (size) >- *size = nextval - kallsyms->symtab[best].st_value; >+ *size = nextval - bestval; > if (offset) >- *offset = addr - kallsyms->symtab[best].st_value; >+ *offset = addr - bestval; > return symname(kallsyms, best); > } > >@@ -4060,8 +4065,10 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type, > continue; > kallsyms = rcu_dereference_sched(mod->kallsyms); > if (symnum < kallsyms->num_symtab) { >- *value = kallsyms->symtab[symnum].st_value; >- *type = kallsyms->symtab[symnum].st_size; >+ const Elf_Sym *sym = &kallsyms->symtab[symnum]; >+ >+ *value = module_kallsyms_symbol_value(sym); >+ *type = sym->st_size; > strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN); > strlcpy(module_name, mod->name, MODULE_NAME_LEN); > *exported = is_exported(name, *value, mod); >@@ -4079,10 +4086,13 @@ static unsigned long mod_find_symname(struct module *mod, const char *name) > unsigned int i; > struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms); > >- for (i = 0; i < kallsyms->num_symtab; i++) >+ for (i = 0; i < kallsyms->num_symtab; i++) { >+ const Elf_Sym *sym = &kallsyms->symtab[i]; >+ > if (strcmp(name, symname(kallsyms, i)) == 0 && >- kallsyms->symtab[i].st_shndx != SHN_UNDEF) >- return kallsyms->symtab[i].st_value; >+ sym->st_shndx != SHN_UNDEF) >+ return module_kallsyms_symbol_value(sym); >+ } > return 0; > } > >@@ -4127,12 +4137,13 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, > if (mod->state == MODULE_STATE_UNFORMED) > continue; > for (i = 0; i < kallsyms->num_symtab; i++) { >+ const Elf_Sym *sym = &kallsyms->symtab[i]; > >- if (kallsyms->symtab[i].st_shndx == SHN_UNDEF) >+ if (sym->st_shndx == SHN_UNDEF) > continue; > >- ret = fn(data, symname(kallsyms, i), >- mod, kallsyms->symtab[i].st_value); >+ ret = fn(data, symname(kallsyms, i), mod, >+ module_kallsyms_symbol_value(sym)); > if (ret != 0) > return ret; > } >-- >2.11.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 2/2] ARM: module: Fix function kallsyms on Thumb-2 2018-12-06 17:29 ` Jessica Yu @ 2018-12-14 16:07 ` Vincent Whitchurch 0 siblings, 0 replies; 5+ messages in thread From: Vincent Whitchurch @ 2018-12-14 16:07 UTC (permalink / raw) To: Jessica Yu; +Cc: linux, dave.martin, linux-arm-kernel, linux-kernel On Thu, Dec 06, 2018 at 06:29:20PM +0100, Jessica Yu wrote: > Also, do you mind if I drop the module_ prefix from > module_kallsyms_symbol_value()? I recently submitted some internal > module kallsyms cleanups [1] and there we have the newly renamed > kallsyms_symbol_name(), so I think it'd be nice if we kept the naming > scheme consistent with just kallsyms_symbol_value(). I could just do > this myself if you don't want to respin a v6. I've sent out a v6 with this change. I also rebased the change on top of your cleanups in modules-next. Thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/2] module: Overwrite st_size instead of st_info 2018-12-04 14:14 [PATCH v5 1/2] module: Overwrite st_size instead of st_info Vincent Whitchurch 2018-12-04 14:14 ` [PATCH v5 2/2] ARM: module: Fix function kallsyms on Thumb-2 Vincent Whitchurch @ 2018-12-06 9:45 ` Miroslav Benes 1 sibling, 0 replies; 5+ messages in thread From: Miroslav Benes @ 2018-12-06 9:45 UTC (permalink / raw) To: Vincent Whitchurch Cc: linux, jeyu, dave.martin, linux-arm-kernel, linux-kernel, Vincent Whitchurch On Tue, 4 Dec 2018, Vincent Whitchurch wrote: > st_info is currently overwritten after relocation and used to store the > elf_type(). However, we're going to need it fix kallsyms on ARM's > Thumb-2 kernels, so preserve st_info and overwrite the st_size field > instead. st_size is neither used by the module core nor by any > architecture. > > Reviewed-by: Dave Martin <Dave.Martin@arm.com> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> Reviewed-by: Miroslav Benes <mbenes@suse.cz> M ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-14 16:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-04 14:14 [PATCH v5 1/2] module: Overwrite st_size instead of st_info Vincent Whitchurch 2018-12-04 14:14 ` [PATCH v5 2/2] ARM: module: Fix function kallsyms on Thumb-2 Vincent Whitchurch 2018-12-06 17:29 ` Jessica Yu 2018-12-14 16:07 ` Vincent Whitchurch 2018-12-06 9:45 ` [PATCH v5 1/2] module: Overwrite st_size instead of st_info Miroslav Benes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).