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

From: James Bottomley
Date: Wed Dec 30 2009 - 10:49:44 EST

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

---

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/