Skip to contentSkip to conversation

2.5-bk AT_GID clash

DISCUSSION Anders Gustafsson 6 participants 12 messages Nov 12, 2002 – Nov 14, 2002

Subsystems (2)
SPARC + UltraSPARC (sparc/sparc64) Module support
lore
Share & tools
lore

Anders Gustafsson

author
#0 lore raw

Hi,

the new module-api making module.h including elf.h have exposed a name clash in xfs:

include/linux/elf.h:175:#define AT_GID 13 /* real gid */
fs/xfs/linux/xfs_vnode.h:547:#define AT_GID 0x00000008

Can one be renamed?

Maybe module.h shouldn't be including elf.h, that afaik is needed by the arch-specific module loaders and not by all modules. A split into module.h for the modules and moduleloader.h for the arch-spec-loaders?

Signature · 1 line
Anders Gustafsson - [email protected] - http://0x63.nu/
#1 lore raw

In message <20021112011858.GB19877@gagarin> you write:

Quoted 9 lines

Hi,

the new module-api making module.h including elf.h have exposed a name clash in xfs:

include/linux/elf.h:175:#define AT_GID 13 /* real gid */
fs/xfs/linux/xfs_vnode.h:547:#define AT_GID 0x00000008

Can one be renamed?

Probably should be. I don't use AT_GID from memory, maybe somewhere else in the kernel is.

Quoted 3 lines

Maybe module.h shouldn't be including elf.h, that afaik is needed by the arch-specific module loaders and not by all modules. A split into module.h for the modules and moduleloader.h for the arch-spec-loaders?

This might be OK too, but in practice I don't think much will be in moduleloader.h: asm/module.h only really defines struct mod_arch_specific, which is embedded in struct module, and struct module needs to be exposed for those inlines...

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#2 lore raw
Quoted 14 lines · Rusty Russell

In message <20021112011858.GB19877@gagarin> you write:

Quoted 9 lines

Hi,

the new module-api making module.h including elf.h have exposed a name clash in xfs:

include/linux/elf.h:175:#define AT_GID 13 /* real gid */
fs/xfs/linux/xfs_vnode.h:547:#define AT_GID 0x00000008

Can one be renamed?

Probably should be. I don't use AT_GID from memory, maybe somewhere else in the kernel is.

Quoted 8 lines
Quoted 3 lines

Maybe module.h shouldn't be including elf.h, that afaik is needed by the arch-specific module loaders and not by all modules. A split into module.h for the modules and moduleloader.h for the arch-spec-loaders?

This might be OK too, but in practice I don't think much will be in moduleloader.h: asm/module.h only really defines struct mod_arch_specific, which is embedded in struct module, and struct module needs to be exposed for those inlines...

But does everyone who wants to implement a module need to be exposed to all the details of the elf header?

Steve

Signature · 2 lines
Steve Lord                                      voice: +1-651-683-3511
Principal Engineer, Filesystem Software         email: [email protected]
#3 lore raw

In message <[email protected]> you write:

Quoted 8 lines · Rusty Russell
Quoted 4 lines

This might be kOK too, but in practice I don't think much will be in moduleloader.h: asm/module.h only really defines struct mod_arch_specific, which is embedded in struct module, and struct module needs to be exposed for those inlines...

But does everyone who wants to implement a module need to be exposed to all the details of the elf header?

Well, linux/module.h -> asm/module.h -> linux/elf.h. Although if you use #define instead of typedef you can break the last link. Feel free to send a patch to split it into moduleload.h or something, but I think it'll look tiny.

But IMHO the nameclash needs to be fixed *anyway*, not hacked around, or someone else will run over it one day. AFAICT, changing fs/binfmt_elf.c and elf.h to AT_RGID is the simplest. Both should be mildly chastised for using a prefix like AT_ publically.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#4 lore raw
Quoted 10 lines · Rusty Russell

In message <[email protected]> you write:

Quoted 3 lines

But does everyone who wants to implement a module need to be exposed to all the details of the elf header?

Well, linux/module.h -> asm/module.h -> linux/elf.h. Although if you use #define instead of typedef you can break the last link. Feel free to send a patch to split it into moduleload.h or something, but I think it'll look tiny.

Quoted 4 lines

