[email protected] writes: > https://bugzilla.kernel.org/show_bug.cgi?id=111541 > > --- Comment #8 from Weilong Chen <[email protected]> --- > (In reply to Rusty Russell from comment #7) >> Rusty Russell <[email protected]> writes: >> > And there are other places with the same issue. This is a more >> > complex, but I think worth it (actually two patches, rolled into one >> > for testing): >> >> And this one actually works... >> >> diff --git a/include/linux/module.h b/include/linux/module.h >> index 4560d8f1545d..2bb0c3085706 100644 >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >> @@ -324,6 +324,12 @@ struct module_layout { >> #define __module_layout_align >> #endif >> >> +struct mod_kallsyms { >> + Elf_Sym *symtab; >> + unsigned int num_symtab; >> + char *strtab; >> +}; >> + >> struct module { >> enum module_state state; >> >> @@ -405,15 +411,10 @@ struct module { >> #endif >> >> #ifdef CONFIG_KALLSYMS >> - /* >> - * We keep the symbol and string tables for kallsyms. >> - * The core_* fields below are temporary, loader-only (they >> - * could really be discarded after module init). >> - */ >> - Elf_Sym *symtab, *core_symtab; >> - unsigned int num_symtab, core_num_syms; >> - char *strtab, *core_strtab; >> - >> + /* Protected by RCU and/or module_mutex: use rcu_dereference() */ >> + struct mod_kallsyms *kallsyms; >> + struct mod_kallsyms core_kallsyms; >> + >> /* Section attributes */ >> struct module_sect_attrs *sect_attrs; >> >> diff --git a/kernel/module.c b/kernel/module.c >> index 2149f7003e49..4e4d567139f4 100644 >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -303,6 +303,9 @@ struct load_info { >> struct _ddebug *debug; >> unsigned int num_debug; >> bool sig_ok; >> +#ifdef CONFIG_KALLSYMS >> + unsigned long mod_kallsyms_init_off; >> +#endif >> struct { >> unsigned int sym, str, mod, vers, info, pcpu; >> } index; >> @@ -2480,10 +2483,21 @@ static void layout_symtab(struct module *mod, struct >> load_info *info) >> strsect->sh_flags |= SHF_ALLOC; >> strsect->sh_entsize = get_offset(mod, &mod->init_layout.size, strsect, >> info->index.str) | INIT_OFFSET_MASK; >> - mod->init_layout.size = debug_align(mod->init_layout.size); >> pr_debug("\t%s\n", info->secstrings + strsect->sh_name); >> + >> + /* We'll tack temporary mod_kallsyms on the end. */ >> + mod->init_layout.size = ALIGN(mod->init_layout.size, >> + __alignof__(struct mod_kallsyms)); >> + info->mod_kallsyms_init_off = mod->init_layout.size; >> + mod->init_layout.size += sizeof(struct mod_kallsyms); >> + mod->init_layout.size = debug_align(mod->init_layout.size); >> } >> >> +/* >> + * We use the full symtab and strtab which layout_symtab arranged to >> + * be appended to the init section Later we switch to the cut-down >> + * core-only one. >> + */ >> static void add_kallsyms(struct module *mod, const struct load_info *info) >> { >> unsigned int i, ndst; >> @@ -2492,29 +2506,34 @@ static void add_kallsyms(struct module *mod, const >> struct load_info *info) >> char *s; >> Elf_Shdr *symsec = &info->sechdrs[info->index.sym]; >> >> - mod->symtab = (void *)symsec->sh_addr; >> - mod->num_symtab = symsec->sh_size / sizeof(Elf_Sym); >> + /* Set up to point into init section. */ >> + mod->kallsyms = mod->init_layout.base + info->mod_kallsyms_init_off; >> + >> + mod->kallsyms->symtab = (void *)symsec->sh_addr; >> + mod->kallsyms->num_symtab = symsec->sh_size / sizeof(Elf_Sym); >> /* Make sure we get permanent strtab: don't use info->strtab. */ >> - mod->strtab = (void *)info->sechdrs[info->index.str].sh_addr; >> + mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr; >> >> /* Set types up while we still have access to sections. */ >> - for (i = 0; i < mod->num_symtab; i++) >> - mod->symtab[i].st_info = elf_type(&mod->symtab[i], info); >> - >> - mod->core_symtab = dst = mod->core_layout.base + info->symoffs; >> - mod->core_strtab = s = mod->core_layout.base + info->stroffs; >> - src = mod->symtab; >> - for (ndst = i = 0; i < mod->num_symtab; i++) { >> + for (i = 0; i < mod->kallsyms->num_symtab; i++) >> + mod->kallsyms->symtab[i].st_info >> + = elf_type(&mod->kallsyms->symtab[i], info); >> + >> + /* Now populate the cut down core kallsyms for after init. */ >> + mod->core_kallsyms.symtab = dst = mod->core_layout.base + info->symoffs; >> + mod->core_kallsyms.strtab = s = mod->core_layout.base + info->stroffs; >> + src = mod->kallsyms->symtab; >> + for (ndst = i = 0; i < mod->kallsyms->num_symtab; i++) { >> if (i == 0 || >> is_core_symbol(src+i, info->sechdrs, info->hdr->e_shnum, >> info->index.pcpu)) { >> dst[ndst] = src[i]; >> - dst[ndst++].st_name = s - mod->core_strtab; >> - s += strlcpy(s, &mod->strtab[src[i].st_name], >> + dst[ndst++].st_name = s - mod->core_kallsyms.strtab; >> + s += strlcpy(s, &mod->kallsyms->strtab[src[i].st_name], >> KSYM_NAME_LEN) + 1; >> } >> } >> - mod->core_num_syms = ndst; >> + mod->core_kallsyms.num_symtab = ndst; >> } >> #else >> static inline void layout_symtab(struct module *mod, struct load_info *info) >> @@ -3263,9 +3282,8 @@ static noinline int do_init_module(struct module *mod) >> module_put(mod); >> trim_init_extable(mod); >> #ifdef CONFIG_KALLSYMS >> - mod->num_symtab = mod->core_num_syms; >> - mod->symtab = mod->core_symtab; >> - mod->strtab = mod->core_strtab; >> + /* Switch to core kallsyms now init is done. */ >> + rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); >> #endif >> mod_tree_remove_init(mod); >> disable_ro_nx(&mod->init_layout); >> @@ -3627,6 +3645,11 @@ static inline int is_arm_mapping_symbol(const char >> *str) >> && (str[2] == '\0' || str[2] == '.'); >> } >> >> +static const char *symname(struct mod_kallsyms *kallsyms, unsigned int >> symnum) >> +{ >> + return kallsyms->strtab + kallsyms->symtab[symnum].st_name; >> +} >> + >> static const char *get_ksymbol(struct module *mod, >> unsigned long addr, >> unsigned long *size, >> @@ -3634,6 +3657,7 @@ static const char *get_ksymbol(struct module *mod, >> { >> unsigned int i, best = 0; >> unsigned long nextval; >> + struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms); >> >> /* At worse, next value is at end of module */ >> if (within_module_init(addr, mod)) >> @@ -3643,32 +3667,32 @@ static const char *get_ksymbol(struct module *mod, >> >> /* Scan for closest preceding symbol, and next symbol. (ELF >> starts real symbols at 1). */ >> - for (i = 1; i < mod->num_symtab; i++) { >> - if (mod->symtab[i].st_shndx == SHN_UNDEF) >> + for (i = 1; i < kallsyms->num_symtab; i++) { >> + if (kallsyms->symtab[i].st_shndx == SHN_UNDEF) >> continue; >> >> /* We ignore unnamed symbols: they're uninformative >> * and inserted at a whim. */ >> - if (mod->symtab[i].st_value <= addr >> - && mod->symtab[i].st_value > mod->symtab[best].st_value >> - && *(mod->strtab + mod->symtab[i].st_name) != '\0' >> - && !is_arm_mapping_symbol(mod->strtab + >> mod->symtab[i].st_name)) >> + if (*symname(kallsyms, i) == '\0' >> + || 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) >> best = i; >> - if (mod->symtab[i].st_value > addr >> - && mod->symtab[i].st_value < nextval >> - && *(mod->strtab + mod->symtab[i].st_name) != '\0' >> - && !is_arm_mapping_symbol(mod->strtab + >> mod->symtab[i].st_name)) >> - nextval = mod->symtab[i].st_value; >> + if (kallsyms->symtab[i].st_value > addr >> + && kallsyms->symtab[i].st_value < nextval) >> + nextval = kallsyms->symtab[i].st_value; >> } >> >> if (!best) >> return NULL; >> >> if (size) >> - *size = nextval - mod->symtab[best].st_value; >> + *size = nextval - kallsyms->symtab[best].st_value; >> if (offset) >> - *offset = addr - mod->symtab[best].st_value; >> - return mod->strtab + mod->symtab[best].st_name; >> + *offset = addr - kallsyms->symtab[best].st_value; >> + return symname(kallsyms, best); >> } >> >> /* For kallsyms to ask for address resolution. NULL means not found. >> Careful >> @@ -3758,19 +3782,21 @@ int module_get_kallsym(unsigned int symnum, unsigned >> long *value, char *type, >> >> preempt_disable(); >> list_for_each_entry_rcu(mod, &modules, list) { >> + struct mod_kallsyms *kallsyms; >> + >> if (mod->state == MODULE_STATE_UNFORMED) >> continue; >> - if (symnum < mod->num_symtab) { >> - *value = mod->symtab[symnum].st_value; >> - *type = mod->symtab[symnum].st_info; >> - strlcpy(name, mod->strtab + mod->symtab[symnum].st_name, >> - KSYM_NAME_LEN); >> + kallsyms = rcu_dereference_sched(mod->kallsyms); >> + if (symnum < kallsyms->num_symtab) { >> + *value = kallsyms->symtab[symnum].st_value; >> + *type = kallsyms->symtab[symnum].st_info; >> + strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN); >> strlcpy(module_name, mod->name, MODULE_NAME_LEN); >> *exported = is_exported(name, *value, mod); >> preempt_enable(); >> return 0; >> } >> - symnum -= mod->num_symtab; >> + symnum -= kallsyms->num_symtab; >> } >> preempt_enable(); >> return -ERANGE; >> @@ -3779,11 +3805,12 @@ int module_get_kallsym(unsigned int symnum, unsigned >> long *value, char *type, >> 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 < mod->num_symtab; i++) >> - if (strcmp(name, mod->strtab+mod->symtab[i].st_name) == 0 && >> - mod->symtab[i].st_info != 'U') >> - return mod->symtab[i].st_value; >> + for (i = 0; i < kallsyms->num_symtab; i++) >> + if (strcmp(name, symname(kallsyms, i)) == 0 && >> + kallsyms->symtab[i].st_info != 'U') >> + return kallsyms->symtab[i].st_value; >> return 0; >> } >> >> @@ -3822,11 +3849,14 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, >> const char *, >> module_assert_mutex(); >> >> list_for_each_entry(mod, &modules, list) { >> + /* We hold module_mutex: no need for rcu_dereference_sched */ >> + struct mod_kallsyms *kallsyms = mod->kallsyms; >> + >> if (mod->state == MODULE_STATE_UNFORMED) >> continue; >> - for (i = 0; i < mod->num_symtab; i++) { >> - ret = fn(data, mod->strtab + mod->symtab[i].st_name, >> - mod, mod->symtab[i].st_value); >> + for (i = 0; i < kallsyms->num_symtab; i++) { >> + ret = fn(data, symname(kallsyms, i), >> + mod, kallsyms->symtab[i].st_value); >> if (ret != 0) >> return ret; >> } > > Test ok. > But its really a big patch. > The lts kernels such as 3.10 also have the problem, this patch seems not work > for them, as some code are different.
Yes :( Thankyou for testing! I will push this with CC: stable, and do the backports myself as required. Thanks, Rusty.
