Re: regression: crash from 'ls /sys/modules/wl1251_spi/notes'

From: AmÃrico Wang
Date: Thu Dec 31 2009 - 22:08:20 EST

Previous message: Alan Stern: "Re: [osd-dev] [PATCH] scsi_lib: Bug in completion of bidi commands"
In reply to: Helge Deller: "Re: regression: crash from 'ls /sys/modules/wl1251_spi/notes'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &notes_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 = &sect_attrs->attrs[0];
> Â Â Â Âgattr = &sect_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 = &notes_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-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/

Previous message: Alan Stern: "Re: [osd-dev] [PATCH] scsi_lib: Bug in completion of bidi commands"
In reply to: Helge Deller: "Re: regression: crash from 'ls /sys/modules/wl1251_spi/notes'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]