But IMHO the nameclash needs to be fixed *anyway*, not hacked around, or someone else will run over it one day. AFAICT, changing fs/binfmt_elf.c and elf.h to AT_RGID is the simplest. Both should be mildly chastised for using a prefix like AT_ publically.

FWIW, we changed XFS earlier today - it will go to Linus in the next batch of XFS mods. We're now using an XFS_AT_* convention instead.

cheers.

Signature · 1 line
Nathan
author
#5 lore raw

linux/elf.h vs linux/module.h [was: 2.5-bk AT_GID clash]

In reply to Rusty Russell
Quoted 7 lines · Rusty Russell

In message <[email protected]> you write:

Quoted 6 lines · Rusty Russell
Quoted 4 lines

This might be kOK too, but in practice I don't think much will be in moduleloader.h: asm/module.h only really defines struct mod_arch_specific, which is embedded in struct module, and struct module needs to be exposed for those inlines...

Quoted 7 lines
Quoted 2 lines

But does everyone who wants to implement a module need to be exposed to all the details of the elf header?

Well, linux/module.h -> asm/module.h -> linux/elf.h. Although if you use #define instead of typedef you can break the last link. Feel free to send a patch to split it into moduleload.h or something, but I think it'll look tiny.

At least for i386 there is no inclusion of elf.h from asm/module.h, and they are already defines. And a quick grep in the other arches shows no direct include of elf.h. So removing elf.h from module.h should relieve all sourcefiles[*] including module.h implicitly including elf.h too, which imho is a good thing.

[*] find . -name "*.c" | xargs grep "include <linux/module.h>" | wc -l
2479

Signature · 1 line
Anders Gustafsson - [email protected] - http://0x63.nu/
#6 lore raw

Re: linux/elf.h vs linux/module.h [was: 2.5-bk AT_GID clash]

In message <20021113111744.GA10014@gagarin> you write:

Quoted 11 lines · Rusty Russell
Quoted 10 lines

In message <[email protected]> you write:

Quoted 8 lines · Rusty Russell
Quoted 4 lines

This might be kOK too, but in practice I don't think much will be in moduleloader.h: asm/module.h only really defines struct mod_arch_specific, which is embedded in struct module, and struct module needs to be exposed for those inlines...

But does everyone who wants to implement a module need to be exposed to all the details of the elf header?

Quoted 7 lines
Quoted 4 lines

Well, linux/module.h -> asm/module.h -> linux/elf.h. Although if you use #define instead of typedef you can break the last link. Feel free to send a patch to split it into moduleload.h or something, but I think it'll look tiny.

At least for i386 there is no inclusion of elf.h from asm/module.h, and they are already defines.

I know, I wrote them. But a header should include all the files needed to use it, as a general rule.

I'm looking at the moduleloader.h patch, and I think it looks sane (the archs need to #include it, too).

Cheers,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#7 lore raw
In reply to Rusty Russell
Quoted 4 lines · Rusty Russell

But IMHO the nameclash needs to be fixed *anyway*, not hacked around, or someone else will run over it one day. AFAICT, changing fs/binfmt_elf.c and elf.h to AT_RGID is the simplest. Both should be mildly chastised for using a prefix like AT_ publically.

The name (AT_GID) is mandated by the ABI specification IIRC.

Paul.

#8 lore raw
In reply to Rusty Russell
Quoted 14 lines · Rusty Russell

In message <20021112011858.GB19877@gagarin> you write:

Quoted 9 lines

Hi,

the new module-api making module.h including elf.h have exposed a name clash in xfs:

include/linux/elf.h:175:#define AT_GID 13 /* real gid */
fs/xfs/linux/xfs_vnode.h:547:#define AT_GID 0x00000008

Can one be renamed?

Probably should be. I don't use AT_GID from memory, maybe somewhere else in the kernel is.

Quoted 8 lines
Quoted 3 lines

Maybe module.h shouldn't be including elf.h, that afaik is needed by the arch-specific module loaders and not by all modules. A split into module.h for the modules and moduleloader.h for the arch-spec-loaders?

This might be OK too, but in practice I don't think much will be in moduleloader.h: asm/module.h only really defines struct mod_arch_specific, which is embedded in struct module, and struct module needs to be exposed for those inlines...

If it's for nothing else it's at least worth not exposing elf details to every single file in the kernel..

