On Wed, Dec 30, 2009 at 11:49 PM, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > On Wed, 2009-12-30 at 13:41 +0200, Kalle Valo wrote: >> Hello, >> >> I noticed weird crashes related to wl1251_spi notes sysfs directory >> with current wireless-testing (2.6.33-rc2 plus some wireless patches). >> The simplest way to reproduce the problem is to do this on a nokia n900 >> (arm/omap 3430): >> >> # ls /sys/module/wl1251_spi/notes/ >> [ 4776.503234] Unable to handle kernel NULL pointer dereference at >> virtual address 00000000 >> [ 4776.511596] pgd = cce88000 >> [ 4776.514343] [00000000] *pgd=8f04a031, *pte=00000000, *ppte=00000000 >> [ 4776.520812] Internal error: Oops: 17 [#1] >> [ 4776.524871] last sysfs file: /sys/class/net/wlan0/flags >> [ 4776.530151] Modules linked in: wl1251_spi wl1251 mac80211 cfg80211 >> [ 4776.536468] CPU: 0 Not tainted (2.6.33-rc2-wl-47091-g981eb84 >> #12) >> [ 4776.542999] PC is at strlen+0xc/0x20 >> [ 4776.546630] LR is at sysfs_readdir+0x15c/0x1e0 >> [ 4776.551116] pc : [<c01476ac>] lr : [<c00f5e6c>] psr: a0000013 >> [ 4776.551147] sp : cce87f28 ip : 22222222 fp : be99961c >> [ 4776.562744] r10: cce87f80 r9 : 00000000 r8 : 00000000 >> [ 4776.568023] r7 : c00b9540 r6 : cce87f80 r5 : ccec4458 r4 : >> ce808980 >> [ 4776.574615] r3 : 00000000 r2 : 00000002 r1 : 22222222 r0 : >> 00000000 >> [ 4776.581207] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM >> Segment user >> [ 4776.588409] Control: 10c5387d Table: 8ce88019 DAC: 00000015 >> [ 4776.594238] Process ls (pid: 1148, stack limit = 0xcce862e8) >> [ 4776.599945] Stack: (0xcce87f28 to 0xcce88000) >> [ 4776.604370] 7f20: 00000001 00000000 00000e16 >> 00000000 00000004 22222222 >> [ 4776.612640] 7f40: ce808980 ce808980 cf79e34c c00b9540 00000000 >> cf79e2b8 cce86000 c00b982c >> [ 4776.620910] 7f60: 00000001 00000000 00001000 000690d0 ce808980 >> c002bae4 00000000 c00b98c4 >> [ 4776.629180] 7f80: 00069100 000690e8 00000fd0 ffffffea 00000000 >> 00000000 00000000 00000000 >> [ 4776.637451] 7fa0: 000000d9 c002b940 00000000 00000000 00000003 >> 000690d0 00001000 00000000 >> [ 4776.645721] 7fc0: 00000000 00000000 00000000 000000d9 000690c8 >> 00000001 00000000 be99961c >> [ 4776.654022] 7fe0: 400ef954 be999614 400efa10 400ef908 60000010 >> 00000003 80c69021 80c69421 >> [ 4776.662292] [<c01476ac>] (strlen+0xc/0x20) from [<c00f5e6c>] >> (sysfs_readdir+0x15c/0x1e0) >> [ 4776.670501] [<c00f5e6c>] (sysfs_readdir+0x15c/0x1e0) from >> [<c00b982c>] (vfs_readdir+0x80/0xb4) >> [ 4776.679229] [<c00b982c>] (vfs_readdir+0x80/0xb4) from [<c00b98c4>] >> (sys_getdents64+0x64/0xb4) >> [ 4776.687866] [<c00b98c4>] (sys_getdents64+0x64/0xb4) from >> [<c002b940>] (ret_fast_syscall+0x0/0x38) >> [ 4776.696838] Code: c027700c e1a03000 ea000000 e2833001 (e5d32000) >> [ 4776.703063] ---[ end trace 6a3b0fdf4e9def99 ]--- >> [ 4776.707794] Kernel panic - not syncing: Fatal exception > >> Also removing wl1251_spi causes a crash. The reason for this is that a >> sysfs file with a null string as name is trying to be removed from the >> notes directory. > > Yes, this is because the notes sections describe the text sections. > When we ignore an empty text section, we'd need to ignore its > corresponding notes section. > > The reason we didn't see this on parisc is because our compiler doesn't > actually produce any notes sections. > >> I found out that reverting this patch solves the problem: >> >> commit 35dead4235e2b67da7275b4122fed37099c2f462 >> Author: Helge Deller <deller@xxxxxx> >> Date: Thu Dec 3 00:29:15 2009 +0100 >> >> modules: don't export section names of empty sections via sysfs >> >> On the parisc architecture we face for each and every loaded >> kernel module this kernel "badness warning": >> >> sysfs: cannot create duplicate filename >> '/module/ac97_bus/sections/.text' >> Badness at fs/sysfs/dir.c:487 >> >> Reason for that is, that on parisc all kernel modules do have >> multiple .text sections due to the usage of the >> -ffunction-sections compiler flag which is needed to reach all >> jump targets on this platform. >> >> An objdump on such a kernel module gives: >> Sections: >> Idx Name Size VMA LMA File off Algn >> 0 .note.gnu.build-id 00000024 00000000 00000000 00000034 >> 2**2 >> CONTENTS, ALLOC, LOAD, READONLY, DATA >> 1 .text 00000000 00000000 00000000 00000058 2**0 >> CONTENTS, ALLOC, LOAD, READONLY, CODE >> 2 .text.ac97_bus_match 0000001c 00000000 00000000 00000058 >> 2**2 >> CONTENTS, ALLOC, LOAD, READONLY, CODE >> 3 .text 00000000 00000000 00000000 000000d4 2**0 >> CONTENTS, ALLOC, LOAD, READONLY, CODE >> ... >> Since the .text sections are empty (size of 0 bytes) and won't be >> loaded by the kernel module loader anyway, I don't see a reason >> why such sections need to be listed under >> /sys/module/<module_name>/sections/<section_name> either. >> >> The attached patch does solve this issue by not exporting section >> names which are empty. >> >> This fixes bugzilla >> http://bugzilla.kernel.org/show_bug.cgi?id=14703 >> >> Signed-off-by: Helge Deller <deller@xxxxxx> >> CC: rusty@xxxxxxxxxxxxxxx >> CC: akpm@xxxxxxxxxxxxxxxxxxxx >> CC: James.Bottomley@xxxxxxxxxxxxxxxxxxxxx >> CC: roland@xxxxxxxxxx >> CC: dave@xxxxxxxxxxxxxxxxxx >> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> >> >> I was also able to reproduce the problem with vanilla 2.6.32. I'm >> pretty sure (but haven't tested) that 2.6.32-rc8 does not have this >> problem. >> >> My original mail containing more info: >> >> http://www.spinics.net/lists/linux-wireless/msg44863.html >> >> Simple bandaid patch below fixes the problem. I know it's not a proper >> solution, but hopefully makes it easier to understand the problem. >> Unfortunately my knowledge about ELF is too limited to fix this >> properly, but I can provide more information as needed. Or even try to >> fix it myself if someone else holds my hand :) >> >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -1189,10 +1189,13 @@ static void add_notes_attrs(struct module >> *mod, unsigned int nsect, >> if (!notes_attrs->dir) >> goto out; >> >> - for (i = 0; i < notes; ++i) >> + for (i = 0; i < notes; ++i) { >> + if (WARN_ON(!notes_attrs->attrs[i].attr.name)) >> + continue; >> if (sysfs_create_bin_file(notes_attrs->dir, >> ¬es_attrs->attrs[i])) >> goto out; >> + } >> >> mod->notes_attrs = notes_attrs; >> return; > > A better, and more comprehensive patch would be to try not to count the > empty text sections when we're building the notes section (and actually > anywhere else in the file). This patch actually relies on the fact that > if sh_size is zero for the text section it should be for the > corresponding notes section. If that doesn't work, we'd actually have > to do the matching in the construction piece. > > Can you try it to see if it works for you? If it doesn't, I'll try > matching notes to text. It works fine on parisc, but as we don't have a > notes section, that's not saying much ... > > Thanks, > > James > This patch looks fine for me, except that I don't think it's necessary to introduce an inline function for that... Reviewed-by: WANG Cong <xiyou.wangcong@xxxxxxxxx> Thanks! > --- > > diff --git a/kernel/module.c b/kernel/module.c > index e96b8ed..957f912 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -132,6 +132,11 @@ void __module_put_and_exit(struct module *mod, long code) > } > EXPORT_SYMBOL(__module_put_and_exit); > > +static inline int section_allocated(Elf_Shdr hdr) > +{ > + return (hdr.sh_flags & SHF_ALLOC) && hdr.sh_size != 0; > +} > + > /* Find a module section: 0 means not found. */ > static unsigned int find_sec(Elf_Ehdr *hdr, > Elf_Shdr *sechdrs, > @@ -142,7 +147,7 @@ static unsigned int find_sec(Elf_Ehdr *hdr, > > for (i = 1; i < hdr->e_shnum; i++) > /* Alloc bit cleared means "ignore it." */ > - if ((sechdrs[i].sh_flags & SHF_ALLOC) > + if (section_allocated(sechdrs[i]) > && strcmp(secstrings+sechdrs[i].sh_name, name) == 0) > return i; > return 0; > @@ -1051,8 +1056,7 @@ static void add_sect_attrs(struct module *mod, unsigned int nsect, > > /* Count loaded sections and allocate structures */ > for (i = 0; i < nsect; i++) > - if (sechdrs[i].sh_flags & SHF_ALLOC > - && sechdrs[i].sh_size) > + if (section_allocated(sechdrs[i])) > nloaded++; > size[0] = ALIGN(sizeof(*sect_attrs) > + nloaded * sizeof(sect_attrs->attrs[0]), > @@ -1070,9 +1074,7 @@ static void add_sect_attrs(struct module *mod, unsigned int nsect, > sattr = §_attrs->attrs[0]; > gattr = §_attrs->grp.attrs[0]; > for (i = 0; i < nsect; i++) { > - if (! (sechdrs[i].sh_flags & SHF_ALLOC)) > - continue; > - if (!sechdrs[i].sh_size) > + if (!section_allocated(sechdrs[i])) > continue; > sattr->address = sechdrs[i].sh_addr; > sattr->name = kstrdup(secstrings + sechdrs[i].sh_name, > @@ -1156,7 +1158,7 @@ static void add_notes_attrs(struct module *mod, unsigned int nsect, > /* Count notes sections and allocate structures. */ > notes = 0; > for (i = 0; i < nsect; i++) > - if ((sechdrs[i].sh_flags & SHF_ALLOC) && > + if (section_allocated(sechdrs[i]) && > (sechdrs[i].sh_type == SHT_NOTE)) > ++notes; > > @@ -1172,7 +1174,7 @@ static void add_notes_attrs(struct module *mod, unsigned int nsect, > notes_attrs->notes = notes; > nattr = ¬es_attrs->attrs[0]; > for (loaded = i = 0; i < nsect; ++i) { > - if (!(sechdrs[i].sh_flags & SHF_ALLOC)) > + if (!section_allocated(sechdrs[i])) > continue; > if (sechdrs[i].sh_type == SHT_NOTE) { > nattr->attr.name = mod->sect_attrs->attrs[loaded].name; > @@ -1720,7 +1722,7 @@ static char elf_type(const Elf_Sym *sym, > return '?'; > if (sechdrs[sym->st_shndx].sh_flags & SHF_EXECINSTR) > return 't'; > - if (sechdrs[sym->st_shndx].sh_flags & SHF_ALLOC > + if (section_allocated(sechdrs[sym->st_shndx]) > && sechdrs[sym->st_shndx].sh_type != SHT_NOBITS) { > if (!(sechdrs[sym->st_shndx].sh_flags & SHF_WRITE)) > return 'r'; > @@ -1751,7 +1753,7 @@ static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs, > return false; > > sec = sechdrs + src->st_shndx; > - if (!(sec->sh_flags & SHF_ALLOC) > + if (!section_allocated(*sec) > #ifndef CONFIG_KALLSYMS_ALL > || !(sec->sh_flags & SHF_EXECINSTR) > #endif > @@ -1913,7 +1915,7 @@ static void kmemleak_load_module(struct module *mod, Elf_Ehdr *hdr, > kmemleak_scan_area(mod, sizeof(struct module), GFP_KERNEL); > > for (i = 1; i < hdr->e_shnum; i++) { > - if (!(sechdrs[i].sh_flags & SHF_ALLOC)) > + if (!section_allocated(sechdrs[i])) > continue; > if (strncmp(secstrings + sechdrs[i].sh_name, ".data", 5) != 0 > && strncmp(secstrings + sechdrs[i].sh_name, ".bss", 4) != 0) > @@ -2139,7 +2141,7 @@ static noinline struct module *load_module(void __user *umod, > for (i = 0; i < hdr->e_shnum; i++) { > void *dest; > > - if (!(sechdrs[i].sh_flags & SHF_ALLOC)) > + if (!section_allocated(sechdrs[i])) > continue; > > if (sechdrs[i].sh_entsize & INIT_OFFSET_MASK) > @@ -2287,7 +2289,7 @@ static noinline struct module *load_module(void __user *umod, > continue; > > /* Don't bother with non-allocated sections */ > - if (!(sechdrs[info].sh_flags & SHF_ALLOC)) > + if (!section_allocated(sechdrs[info])) > continue; > > if (sechdrs[i].sh_type == SHT_REL) > > > -- > 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/ > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
