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

Prev: [PATCH] some clean up in fs/proc
Next: [tip:x86/asm] x86-64: Modify copy_user_generic() alternatives mechanism

From: Kalle Valo on 30 Dec 2009 06:50

---

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.

I found out that reverting this patch solves the problem:

commit 35dead4235e2b67da7275b4122fed37099c2f462
Author: Helge Deller <deller(a)gmx.de>
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(a)gmx.de>
CC: rusty(a)rustcorp.com.au
CC: akpm(a)linux-foundation.org
CC: James.Bottomley(a)HansenPartnership.com
CC: roland(a)redhat.com
CC: dave(a)hiauly1.hia.nrc.ca
Signed-off-by: Linus Torvalds <torvalds(a)linux-foundation.org>

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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: James Bottomley on 30 Dec 2009 10:50

---

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(a)gmx.de>
> 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(a)gmx.de>
> CC: rusty(a)rustcorp.com.au
> CC: akpm(a)linux-foundation.org
> CC: James.Bottomley(a)HansenPartnership.com
> CC: roland(a)redhat.com
> CC: dave(a)hiauly1.hia.nrc.ca
> Signed-off-by: Linus Torvalds <torvalds(a)linux-foundation.org>
>
> 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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Kalle Valo on 30 Dec 2009 12:30

---

James Bottomley <James.Bottomley(a)HansenPartnership.com> writes:

> On Wed, 2009-12-30 at 13:41 +0200, Kalle Valo wrote:
>
>> 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.

Also on omap not all modules can reproduce the problem. For example
cfg80211, mac80211 and wl1251 modules work just fine, but with
wl1251_spi I see the problem everytime.

> 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. I tested the patch on my setup, this time on top of Linus'
tree (commit 6b7b284). And the patch fixes the issue, I can't
reproduce the problem at all, both 'rmmod wl1251_spi' and ls work
without any problems. So here's my tested-by:

Tested-by: Kalle Valo <kalle.valo(a)iki.fi>

Also please consider sending the fix to stable.

Thank you for fixing this so fast.

--
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Helge Deller on 31 Dec 2009 16:20

---

On 12/30/2009 04:49 PM, James Bottomley wrote:
> 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


Ben Hutchings already sent a similar patch.
See: http://patchwork.kernel.org/patch/68925/

IMHO James patch below seems better since it
checks if a section will be allocated at a few more
places...

Helge


> ---
>
> 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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Américo Wang on 31 Dec 2009 22:10

---

On Wed, Dec 30, 2009 at 11:49 PM, James Bottomley
<James.Bottomley(a)hansenpartnership.com> 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(a)gmx.de>
>>   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(a)gmx.de>
>>     CC: rusty(a)rustcorp.com.au
>>     CC: akpm(a)linux-foundation.org
>>     CC: James.Bottomley(a)HansenPartnership.com
>>     CC: roland(a)redhat.com
>>     CC: dave(a)hiauly1.hia.nrc.ca
>>     Signed-off-by: Linus Torvalds <torvalds(a)linux-foundation.org>
>>
>> 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(a)gmail.com>

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(a)vger.kernel.org
> 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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

 |  Next  |  Last
Pages: 1 2
Prev: [PATCH] some clean up in fs/proc
Next: [tip:x86/asm] x86-64: Modify copy_user_generic() alternatives mechanism