author
#9 lore raw
In reply to Rusty Russell
Quoted 8 lines · Rusty Russell
Quoted 3 lines

Maybe module.h shouldn't be including elf.h, that afaik is needed by the arch-specific module loaders and not by all modules. A split into module.h for the modules and moduleloader.h for the arch-spec-loaders?

This might be OK too, but in practice I don't think much will be in moduleloader.h: asm/module.h only really defines struct mod_arch_specific, which is embedded in struct module, and struct module needs to be exposed for those inlines...

But there are things in linux/module.h that are arch-generic but not needed for the modules if i understand it correctly, things that need elf.h:

find_symbol_internal, module_core_alloc, module_init_alloc, apply_relocate, apply_relocate_add and module_finalize

Something like this patch...

Signature · 185 lines
Anders Gustafsson - [email protected] - http://0x63.nu/

You can import this changeset into BK by piping this whole message to:
'| bk receive [path to repository]' or apply the patch as usual.

===================================================================


[email protected], 2002-11-12 23:44:23+01:00, [email protected]
  Do not include elf.h into all modules via module.h


 include/linux/module.h       |   45 --------------------------------------
 include/linux/moduleloader.h |   50 +++++++++++++++++++++++++++++++++++++++++++
 kernel/module.c              |    1 
 3 files changed, 51 insertions(+), 45 deletions(-)


diff -Nru a/include/linux/module.h b/include/linux/module.h
--- a/include/linux/module.h	Tue Nov 12 23:46:58 2002
+++ b/include/linux/module.h	Tue Nov 12 23:46:58 2002
@@ -10,7 +10,6 @@
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/list.h>
-#include <linux/elf.h>
 #include <linux/stat.h>
 #include <linux/compiler.h>
 #include <linux/cache.h>
@@ -143,50 +142,6 @@
 	   keeping pointers to this stuff */
 	char args[0];
 };
-
-/* Helper function for arch-specific module loaders */
-unsigned long find_symbol_internal(Elf_Shdr *sechdrs,
-				   unsigned int symindex,
-				   const char *strtab,
-				   const char *name,
-				   struct module *mod,
-				   struct kernel_symbol_group **group);
-
-/* These must be implemented by the specific architecture */
-
-/* vmalloc AND zero for the non-releasable code; return ERR_PTR() on error. */
-void *module_core_alloc(const Elf_Ehdr *hdr,
-			const Elf_Shdr *sechdrs,
-			const char *secstrings,
-			struct module *mod);
-
-/* vmalloc and zero (if any) for sections to be freed after init.
-   Return ERR_PTR() on error. */
-void *module_init_alloc(const Elf_Ehdr *hdr,
-			const Elf_Shdr *sechdrs,
-			const char *secstrings,
-			struct module *mod);
-
-/* Apply the given relocation to the (simplified) ELF.  Return -error
-   or 0. */
-int apply_relocate(Elf_Shdr *sechdrs,
-		   const char *strtab,
-		   unsigned int symindex,
-		   unsigned int relsec,
-		   struct module *mod);
-
-/* Apply the given add relocation to the (simplified) ELF.  Return
-   -error or 0 */
-int apply_relocate_add(Elf_Shdr *sechdrs,
-		       const char *strtab,
-		       unsigned int symindex,
-		       unsigned int relsec,
-		       struct module *mod);
-
-/* Any final processing of module before access.  Return -error or 0. */
-int module_finalize(const Elf_Ehdr *hdr,
-		    const Elf_Shdr *sechdrs,
-		    struct module *mod);
 
 /* Free memory returned from module_core_alloc/module_init_alloc */
 void module_free(struct module *mod, void *module_region);
