| Message ID | 1250549376.7858.96.camel@mulgrave.site (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
> The root cause is a duplicate section name (.text); is this legal? It's a complicated subject, but I don't think they should ever actually exist in a .ko. The linker script for making .ko's should take care of that. But if modules do some crazy things like using section groups (e.g. for COMDAT, i.e. compile C++ code into modules) then I'm not sure how easy it is to get ld to combine things ideally as we'd like. Suffice it to say that any occurrence of this merits further investigation. It is certainly a red flag at first blush (as it were). > However, there's a problem with commit > 6d76013381ed28979cd122eb4b249a88b5e384fa in that if you fail to allocate > a mod->sect_attrs (in this case it's null because of the duplication), > it still gets used without checking in add_notes_attrs() > > This should fix it Yes, good catch. Sorry about that complete failure of defensive programming. Acked-by: Roland McGrath <roland@redhat.com> Thanks, Roland -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 18 Aug 2009 08:19:36 am James Bottomley wrote: > The root cause is a duplicate section name (.text); is this legal? I'd be happy to fail to load it. There might be sysfs issues with it too. > However, there's a problem with commit > 6d76013381ed28979cd122eb4b249a88b5e384fa in that if you fail to allocate > a mod->sect_attrs (in this case it's null because of the duplication), > it still gets used without checking in add_notes_attrs() > > This should fix it No, the real problem is that it ignores failure. I'd much rather fail the module load than various features mysteriously MIA. Which brings us to "patches which don't go thru the maintainer" (or perhaps, non-responsive maintainers who get bypassed). Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2009-08-18 at 12:48 +0930, Rusty Russell wrote: > On Tue, 18 Aug 2009 08:19:36 am James Bottomley wrote: > > The root cause is a duplicate section name (.text); is this legal? > > I'd be happy to fail to load it. There might be sysfs issues with it > too. Well, that's why I want clarification. The bad code has been in since 2007 so it looks like a recent change, probably the one to more default linker scripts is the cause. > > However, there's a problem with commit > > 6d76013381ed28979cd122eb4b249a88b5e384fa in that if you fail to allocate > > a mod->sect_attrs (in this case it's null because of the duplication), > > it still gets used without checking in add_notes_attrs() > > > > This should fix it > > No, the real problem is that it ignores failure. I'd much rather fail > the module load than various features mysteriously MIA. There are two separate problems. One is why the the module has duplicate sections. Under my reading of the ELF spec, they seem to be allowable ... however we control the linker scripts and I believe we shouldn't have generated them. The other is the missing error handling in the module loader. The question of whether this is a generic failure that needs load refusal or an expected artifact of our new linker scripts needs investigating. > Which brings us to "patches which don't go thru the maintainer" (or > perhaps, non-responsive maintainers who get bypassed). Well, the original code was in 2007, so it's probably a bit late for a postmortem. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I'd be happy to fail to load it. There might be sysfs issues with it too. That sounds reasonable to me. And I'd be happy to at least look a little and maybe give some advice to anybody who finds themself building such a (free) module, doesn't know why or how it got that way, and wants to ask. > No, the real problem is that it ignores failure. I'd much rather fail > the module load than various features mysteriously MIA. In that regard, I just made add_notes_attrs() follow the model of add_sect_attrs(), which (gracefully) ignores all its failures. I don't know what the thought behind that was. My only guess was that since this is all CONFIG_KALLSYMS-only features, that someone thought turning on CONFIG_KALLSYMS should not add new ways to lose that weren't there before, only new ways to lose the new features that weren't there before either. Having these other alloc/sysfs failures cause the module load to fail would certainly be fine with me. Thanks, Roland -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[resending, fluffed reply-all] On Mon, 2009-08-17 at 22:06 -0700, Roland McGrath wrote: > > I'd be happy to fail to load it. There might be sysfs issues with it too. > > That sounds reasonable to me. And I'd be happy to at least look a little > and maybe give some advice to anybody who finds themself building such a > (free) module, doesn't know why or how it got that way, and wants to ask. Actually, for parisc, its not reasonable. It's expected that our modules have multiple text sections (we have to use -ffunction-sections to generate them in order that the PCREL17 jump stubs can be interleaved). The problem looks to be that some linker error gave the one of the named function text sections a duplicate name. Helge, can you post he objdump info that shows which section had a duplicate name? Even with the duplicate name, though, the module should be perfectly loadable. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Actually, for parisc, its not reasonable. It's expected that our modules > have multiple text sections (we have to use -ffunction-sections to > generate them in order that the PCREL17 jump stubs can be interleaved). I don't think you need what you think you need. Having lots of sections in your .o's when you compile is fine. These should be combined by the linker script that creates the .ko, however. Unless I am missing something, there is no purpose to this section distinction at insmod time--it's only important for the relative layout of the parts of the .ko's text, which winds up contiguous whether laid out that way at ld -r (.ko creation) time or at insmod time. > Even with the duplicate name, though, the module should be perfectly > loadable. But its /sys/module/foo/sections/ virtual directory becomes useless, as a single name space can no longer describe what sections it has. So perhaps it is then proper for add_sect_attrs() to punt on it. But that reduces the functionality you get from CONFIG_KALLSYMS. Thanks, Roland -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2009-08-18 at 17:14 -0700, Roland McGrath wrote: > > Actually, for parisc, its not reasonable. It's expected that our modules > > have multiple text sections (we have to use -ffunction-sections to > > generate them in order that the PCREL17 jump stubs can be interleaved). > > I don't think you need what you think you need. Having lots of sections in > your .o's when you compile is fine. These should be combined by the linker > script that creates the .ko, however. Unless I am missing something, there > is no purpose to this section distinction at insmod time--it's only > important for the relative layout of the parts of the .ko's text, which > winds up contiguous whether laid out that way at ld -r (.ko creation) time > or at insmod time. Actually, I think we do; the module loader is a runtime linker, after all. We have specific module bugs we fixed by inserting relocation stubs between the text sections. Our specific problem is that the standard pa relocation is PCREL17 ... as you can appreciate, 17 bits of relative jump isn't a lot. If the target isn't within the 17 bits, we have to insert a relocation stub to do an indirect absolute jump thought the GOT. Our problems occur when a text section is wider than the 17 bits, which happens a lot in the larger modules ... with nowhere to put the stub within range of the relocation we can't load them. Our fix is to split the text sections with -ffunction-sections so we can be pretty much assured of having a stub location within the 17 bits. We don't care what they're called or anything. We just care that the .text is split up into multiple separately relocateable sections so we can get the stubs within range of the jumps. Now, of course, if the final linker could be persuaded to sprinkle needed stubs through the text section and all we have to do is GOT relocations, we don't need all the jiggery-pokery ... but I'm told this can't be done. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Actually, I think we do; the module loader is a runtime linker, after > all. [...] Indeed you do. I've just read some of the parts of ld that normally address this issue for HPPA. They don't run for ld -r. So this is just another fine example of the lunacy of the ET_REL .ko madness that would be naturally avoided by a sensible tweaked ET_DYN scheme. But that battle was lost way, way back in the long, long ago, so long ago they were probably even still making HPPA machines then. > Now, of course, if the final linker could be persuaded to sprinkle > needed stubs through the text section and all we have to do is GOT > relocations, we don't need all the jiggery-pokery ... but I'm told this > can't be done. Not with ld -r as it is today. That's what ld does for you in proper final links. It looks to me like you might be able to enable some special mode ("finalish link" for -r) with a hack to HPPA ld to apply this stub-creation logic based on the assumption that the symbols in the relocs will be resolved to themselves, and barf on you if they're used for SHN_UNDEF symbols. But nobody cares enough to fiddle with ld. Thanks, Roland -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2009-08-18 at 18:31 -0700, Roland McGrath wrote: > > Actually, I think we do; the module loader is a runtime linker, after > > all. [...] > > Indeed you do. I've just read some of the parts of ld that normally > address this issue for HPPA. They don't run for ld -r. So this is just > another fine example of the lunacy of the ET_REL .ko madness that would be > naturally avoided by a sensible tweaked ET_DYN scheme. Using ET_DYN would have made our life easier when trying to code the kernel module loader as well. The basic problem is, of course, that this is simple on an x86, so it didn't matter that much for the initial implementation. It just becomes less simple on anything else. > But that battle was > lost way, way back in the long, long ago, so long ago they were probably > even still making HPPA machines then. Well, since they're still making parisc machines, I assume you mean further back than when the production lines knocked off today? > > Now, of course, if the final linker could be persuaded to sprinkle > > needed stubs through the text section and all we have to do is GOT > > relocations, we don't need all the jiggery-pokery ... but I'm told this > > can't be done. > > Not with ld -r as it is today. That's what ld does for you in proper final > links. It looks to me like you might be able to enable some special mode > ("finalish link" for -r) with a hack to HPPA ld to apply this stub-creation > logic based on the assumption that the symbols in the relocs will be > resolved to themselves, and barf on you if they're used for SHN_UNDEF symbols. > But nobody cares enough to fiddle with ld. So that leaves us stuck with the current implementation and still needing a solution for the duplicate section names? James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Using ET_DYN would have made our life easier when trying to code the > kernel module loader as well. The basic problem is, of course, that > this is simple on an x86, so it didn't matter that much for the initial > implementation. It just becomes less simple on anything else. There are generic ways that ET_DYN is better for all, too. A little birdie told me it was all the fault of mips. Anyway, water under the bridge. > Well, since they're still making parisc machines, I assume you mean > further back than when the production lines knocked off today? I'm pretty sure you knew what I meant. > So that leaves us stuck with the current implementation and still > needing a solution for the duplicate section names? Something like that. Your fix makes module loading work again, so we could just let /sys/module/*/{sections,notes}/ be missing on parisc and see how long it takes anybody to complain, assuming you talk Rusty out of his preference to change those error cases to refuse to load the module. OTOH, you said earlier that it was only some mistake that produced multiple same-named sections to begin with, so you could just try to fix that and then forget about it. Finally, we could decide to really support the general case including duplicate section names in these features. That's really not very hard to do, if we just completely change the interfaces for those features. e.g. instead of a /sys/module/*/sections directory, it could just be a single file that contains lines of: SHNDX 0xADDR NAME For /sys/module/*/notes, no consumer really cares about the section distinctions at all. It would be easy enough just to have a single pseudo-file that delivers all note sections concatenated together. (In practice, there is usually only one note section at most anyway.) Of course, such changes would be a new incompatible change to the userland sysfs ABI, require compatibility concerns, etc. It seems unlikely anyone really wants to bother with all that. It would be more natural presentation of the info IMHO, but it hardly matters. Thanks, Roland -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Mon, 2009-08-17 at 22:06 -0700, Roland McGrath wrote: > > > I'd be happy to fail to load it. There might be sysfs issues with it > too. > > > > That sounds reasonable to me. And I'd be happy to at least look a > little > > and maybe give some advice to anybody who finds themself building such a > > (free) module, doesn't know why or how it got that way, and wants to > ask. > > Actually, for parisc, its not reasonable. It's expected that our > modules have multiple text sections (we have to use -ffunction-sections > to generate them in order that the PCREL17 jump stubs can be > interleaved). > > The problem looks to be that some linker error gave the one of the named > function text sections a duplicate name. Helge, can you post he objdump > info that shows which section had a duplicate name? > > Even with the duplicate name, though, the module should be perfectly > loadable. It's the ac97_bus kernel module: -rw-r--r-- 1 root root 3.0K 2009-08-19 12:25 ac97_bus.ko "objdump -x ac97_bus.ko" shows two .text sections: ac97_bus.ko: file format elf32-hppa-linux ac97_bus.ko architecture: hppa1.1, flags 0x00000011: HAS_RELOC, HAS_SYMS start address 0x00000000 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 .exit.text 00000030 00000000 00000000 00000074 2**2 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE 4 .init.text 00000030 00000000 00000000 000000a4 2**2 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE 5 .text 00000000 00000000 00000000 000000d4 2**0 CONTENTS, ALLOC, LOAD, READONLY, CODE 6 .rodata.str1.4 00000008 00000000 00000000 000000d4 2**2 CONTENTS, ALLOC, LOAD, READONLY, DATA 7 .modinfo 00000040 00000000 00000000 000000dc 2**2 CONTENTS, ALLOC, LOAD, READONLY, DATA 8 __ksymtab 00000008 00000000 00000000 0000011c 2**2 CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA 9 __ksymtab_strings 0000000e 00000000 00000000 00000124 2**0 CONTENTS, ALLOC, LOAD, READONLY, DATA 10 .PARISC.unwind 00000030 00000000 00000000 00000134 2**2 CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA 11 .data 00000034 00000000 00000000 00000164 2**2 CONTENTS, ALLOC, LOAD, RELOC, DATA 12 .gnu.linkonce.this_module 00000160 00000000 00000000 00000198 2**2 CONTENTS, ALLOC, LOAD, RELOC, DATA, LINK_ONCE_DISCARD 13 .bss 00000000 00000000 00000000 000002f8 2**0 ALLOC 14 .comment 0000003a 00000000 00000000 000002f8 2**0 CONTENTS, READONLY SYMBOL TABLE: 00000000 l d .note.gnu.build-id 00000000 .note.gnu.build-id 00000000 l d .text 00000000 .text 00000000 l d .text.ac97_bus_match 00000000 .text.ac97_bus_match 00000000 l d .exit.text 00000000 .exit.text 00000000 l d .init.text 00000000 .init.text 00000000 l d .text 00000000 .text 00000000 l d .rodata.str1.4 00000000 .rodata.str1.4 00000000 l d .modinfo 00000000 .modinfo 00000000 l d __ksymtab 00000000 __ksymtab 00000000 l d __ksymtab_strings 00000000 __ksymtab_strings 00000000 l d .PARISC.unwind 00000000 .PARISC.unwind 00000000 l d .data 00000000 .data 00000000 l d .gnu.linkonce.this_module 00000000 .gnu.linkonce.this_module 00000000 l d .bss 00000000 .bss 00000000 l d .comment 00000000 .comment 00000000 l F .text.ac97_bus_match 0000001c ac97_bus_match 00000000 l F .exit.text 00000030 ac97_bus_exit 00000000 l F .init.text 00000030 ac97_bus_init 00000000 l O .modinfo 0000000c __mod_license77 00000000 l O __ksymtab 00000008 __ksymtab_ac97_bus_type 00000000 l O __ksymtab_strings 0000000e __kstrtab_ac97_bus_type 0000000c l O .modinfo 00000009 __module_depends 00000018 l O .modinfo 00000026 __mod_vermagic5 00000000 g O .gnu.linkonce.this_module 00000160 __this_module 00000000 g F .exit.text 00000030 cleanup_module 00000000 g F .init.text 00000030 init_module 00000000 *UND* 00000000 bus_unregister 00000000 g O .data 00000034 ac97_bus_type 00000000 *UND* 00000000 bus_register RELOCATION RECORDS FOR [.exit.text]: OFFSET TYPE VALUE 00000010 R_PARISC_DPREL21L ac97_bus_type 00000014 R_PARISC_DPREL14R ac97_bus_type 00000018 R_PARISC_PCREL17F bus_unregister RELOCATION RECORDS FOR [.init.text]: OFFSET TYPE VALUE 00000010 R_PARISC_DPREL21L ac97_bus_type 00000014 R_PARISC_DPREL14R ac97_bus_type 00000018 R_PARISC_PCREL17F bus_register RELOCATION RECORDS FOR [__ksymtab]: OFFSET TYPE VALUE 00000000 R_PARISC_DIR32 ac97_bus_type 00000004 R_PARISC_DIR32 __ksymtab_strings RELOCATION RECORDS FOR [.PARISC.unwind]: OFFSET TYPE VALUE 00000000 R_PARISC_SEGREL32 ac97_bus_match 00000004 R_PARISC_SEGREL32 .text.ac97_bus_match+0x00000018 00000010 R_PARISC_SEGREL32 ac97_bus_exit 00000014 R_PARISC_SEGREL32 .exit.text+0x0000002c 00000020 R_PARISC_SEGREL32 ac97_bus_init 00000024 R_PARISC_SEGREL32 .init.text+0x0000002c RELOCATION RECORDS FOR [.data]: OFFSET TYPE VALUE 00000000 R_PARISC_DIR32 .rodata.str1.4 00000010 R_PARISC_PLABEL32 ac97_bus_match RELOCATION RECORDS FOR [.gnu.linkonce.this_module]: OFFSET TYPE VALUE 000000d4 R_PARISC_PLABEL32 init_module 00000150 R_PARISC_PLABEL32 cleanup_module Helge
> The root cause is a duplicate section name (.text); is this legal? > > However, there's a problem with commit > 6d76013381ed28979cd122eb4b249a88b5e384fa in that if you fail to allocate > a mod->sect_attrs (in this case it's null because of the duplication), > it still gets used without checking in add_notes_attrs() > > This should fix it > > Signed-off-by: James Bottomley <James.Bottomley@suse.de> Thanks! I tested it, and it does at least fix the kernel crash. Tested-by: Helge Deller <deller@gmx.de> > diff --git a/kernel/module.c b/kernel/module.c > index fd14114..a703c49 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -2353,7 +2353,8 @@ static noinline struct module *load_module(void > __user *umod, > if (err < 0) > goto unlink; > add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs); > - add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs); > + if (mod->sect_attrs) > + add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs); > > /* Get rid of temporary copy */ > vfree(hdr); >
> > On Mon, 2009-08-17 at 22:06 -0700, Roland McGrath wrote: > > > > I'd be happy to fail to load it. There might be sysfs issues with > it > > too. > > > > > > That sounds reasonable to me. And I'd be happy to at least look a > > little > > > and maybe give some advice to anybody who finds themself building such > a > > > (free) module, doesn't know why or how it got that way, and wants to > > ask. > > > > Actually, for parisc, its not reasonable. It's expected that our > > modules have multiple text sections (we have to use -ffunction-sections > > to generate them in order that the PCREL17 jump stubs can be > > interleaved). > > > > The problem looks to be that some linker error gave the one of the named > > function text sections a duplicate name. Helge, can you post he objdump > > info that shows which section had a duplicate name? > > > > Even with the duplicate name, though, the module should be perfectly > > loadable. > > > It's the ac97_bus kernel module: > -rw-r--r-- 1 root root 3.0K 2009-08-19 12:25 ac97_bus.ko That's actually a problem of all kernel modules on hppa when using a newer compiler, not only the ac97_bus module. The reason seems to be, that something in the newer gcc compilers changed to generate multiple sections all named ".text" for the PCREL17 relocations. Older compilers named those sections ".text.1", ".text.2", ".text.3" and so forth. "objdump -x ac97_bus.ko" with current (newer) compiler 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 .exit.text 00000030 00000000 00000000 00000074 2**2 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > 4 .init.text 00000030 00000000 00000000 000000a4 2**2 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > 5 .text 00000000 00000000 00000000 000000d4 2**0 > CONTENTS, ALLOC, LOAD, READONLY, CODE >... older compiler produced: Sections: Idx Name Size VMA LMA File off Algn 0 .text 00000000 00000000 00000000 00000034 2**0 CONTENTS, ALLOC, LOAD, READONLY, CODE 1 .text.ac97_bus_match 0000001c 00000000 00000000 00000034 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE 2 .exit.text 00000030 00000000 00000000 00000050 2**2 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE 3 .init.text 00000030 00000000 00000000 00000080 2**2 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE 4 .text.1 00000000 00000000 00000000 000000b0 2**0 CONTENTS, ALLOC, LOAD, READONLY, CODE ... Helge
On Wed, 19 Aug 2009 09:39:24 am James Bottomley wrote: > Even with the duplicate name, though, the module should be perfectly > loadable. In a perfect world, yes, but there are places which assume they'll be unique and I always thought that reasonable. Front of my mind is /sys/module/<MODNAME/sections/ which has one file per section. Let's figure out how it happened tho; I'd rather fail cleanly than break subtly and horribly later... Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 19 Aug 2009 11:08:36 am James Bottomley wrote: > On Tue, 2009-08-18 at 18:31 -0700, Roland McGrath wrote: > > > Actually, I think we do; the module loader is a runtime linker, after > > > all. [...] > > > > Indeed you do. I've just read some of the parts of ld that normally > > address this issue for HPPA. They don't run for ld -r. So this is just > > another fine example of the lunacy of the ET_REL .ko madness that would be > > naturally avoided by a sensible tweaked ET_DYN scheme. > > Using ET_DYN would have made our life easier when trying to code the > kernel module loader as well. The basic problem is, of course, that > this is simple on an x86, so it didn't matter that much for the initial > implementation. It just becomes less simple on anything else. Actually, x86 was one of the archs which fucked us. Richard Henderson and I *had* this, but ld -shared without -fPIC helpfully tells you "you're doing it wrong" on x86-64. There were other issues, ISTR MIPS was a showstopper. Google finds the following summary I wrote when this stuff was fresher: http://lkml.org/lkml/2003/1/12/271 : While ET_DYN modules are a reasonably serious win for ia64 (and probably hppa) (ie. -300 lines or so), they're a minor win for alpha and ppc64 (-100 lines or so), and no real change for arm, i386, ppc, sparc, and sparc64. It's a lose for x86_64 (toolchain fixes, unless they want to use -fPIC for modules), mips and mips64 (major toolchain fixes, unless they want to use -fPIC for modules and stop using r28 for current inside modules). > > But that battle was > > lost way, way back in the long, long ago, so long ago they were probably > > even still making HPPA machines then. This isn't quite true; userspace should handle ET_DYN fine (at least, it was supposed to). So you could change any arch to use that, but it's a fair refactor if we leave some archs behind. If anyone's really interested, I can dig out the bits I have... > So that leaves us stuck with the current implementation and still > needing a solution for the duplicate section names? If this is not a "don't do that" bug, we could try hacking around it in parisc's module_arch_frob_sections? Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Actually, x86 was one of the archs which fucked us. Richard Henderson and > I *had* this, but ld -shared without -fPIC helpfully tells you "you're doing > it wrong" on x86-64. This complaint could easily be (have been) made optional. > There were other issues, ISTR MIPS was a showstopper. Richard told me it's all MIPS' fault, but I was being discreet. Thanks, Roland -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 20 August 2009 08:25:37 Helge Deller wrote: > The reason seems to be, that something in the newer gcc compilers changed > to generate multiple sections all named ".text" for the PCREL17 > relocations. Older compilers named those sections ".text.1", ".text.2", > ".text.3" and so forth. we saw this on Blackfin some time ago and it was tracked back to attribute mismatches in assembly files: http://blackfin.uclinux.org/gf/tracker/3638 http://blackfin.uclinux.org/gf/project/linux-kernel/scmsvn/?action=browse&path=/trunk/arch/blackfin/mach- common/entry.S&r1=3898&r2=3897&pathrev=3898 when `ld -r` ran, the sections named .text couldnt be combined due to different attributes so ld just added the .# suffix for us if you have scanelf installed (from pax-utils), that might help track back the source object ... `scanelf -qrk .text.1 ./` -mike
On Wed, 26 Aug 2009 04:54:32 am Roland McGrath wrote: > > Actually, x86 was one of the archs which fucked us. Richard Henderson and > > I *had* this, but ld -shared without -fPIC helpfully tells you "you're doing > > it wrong" on x86-64. > > This complaint could easily be (have been) made optional. Yep, I even had a patch. And meanwhile we could have lived with -fPIC modules with some loss of performance (I think). But at some level the problems accumulate until you go "nice idea, let's go do something else" :) But don't let me discourage you! Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> But at some level the problems accumulate until you go "nice idea, let's go > do something else" :) Yeah, done been gone. > But don't let me discourage you! I've already spent too much time making ET_REL .ko DWARF parsing work to be very motivated to do things right so all that work would never have been necessary like I've been grumbling about the whole time. ;-) Thanks, Roland -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/module.c b/kernel/module.c index fd14114..a703c49 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2353,7 +2353,8 @@ static noinline struct module *load_module(void __user *umod, if (err < 0) goto unlink; add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs); - add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs); + if (mod->sect_attrs) + add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs); /* Get rid of temporary copy */ vfree(hdr);