diff -Nru a/include/linux/moduleloader.h b/include/linux/moduleloader.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/include/linux/moduleloader.h	Tue Nov 12 23:46:58 2002
@@ -0,0 +1,50 @@
+#ifndef _LINUX_MODULELOADER_H
+#define _LINUX_MODULELOADER_H
+
+#include <linux/elf.h>
+
+/* Helper function for arch-specific module loaders */
+unsigned long find_symbol_internal(Elf_Shdr *sechdrs,
+				   unsigned int symindex,
+				   const char *strtab,
+				   const char *name,
+				   struct module *mod,
+				   struct kernel_symbol_group **group);
+
+/* These must be implemented by the specific architecture */
+
+/* vmalloc AND zero for the non-releasable code; return ERR_PTR() on error. */
+void *module_core_alloc(const Elf_Ehdr *hdr,
+			const Elf_Shdr *sechdrs,
+			const char *secstrings,
+			struct module *mod);
+
+/* vmalloc and zero (if any) for sections to be freed after init.
+   Return ERR_PTR() on error. */
+void *module_init_alloc(const Elf_Ehdr *hdr,
+			const Elf_Shdr *sechdrs,
+			const char *secstrings,
+			struct module *mod);
+
+/* Apply the given relocation to the (simplified) ELF.  Return -error
+   or 0. */
+int apply_relocate(Elf_Shdr *sechdrs,
+		   const char *strtab,
+		   unsigned int symindex,
+		   unsigned int relsec,
+		   struct module *mod);
+
+/* Apply the given add relocation to the (simplified) ELF.  Return
+   -error or 0 */
+int apply_relocate_add(Elf_Shdr *sechdrs,
+		       const char *strtab,
+		       unsigned int symindex,
+		       unsigned int relsec,
+		       struct module *mod);
+
+/* Any final processing of module before access.  Return -error or 0. */
+int module_finalize(const Elf_Ehdr *hdr,
+		    const Elf_Shdr *sechdrs,
+		    struct module *mod);
+
+#endif
diff -Nru a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c	Tue Nov 12 23:46:58 2002
+++ b/kernel/module.c	Tue Nov 12 23:46:58 2002
@@ -17,6 +17,7 @@
 */
 #include <linux/config.h>
 #include <linux/module.h>
+#include <linux/moduleloader.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>

===================================================================


This BitKeeper patch contains the following changesets:
1.858
## Wrapped with gzip_uu ##


begin 664 bkpatch19251
M'XL(`.*$T3T``\U8;4_;2!#^C'_%2'P!>DEV_9:7'E5ID[X(CE9I.56Z.T6;
M]3BVZNQ&MD.A\H^_\=J$PR$AI<?I'!1CSXOG>69F=YQ]N,@P'>P)%6":S:Q]
M>*>S?+#'KGRGK99T/=::KCN1GF.GUNI\Q51ATIE^[4P3_<TBK8\BEQ%<DG2P
MQ]O.ZDY^O<#!WGCT]N+L9&Q9Q\?P.A)JAI\PA^-C*]?II4B"[*7(HT2K=IX*
ME<TQ%VVIY\5*M;`9L^GC\:[#/+_@/G.[A>0!Y\+E&##;[?FN58?WL@Z^8<\Y
MIY/'."M<MVL[UA!XN^?U@-D=SCO<!ML9N.[`=IXQ/F`,&N[@F0,M9KV"?S?H
MUY:$H0:E<XB53)8!`B9A.Z*K7(-($ICK8)E@!I>QJ/]O1]8I>$ZWYU@?;PFU
M6C]X6!83S'K1!%K4<7226"VO.M4C$RU(J1VMJ+1MU^6$RNUSM_`92KLOA=WW
MO2#LXT^X[!9>U_:[%-9VGN_S6/NJ2>^[K+"[S.\7KM_W^NC[#O?8U`GY3N$U
M`_,*S^UUV8.!U<U1.Y%W(G+ZA6?;GD.N?,^6GG!\QJ:]GK,6T7U>_DD[`6.F
MG[8Q6[;8.7Z#,$YPL%73^AU<:ZV!?KX03).Q9HMQ?U.+L:=H,0FOXOP4<8&I
MH0*VK6:=K32=@L%);;=5;0QK1'P!=M6S^2.:]*G2PG=/B_<D>7EMO0%>KF2F
MX3]`*_UF_@CT`_S^.(GO&7C,VH]#@A;"Y.S]^<67R6\?AA=GH[,/)\/1>/+.
MVB=1K'"#]$^RKA?H7ZN@S#K]@@2=(WB'B2FOI9)YK!6$.@61RJB5+5#&82SK
MI1LJ#!D<=:RERN*9PH#NJ1E5I@HFV?5\JI,)K?U4E"(Y&"7AY%,4I'"4H:1S
M]HNU1P<`K(Q)%\B,K/%J)95:93G(2)26>9J+Z;TB)>:X$I#>4N8W<1[1N2FJ
M.N4FR%FJEPLX.C+GP^<5#Y\CS!#F2WK$%"&>+Q*<(Z$)8'H->82PXJ-D)\Y1
MYLL42S:,^>6<=CPMX>1\"-\QU8;'TDQIU4HQ09&)*44G=8#/(44R5C`:CR<?
M/X\/#H&(QS35:;MT>*GCP,`@.!.I4YP8WP<5`26S(\,L?1F@M_?7&;_#)TKB
M(U:S2K1.VPT7-V"HG2HP!W%(%]>'!A6Y*2LEHZ8JJ0I3)))$2(FGE,9YVR+>
MQ[L#+&W^:X`GBT5297467Z*B?-#CA:E_`E7>/\C*$J!\8W`(H[,W[16FEL%1
M@B0NF,%35K(H74YJ1WA__6^L[VUMT931(\AE+=D=H@B"'X%9XJN0&ICWHYR0
MTXU(81M:>`#QFOP.:MB.7%V7:Y)(8)%JB5E&!0$ZO%&=(A4Q#7JR%#73>C>G
M=84:9_%WW%2?MT@W<K$AVGU401QN'(>J0>CIQLE;U^5<L;MC7AX]YGENX7+'
M]LRN;*]OR]Z6:<GU_D=O)&8^WF$??]0./N0.30M#[OK@NB;7C1GYX20_:C2W
M4MK,KE^6W[2)+$I';;'\X^9)?VT:U3GO,DINP?I=NUL-7/[N$Q=_JG?-$UK`
M&F\)(@-1#2LS5)BN9I56I0$1&L5R^#6O'8T,-_`_9CCC?4IM<[RZ&^6+VY\9
89(3R:[:<'T^=/F>N[%I_`[O-$U_'$```
`
end
#10 lore raw

In message <20021112225022.GA10689@gagarin> you write:

Quoted 7 lines

But there are things in linux/module.h that are arch-generic but not needed for the modules if i understand it correctly, things that need elf.h:

find_symbol_internal, module_core_alloc, module_init_alloc, apply_relocate, apply_relocate_add and module_finalize

Something like this patch...

You missed the arch's module.c and the module_free function. Does this work for you (untested here, snarfed off on your patch)?

Frankly, it's nice to clean some of the cruft out of module.h, too.

Thanks!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

arch/i386/kernel/module.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/arch/i386/kernel/module.c working-2.5-bk-noelf/arch/i386/kernel/module.c
@@ -15,7 +15,7 @@
     along with this program; if not, write to the Free Software
     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 */
-#include <linux/module.h>
+#include <linux/moduleloader.h>
 #include <linux/elf.h>
 #include <linux/vmalloc.h>
 #include <linux/fs.h>
arch/sparc/kernel/module.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/arch/sparc/kernel/module.c working-2.5-bk-noelf/arch/sparc/kernel/module.c
@@ -4,7 +4,7 @@
  * Copyright (C) 2002 David S. Miller.
  */
 
-#include <linux/module.h>
+#include <linux/moduleloader.h>
 #include <linux/kernel.h>
 #include <linux/elf.h>
 #include <linux/vmalloc.h>
arch/sparc64/kernel/module.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/arch/sparc64/kernel/module.c working-2.5-bk-noelf/arch/sparc64/kernel/module.c
@@ -4,7 +4,7 @@
  * Copyright (C) 2002 David S. Miller.
  */
 
-#include <linux/module.h>
+#include <linux/moduleloader.h>
 #include <linux/kernel.h>
 #include <linux/elf.h>
 #include <linux/vmalloc.h>
include/linux/module.h +0 -48

60 lines not on this page · raw message

include/linux/moduleloader.h +55 -0

55 lines not on this page · raw message

kernel/module.c +1 -0
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/kernel/module.c working-2.5-bk-noelf/kernel/module.c
@@ -17,6 +17,7 @@
 */
 #include <linux/config.h>
 #include <linux/module.h>
+#include <linux/moduleloader.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
author
#11 lore raw
Quoted 2 lines · Rusty Russell

You missed the arch's module.c and the module_free function. Does this work for you (untested here, snarfed off on your patch)?

Yes, works fine!

And I dont use any modules that need parameters, so the new in-kernel-moduleloader works great for me.

Signature · 1 line
Anders Gustafsson - [email protected] - http://0x63.nu/