[PATCH] parisc: fix module loading failure of large kernel modules (take 2)

[PATCH] parisc: fix module loading failure of large kernel modules (take 2)

[Helge Deller]

This is the second take of the patch series.
Changes to previous version:
- new CONFIG_HAVE_MODULE_SECTION_STUBS config option
- put stub entries of a code section in front of the section

____________
The parisc port (esp. the 32bit kernel) currently lacks the ability to
load large kernel modules like xfs or ipv6. This is a long outstanding
bug and has already been reported a few times, e.g.:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=350482,
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=401439,
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508489

The symptom is like this:
# modprobe xfs
FATAL: Error inserting xfs
(/lib/modules/2.6.26-1-parisc/kernel/fs/xfs/xfs.ko): Invalid module
format

In dmesg:
module xfs relocation of symbol xfs_btree_read_bufs is out of range
(0x3ffefffe in 17 bits)

The reason for the failure is, that the architecture only provides the
R_PARISC_PCREL17F (for 32bit kernels) and R_PARISC_PCREL22F (for PA2.0
and 64bit kernels) relocations, which sometimes can't reach the target
address of the stub entry if the kernel module is too large. Currently
parisc (like other architectures) creates one big PLT section for all
stubs at the beginning of the init and core sections.

The following two patches changes the parisc module loader to put stubs
for the code sections in front of each section, so that the distance to
the stubs more easily fits into the available 17/22 bits.

The first patch touches the generic module loader and adds a call to the
new module_additional_section_size() function to get_offset() if
CONFIG_HAVE_MODULE_SECTION_STUBS is defined. On parisc this
function returns the additional bytes for the stub area of a given section.

The second patch implements the parisc-specific changes.

Tested with 32- and 64bit parisc kernels.

Helge

[PATCH 1/2] module.c: fix module loading failure of large modules (take 2)

[Helge Deller]

[PATCH 1/2] module.c: fix module loading failure of large modules (take 2)

When creating the final layout of a kernel module in memory, allow the
module loader to reserve some additional memory in front of a given section.
This is currently only needed for the parisc port which needs to put the
stub entries there to fulfill the 17/22bit PCREL relocations with large
kernel modules like xfs.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index eb10339..65fb34c 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,9 @@ int module_frob_arch_sections(Elf_Ehdr *hdr,
 			      char *secstrings,
 			      struct module *mod);
 
+unsigned long module_additional_section_size(struct module *mod,
+					     unsigned int section);
+
 /* Allocator used for allocating struct module, core sections and init
    sections.  Returns NULL on failure. */
 void *module_alloc(unsigned long size);
diff --git a/init/Kconfig b/init/Kconfig
index f763762..5383815 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -908,6 +908,9 @@ config MODULE_SRCVERSION_ALL
 	  the version).  With this option, such a "srcversion" field
 	  will be created for all modules.  If unsure, say N.
 
+config HAVE_MODULE_SECTION_STUBS
+	bool
+
 config KMOD
 	def_bool y
 	help
diff --git a/kernel/module.c b/kernel/module.c
index 1f4cc00..4b86308 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1579,10 +1579,15 @@ static int simplify_symbols(Elf_Shdr *sechdrs,
 }
 
 /* Update size with this section: return offset. */
-static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
+static long get_offset(struct module *mod, unsigned int *size,
+		Elf_Shdr *sechdr, unsigned int section)
 {
 	long ret;
 
+#ifdef CONFIG_HAVE_MODULE_SECTION_STUBS
+	/* allocate some memory for stubs in front of each section */
+	*size += module_additional_section_size(mod, section);
+#endif
 	ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
 	*size = ret + sechdr->sh_size;
 	return ret;
@@ -1622,7 +1627,7 @@ static void layout_sections(struct module *mod,
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) == 0)
 				continue;
-			s->sh_entsize = get_offset(&mod->core_size, s);
+			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
 		if (m == 0)
@@ -1640,7 +1645,7 @@ static void layout_sections(struct module *mod,
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) != 0)
 				continue;
-			s->sh_entsize = (get_offset(&mod->init_size, s)
+			s->sh_entsize = (get_offset(mod, &mod->init_size, s, i)
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}

[PATCH 2/2] parisc: fix module loading failure of large modules (take 2)

[Helge Deller]

[PATCH 2/2] parisc: fix module loading failure of large modules (take 2)

On 32bit (and sometimes 64bit) and with big kernel modules like xfs or
ipv6 the relocation types R_PARISC_PCREL17F and R_PARISC_PCREL22F may
fail to reach their PLT stub if we only create one big stub array for
all sections at the beginning of the core or init section.

With this patch we now instead add individual PLT stub entries
directly in front of the code sections where the stubs are actually
called. This reduces the distance between the PCREL location and the
stub entry so that the relocations can be fulfilled.

The kernel module loader will call module_additional_section_size() and
request us to return the amount of additional memory we need for the
stubs of each section. This memory will then be allocated in front
of the code code segment when the kernel layouts the final addresses 
of all sections.

Tested with 32- and 64bit kernels.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 644a70b..f975e31 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -9,6 +9,7 @@ config PARISC
 	def_bool y
 	select HAVE_IDE
 	select HAVE_OPROFILE
+	select HAVE_MODULE_SECTION_STUBS
 	select RTC_CLASS
 	select RTC_DRV_PARISC
 	help
diff --git a/arch/parisc/include/asm/module.h b/arch/parisc/include/asm/module.h
index c2cb49e..1f41234 100644
--- a/arch/parisc/include/asm/module.h
+++ b/arch/parisc/include/asm/module.h
@@ -23,8 +23,10 @@ struct mod_arch_specific
 {
 	unsigned long got_offset, got_count, got_max;
 	unsigned long fdesc_offset, fdesc_count, fdesc_max;
-	unsigned long stub_offset, stub_count, stub_max;
-	unsigned long init_stub_offset, init_stub_count, init_stub_max;
+	struct {
+		unsigned long stub_offset;
+		unsigned int stub_entries;
+		} *section;
 	int unwind_section;
 	struct unwind_table *unwind;
 };
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index 44138c3..f8129ce 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -6,6 +6,7 @@
  *
  *    Linux/PA-RISC Project (http://www.parisc-linux.org/)
  *    Copyright (C) 2003 Randolph Chung <tausq at debian . org>
+ *    Copyright (C) 2008 Helge Deller <deller@gmx.de>
  *
  *
  *    This program is free software; you can redistribute it and/or modify
@@ -24,6 +25,20 @@
  *
  *
  *    Notes:
+ *    - PLT stub handling
+ *      On 32bit (and sometimes 64bit) and with big kernel modules like xfs or
+ *      ipv6 the relocation types R_PARISC_PCREL17F and R_PARISC_PCREL22F may
+ *      fail to reach its PLT stub if we only create one big stub array for
+ *      all sections at the beginning of the core or init section.
+ *      Instead we now insert individual PLT stub entries directly in front of
+ *      of the code sections where the stubs are actually called.
+ *      This reduces the distance between the PCREL location and the stub entry
+ *      so that the relocations can be fulfilled.
+ *      The kernel module loader will call module_additional_section_size()
+ *      and request us to return the amount of additional memory we need for
+ *      the stubs of each section. This memory will then be allocated in front
+ *      of the code code segment.
+ *
  *    - SEGREL32 handling
  *      We are not doing SEGREL32 handling correctly. According to the ABI, we
  *      should do a value offset, like this:
@@ -58,9 +73,13 @@
 #define DEBUGP(fmt...)
 #endif
 
+#define RELOC_REACHABLE(val, bits) \
+	(( ( !((val) & (1<<((bits)-1))) && ((val)>>(bits)) != 0 )  ||	\
+	     ( ((val) & (1<<((bits)-1))) && ((val)>>(bits)) != (((__typeof__(val))(~0))>>((bits)+2)))) ? \
+	0 : 1)
+
 #define CHECK_RELOC(val, bits) \
-	if ( ( !((val) & (1<<((bits)-1))) && ((val)>>(bits)) != 0 )  ||	\
-	     ( ((val) & (1<<((bits)-1))) && ((val)>>(bits)) != (((__typeof__(val))(~0))>>((bits)+2)))) { \
+	if (!RELOC_REACHABLE(val, bits)) { \
 		printk(KERN_ERR "module %s relocation of symbol %s is out of range (0x%lx in %d bits)\n", \
 		me->name, strtab + sym->st_name, (unsigned long)val, bits); \
 		return -ENOEXEC;			\
@@ -92,13 +111,6 @@ static inline int in_local(struct module *me, void *loc)
 	return in_init(me, loc) || in_core(me, loc);
 }
 
-static inline int in_local_section(struct module *me, void *loc, void *dot)
-{
-	return (in_init(me, loc) && in_init(me, dot)) ||
-		(in_core(me, loc) && in_core(me, dot));
-}
-
-
 #ifndef CONFIG_64BIT
 struct got_entry {
 	Elf32_Addr addr;
@@ -258,23 +270,42 @@ static inline unsigned long count_stubs(const Elf_Rela *rela, unsigned long n)
 /* Free memory returned from module_alloc */
 void module_free(struct module *mod, void *module_region)
 {
+	kfree(mod->arch.section);
+	mod->arch.section = NULL;
+
 	vfree(module_region);
 	/* FIXME: If module_region == mod->init_region, trim exception
            table entries. */
 }
 
+/* return number of additional bytes to reserve for a section */
+unsigned long module_additional_section_size(struct module *mod,
+					     unsigned int section)
+{
+	/* size needed for all stubs of this section (including
+	 * one additional for correct alignment of the stubs) */
+	return (mod->arch.section[section].stub_entries + 1)
+		* sizeof(struct stub_entry);
+}
+
 #define CONST 
 int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 			      CONST Elf_Shdr *sechdrs,
 			      CONST char *secstrings,
 			      struct module *me)
 {
-	unsigned long gots = 0, fdescs = 0, stubs = 0, init_stubs = 0;
+	unsigned long gots = 0, fdescs = 0, len;
 	unsigned int i;
 
+	len = hdr->e_shnum * sizeof(me->arch.section[0]);
+	me->arch.section = kzalloc(len, GFP_KERNEL);
+	if (!me->arch.section)
+		return -ENOMEM;
+
 	for (i = 1; i < hdr->e_shnum; i++) {
-		const Elf_Rela *rels = (void *)hdr + sechdrs[i].sh_offset;
+		const Elf_Rela *rels = (void *)sechdrs[i].sh_addr;
 		unsigned long nrels = sechdrs[i].sh_size / sizeof(*rels);
+		unsigned int count, s;
 
 		if (strncmp(secstrings + sechdrs[i].sh_name,
 			    ".PARISC.unwind", 14) == 0)
@@ -290,11 +321,23 @@ int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 		 */
 		gots += count_gots(rels, nrels);
 		fdescs += count_fdescs(rels, nrels);
-		if(strncmp(secstrings + sechdrs[i].sh_name,
-			   ".rela.init", 10) == 0)
-			init_stubs += count_stubs(rels, nrels);
-		else
-			stubs += count_stubs(rels, nrels);
+
+		/* XXX: By sorting the relocs and finding duplicate entries
+		 *  we could reduce the number of necessary stubs and save
+		 *  some memory. */
+		count = count_stubs(rels, nrels);
+		if (!count)
+			continue;
+
+		/* so we need relocation stubs. reserve necessary memory. */
+		/* sh_info gives the section for which we need to add stubs. */
+		s = sechdrs[i].sh_info;
+
+		/* each code section should only have one relocation section */
+		WARN_ON(me->arch.section[s].stub_entries);
+
+		/* store number of stubs we need for this section */
+		me->arch.section[s].stub_entries += count;
 	}
 
 	/* align things a bit */
@@ -306,18 +349,8 @@ int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 	me->arch.fdesc_offset = me->core_size;
 	me->core_size += fdescs * sizeof(Elf_Fdesc);
 
-	me->core_size = ALIGN(me->core_size, 16);
-	me->arch.stub_offset = me->core_size;
-	me->core_size += stubs * sizeof(struct stub_entry);
-
-	me->init_size = ALIGN(me->init_size, 16);
-	me->arch.init_stub_offset = me->init_size;
-	me->init_size += init_stubs * sizeof(struct stub_entry);
-
 	me->arch.got_max = gots;
 	me->arch.fdesc_max = fdescs;
-	me->arch.stub_max = stubs;
-	me->arch.init_stub_max = init_stubs;
 
 	return 0;
 }
@@ -380,23 +413,27 @@ enum elf_stub_type {
 };
 
 static Elf_Addr get_stub(struct module *me, unsigned long value, long addend,
-	enum elf_stub_type stub_type, int init_section)
+	enum elf_stub_type stub_type, Elf_Addr loc0, unsigned int targetsec)
 {
-	unsigned long i;
 	struct stub_entry *stub;
 
-	if(init_section) {
-		i = me->arch.init_stub_count++;
-		BUG_ON(me->arch.init_stub_count > me->arch.init_stub_max);
-		stub = me->module_init + me->arch.init_stub_offset + 
-			i * sizeof(struct stub_entry);
-	} else {
-		i = me->arch.stub_count++;
-		BUG_ON(me->arch.stub_count > me->arch.stub_max);
-		stub = me->module_core + me->arch.stub_offset + 
-			i * sizeof(struct stub_entry);
+	/* initialize stub_offset to point in front of the section */
+	if (!me->arch.section[targetsec].stub_offset) {
+		loc0 -= (me->arch.section[targetsec].stub_entries + 1) *
+				sizeof(struct stub_entry);
+		/* get correct alignment for the stubs */
+		loc0 = ALIGN(loc0, sizeof(struct stub_entry));
+		me->arch.section[targetsec].stub_offset = loc0;
 	}
 
+	/* get address of stub entry */
+	stub = (void *) me->arch.section[targetsec].stub_offset;
+	me->arch.section[targetsec].stub_offset += sizeof(struct stub_entry);
+
+	/* do not write outside available stub area */
+	BUG_ON(0 == me->arch.section[targetsec].stub_entries--);
+
+
 #ifndef CONFIG_64BIT
 /* for 32-bit the stub looks like this:
  * 	ldil L'XXX,%r1
@@ -489,15 +526,19 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 	Elf32_Addr val;
 	Elf32_Sword addend;
 	Elf32_Addr dot;
+	Elf_Addr loc0;
+	unsigned int targetsec = sechdrs[relsec].sh_info;
 	//unsigned long dp = (unsigned long)$global$;
 	register unsigned long dp asm ("r27");
 
 	DEBUGP("Applying relocate section %u to %u\n", relsec,
-	       sechdrs[relsec].sh_info);
+	       targetsec);
 	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
 		/* This is where to make the change */
-		loc = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+		loc = (void *)sechdrs[targetsec].sh_addr
 		      + rel[i].r_offset;
+		/* This is the start of the target section */
+		loc0 = sechdrs[targetsec].sh_addr;
 		/* This is the symbol it is referring to */
 		sym = (Elf32_Sym *)sechdrs[symindex].sh_addr
 			+ ELF32_R_SYM(rel[i].r_info);
@@ -569,19 +610,32 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			break;
 		case R_PARISC_PCREL17F:
 			/* 17-bit PC relative address */
-			val = get_stub(me, val, addend, ELF_STUB_GOT, in_init(me, loc));
+			/* calculate direct call offset */
+			val += addend;
 			val = (val - dot - 8)/4;
-			CHECK_RELOC(val, 17)
+			if (!RELOC_REACHABLE(val, 17)) {
+				/* direct distance too far, create
+				 * stub entry instead */
+				val = get_stub(me, sym->st_value, addend,
+					ELF_STUB_DIRECT, loc0, targetsec);
+				val = (val - dot - 8)/4;
+				CHECK_RELOC(val, 17);
+			}
 			*loc = (*loc & ~0x1f1ffd) | reassemble_17(val);
 			break;
 		case R_PARISC_PCREL22F:
 			/* 22-bit PC relative address; only defined for pa20 */
-			val = get_stub(me, val, addend, ELF_STUB_GOT, in_init(me, loc));
-			DEBUGP("STUB FOR %s loc %lx+%lx at %lx\n", 
-			       strtab + sym->st_name, (unsigned long)loc, addend, 
-			       val)
+			/* calculate direct call offset */
+			val += addend;
 			val = (val - dot - 8)/4;
-			CHECK_RELOC(val, 22);
+			if (!RELOC_REACHABLE(val, 22)) {
+				/* direct distance too far, create
+				 * stub entry instead */
+				val = get_stub(me, sym->st_value, addend,
+					ELF_STUB_DIRECT, loc0, targetsec);
+				val = (val - dot - 8)/4;
+				CHECK_RELOC(val, 22);
+			}
 			*loc = (*loc & ~0x3ff1ffd) | reassemble_22(val);
 			break;
 
@@ -610,13 +664,17 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 	Elf64_Addr val;
 	Elf64_Sxword addend;
 	Elf64_Addr dot;
+	Elf_Addr loc0;
+	unsigned int targetsec = sechdrs[relsec].sh_info;
 
 	DEBUGP("Applying relocate section %u to %u\n", relsec,
-	       sechdrs[relsec].sh_info);
+	       targetsec);
 	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
 		/* This is where to make the change */
-		loc = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+		loc = (void *)sechdrs[targetsec].sh_addr
 		      + rel[i].r_offset;
+		/* This is the start of the target section */
+		loc0 = sechdrs[targetsec].sh_addr;
 		/* This is the symbol it is referring to */
 		sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
 			+ ELF64_R_SYM(rel[i].r_info);
@@ -672,42 +730,40 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			DEBUGP("PCREL22F Symbol %s loc %p val %lx\n",
 			       strtab + sym->st_name,
 			       loc, val);
+			val += addend;
 			/* can we reach it locally? */
-			if(!in_local_section(me, (void *)val, (void *)dot)) {
-
-				if (in_local(me, (void *)val))
-					/* this is the case where the
-					 * symbol is local to the
-					 * module, but in a different
-					 * section, so stub the jump
-					 * in case it's more than 22
-					 * bits away */
-					val = get_stub(me, val, addend, ELF_STUB_DIRECT,
-						       in_init(me, loc));
-				else if (strncmp(strtab + sym->st_name, "$$", 2)
+			if (in_local(me, (void *)val)) {
+				/* this is the case where the symbol is local
+				 * to the module, but in a different section,
+				 * so stub the jump in case it's more than 22
+				 * bits away */
+				val = (val - dot - 8)/4;
+				if (!RELOC_REACHABLE(val, 22)) {
+					/* direct distance too far, create
+					 * stub entry instead */
+					val = get_stub(me, sym->st_value,
+						addend, ELF_STUB_DIRECT,
+						loc0, targetsec);
+				} else {
+					/* Ok, we can reach it directly. */
+					val = sym->st_value;
+					val += addend;
+				}
+			} else {
+				val = sym->st_value;
+				if (strncmp(strtab + sym->st_name, "$$", 2)
 				    == 0)
 					val = get_stub(me, val, addend, ELF_STUB_MILLI,
-						       in_init(me, loc));
+						       loc0, targetsec);
 				else
 					val = get_stub(me, val, addend, ELF_STUB_GOT,
-						       in_init(me, loc));
+						       loc0, targetsec);
 			}
 			DEBUGP("STUB FOR %s loc %lx, val %lx+%lx at %lx\n", 
 			       strtab + sym->st_name, loc, sym->st_value,
 			       addend, val);
-			/* FIXME: local symbols work as long as the
-			 * core and init pieces aren't separated too
-			 * far.  If this is ever broken, you will trip
-			 * the check below.  The way to fix it would
-			 * be to generate local stubs to go between init
-			 * and core */
-			if((Elf64_Sxword)(val - dot - 8) > 0x800000 -1 ||
-			   (Elf64_Sxword)(val - dot - 8) < -0x800000) {
-				printk(KERN_ERR "Module %s, symbol %s is out of range for PCREL22F relocation\n",
-				       me->name, strtab + sym->st_name);
-				return -ENOEXEC;
-			}
 			val = (val - dot - 8)/4;
+			CHECK_RELOC(val, 22);
 			*loc = (*loc & ~0x3ff1ffd) | reassemble_22(val);
 			break;
 		case R_PARISC_DIR64:
@@ -794,12 +850,8 @@ int module_finalize(const Elf_Ehdr *hdr,
 	addr = (u32 *)entry->addr;
 	printk("INSNS: %x %x %x %x\n",
 	       addr[0], addr[1], addr[2], addr[3]);
-	printk("stubs used %ld, stubs max %ld\n"
-	       "init_stubs used %ld, init stubs max %ld\n"
-	       "got entries used %ld, gots max %ld\n"
+	printk("got entries used %ld, gots max %ld\n"
 	       "fdescs used %ld, fdescs max %ld\n",
-	       me->arch.stub_count, me->arch.stub_max,
-	       me->arch.init_stub_count, me->arch.init_stub_max,
 	       me->arch.got_count, me->arch.got_max,
 	       me->arch.fdesc_count, me->arch.fdesc_max);
 #endif
@@ -829,7 +881,10 @@ int module_finalize(const Elf_Ehdr *hdr,
 				me->name, me->arch.got_count, MAX_GOTS);
 		return -EINVAL;
 	}
-	
+
+	kfree(me->arch.section);
+	me->arch.section = NULL;
+
 	/* no symbol table */
 	if(symhdr == NULL)
 		return 0;

Re: [PATCH 1/2] module.c: fix module loading failure of large modules (take 2)

[Kyle McMartin]

Ugh, I fail at typing, resending for rusty's benefit.

On Tue, Dec 30, 2008 at 01:07:24PM -0500, Kyle McMartin wrote:
> [Adding rusty to CC]
> 
> On Mon, Dec 29, 2008 at 09:43:12PM +0100, Helge Deller wrote:
> > +unsigned long module_additional_section_size(struct module *mod,
> > +					     unsigned int section);
> > +
> 
> I think this would be more palatable as
> 
> #ifdef HAVE_MODULE_SECTION_STUBS
> unsigned long module_additional_section_size(struct module *mod,
> 	unsigned int section);
> #else
> static inline unsigned long module_additional_section_size(struct module *mod,
> 	unsigned int section)
> {
> 	return 0;
> }
> #endif
> 
> and removing the conditional in kernel/module.c, possibly the symbol
> should be "arch_module_a..." just to make it clear to anyone reading.
> 
> Anyway, it's up to rusty.
> 
> Rusty, we'd like to get this patch in, so I can merge the dependent
> parisc-specific patch.
> 
> regards, Kyle
> 
> >  /* Allocator used for allocating struct module, core sections and init
> >     sections.  Returns NULL on failure. */
> >  void *module_alloc(unsigned long size);
> > diff --git a/init/Kconfig b/init/Kconfig
> > index f763762..5383815 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -908,6 +908,9 @@ config MODULE_SRCVERSION_ALL
> >  	  the version).  With this option, such a "srcversion" field
> >  	  will be created for all modules.  If unsure, say N.
> >  
> > +config HAVE_MODULE_SECTION_STUBS
> > +	bool
> > +
> >  config KMOD
> >  	def_bool y
> >  	help
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 1f4cc00..4b86308 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -1579,10 +1579,15 @@ static int simplify_symbols(Elf_Shdr *sechdrs,
> >  }
> >  
> >  /* Update size with this section: return offset. */
> > -static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
> > +static long get_offset(struct module *mod, unsigned int *size,
> > +		Elf_Shdr *sechdr, unsigned int section)
> >  {
> >  	long ret;
> >  
> > +#ifdef CONFIG_HAVE_MODULE_SECTION_STUBS
> > +	/* allocate some memory for stubs in front of each section */
> > +	*size += module_additional_section_size(mod, section);
> > +#endif
> >  	ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
> >  	*size = ret + sechdr->sh_size;
> >  	return ret;
> > @@ -1622,7 +1627,7 @@ static void layout_sections(struct module *mod,
> >  			    || strncmp(secstrings + s->sh_name,
> >  				       ".init", 5) == 0)
> >  				continue;
> > -			s->sh_entsize = get_offset(&mod->core_size, s);
> > +			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
> >  			DEBUGP("\t%s\n", secstrings + s->sh_name);
> >  		}
> >  		if (m == 0)
> > @@ -1640,7 +1645,7 @@ static void layout_sections(struct module *mod,
> >  			    || strncmp(secstrings + s->sh_name,
> >  				       ".init", 5) != 0)
> >  				continue;
> > -			s->sh_entsize = (get_offset(&mod->init_size, s)
> > +			s->sh_entsize = (get_offset(mod, &mod->init_size, s, i)
> >  					 | INIT_OFFSET_MASK);
> >  			DEBUGP("\t%s\n", secstrings + s->sh_name);
> >  		}
> > 
> 

Re: [PATCH 1/2] module.c: fix module loading failure of large modules (take 2)

[Helge Deller]

Kyle McMartin wrote:
> Ugh, I fail at typing, resending for rusty's benefit.
> 
> On Tue, Dec 30, 2008 at 01:07:24PM -0500, Kyle McMartin wrote:
>> [Adding rusty to CC]
>>
>> On Mon, Dec 29, 2008 at 09:43:12PM +0100, Helge Deller wrote:
>>> +unsigned long module_additional_section_size(struct module *mod,
>>> +					     unsigned int section);
>>> +
>> I think this would be more palatable as
>>
>> #ifdef HAVE_MODULE_SECTION_STUBS
>> unsigned long module_additional_section_size(struct module *mod,
>> 	unsigned int section);
>> #else
>> static inline unsigned long module_additional_section_size(struct module *mod,
>> 	unsigned int section)
>> {
>> 	return 0;
>> }
>> #endif
>>
>> and removing the conditional in kernel/module.c, possibly the symbol
>> should be "arch_module_a..." just to make it clear to anyone reading.
>>
>> Anyway, it's up to rusty.
>>
>> Rusty, we'd like to get this patch in, so I can merge the dependent
>> parisc-specific patch.

Rusty, I think I've a cleaner patch for that and will send it soon...

Helge

Re: [PATCH 1/2] module.c: fix module loading failure of large modules (take 3)

[Helge Deller]

Helge Deller wrote:
> Kyle McMartin wrote:
>>> Anyway, it's up to rusty.
>>>
>>> Rusty, we'd like to get this patch in, so I can merge the dependent
>>> parisc-specific patch.

Rusty,

While my initial patch (http://lkml.org/lkml/2008/12/29/274) added a
new CONFIG option and an optional callback function, this version instead
reuses the ElfHdr.sh_entsize field to tell the layout function how many
bytes to reserve.
It seems that only the IA64 module loader is affected by that change,
so the trivial patch for IA64 is below as well.
Other architectures use the sh_entsize field as well, but only for
bootloaders (x86, mips) or for special purpose loaders (powerpc cell),
but not for kernel modules. So the patch below should be pretty safe.

Personally I prefer this patch, but if you think my initial patch is
better, I'd be happy as well.


 arch/ia64/kernel/module.c |    4 +++-
 kernel/module.c           |   37 ++++++++++++++++++++-----------------
 2 files changed, 23 insertions(+), 18 deletions(-)


[PATCH 1/2] module.c: fix module loading failure of large modules (take 3)

When creating the final layout of a kernel module in memory, allow the
module loader to reserve some additional memory in front of a given section.
The amount of to-be allocated memory is returned by the architecture's
module_frob_arch_sections() function in the sechdrs.sh_entsize field.
By default this field is initialized by the module loader to zero.
Additional memory in front of code sections is currently only needed 
for the parisc port which needs to put the stub entries there to fulfill
the 17/22bit PCREL relocations with large kernel modules like xfs.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index aaa7d90..5bd35f1 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c
@@ -53,6 +53,8 @@
 
 #define MAX_LTOFF	((uint64_t) (1 << 22))	/* max. allowable linkage-table offset */
 
+#define INIT_LAYOUT_MASK (1UL << (BITS_PER_LONG-1))
+
 /* Define some relocation helper macros/types: */
 
 #define FORMAT_SHIFT	0
@@ -804,7 +806,7 @@ apply_relocate_add (Elf64_Shdr *sechdrs, const char *strtab, unsigned int symind
 
 	target_sec = sechdrs + sechdrs[relsec].sh_info;
 
-	if (target_sec->sh_entsize == ~0UL)
+	if (!(target_sec->sh_entsize & INIT_LAYOUT_MASK))
 		/*
 		 * If target section wasn't allocated, we don't need to relocate it.
 		 * Happens, e.g., for debug sections.
diff --git a/kernel/module.c b/kernel/module.c
index 1f4cc00..a09174d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -62,8 +62,9 @@
 #define ARCH_SHF_SMALL 0
 #endif
 
-/* If this is set, the section belongs in the init part of the module */
-#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
+#define INIT_LAYOUT_MASK (1UL << (BITS_PER_LONG-1)) /* section-layout done  */
+#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-2)) /* part of init section */
+#define INIT_STRIP_MASK  (INIT_OFFSET_MASK | INIT_LAYOUT_MASK)
 
 /* List of modules, protected by module_mutex or preempt_disable
  * (delete uses stop_machine/add uses RCU list operations). */
@@ -1583,6 +1584,8 @@ static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
 {
 	long ret;
 
+	/* architectures can request sh_entsize bytes in front of a section */
+	*size += (sechdr->sh_entsize & ~INIT_STRIP_MASK);
 	ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
 	*size = ret + sechdr->sh_size;
 	return ret;
@@ -1590,8 +1593,8 @@ static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
 
 /* Lay out the SHF_ALLOC sections in a way not dissimilar to how ld
    might -- code, read-only data, read-write data, small data.  Tally
-   sizes, and place the offsets into sh_entsize fields: high bit means it
-   belongs in init. */
+   sizes, and place the offsets into sh_entsize fields: INIT_OFFSET_MASK
+   indicates that it belongs in init. */
 static void layout_sections(struct module *mod,
 			    const Elf_Ehdr *hdr,
 			    Elf_Shdr *sechdrs,
@@ -1608,9 +1611,6 @@ static void layout_sections(struct module *mod,
 	};
 	unsigned int m, i;
 
-	for (i = 0; i < hdr->e_shnum; i++)
-		sechdrs[i].sh_entsize = ~0UL;
-
 	DEBUGP("Core section allocation order:\n");
 	for (m = 0; m < ARRAY_SIZE(masks); ++m) {
 		for (i = 0; i < hdr->e_shnum; ++i) {
@@ -1618,11 +1618,12 @@ static void layout_sections(struct module *mod,
 
 			if ((s->sh_flags & masks[m][0]) != masks[m][0]
 			    || (s->sh_flags & masks[m][1])
-			    || s->sh_entsize != ~0UL
+			    || (s->sh_entsize & INIT_LAYOUT_MASK)
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) == 0)
 				continue;
-			s->sh_entsize = get_offset(&mod->core_size, s);
+			s->sh_entsize = get_offset(&mod->core_size, s)
+					| INIT_LAYOUT_MASK;
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
 		if (m == 0)
@@ -1636,12 +1637,12 @@ static void layout_sections(struct module *mod,
 
 			if ((s->sh_flags & masks[m][0]) != masks[m][0]
 			    || (s->sh_flags & masks[m][1])
-			    || s->sh_entsize != ~0UL
+			    || (s->sh_entsize & INIT_LAYOUT_MASK)
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) != 0)
 				continue;
 			s->sh_entsize = (get_offset(&mod->init_size, s)
-					 | INIT_OFFSET_MASK);
+					 | INIT_OFFSET_MASK | INIT_LAYOUT_MASK);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
 		if (m == 0)
@@ -1986,7 +1987,11 @@ static noinline struct module *load_module(void __user *umod,
 
 	mod->state = MODULE_STATE_COMING;
 
-	/* Allow arches to frob section contents and sizes.  */
+	/* Allow arches to frob section contents, sh_entsize will tell the
+	 * the section layouter how much space to allocate in front of each
+	 * section */
+	for (i = 0; i < hdr->e_shnum; i++)
+		sechdrs[i].sh_entsize = 0UL;
 	err = module_frob_arch_sections(hdr, sechdrs, secstrings, mod);
 	if (err < 0)
 		goto free_mod;
@@ -2034,11 +2039,9 @@ static noinline struct module *load_module(void __user *umod,
 		if (!(sechdrs[i].sh_flags & SHF_ALLOC))
 			continue;
 
-		if (sechdrs[i].sh_entsize & INIT_OFFSET_MASK)
-			dest = mod->module_init
-				+ (sechdrs[i].sh_entsize & ~INIT_OFFSET_MASK);
-		else
-			dest = mod->module_core + sechdrs[i].sh_entsize;
+		dest = ((sechdrs[i].sh_entsize & INIT_OFFSET_MASK) ?
+			 mod->module_init : mod->module_core)
+			+ (sechdrs[i].sh_entsize & ~INIT_STRIP_MASK);
 
 		if (sechdrs[i].sh_type != SHT_NOBITS)
 			memcpy(dest, (void *)sechdrs[i].sh_addr,

Re: [PATCH 2/2] parisc: fix module loading failure of large modules (take 3)

[Helge Deller]

[PATCH 2/2] parisc: fix module loading failure of large modules (take 3)

On 32bit (and sometimes 64bit) and with big kernel modules like xfs or
ipv6 the relocation types R_PARISC_PCREL17F and R_PARISC_PCREL22F may
fail to reach their PLT stub if we only create one big stub array for
all sections at the beginning of the core or init section.

With this patch we now instead add individual PLT stub entries
directly in front of the code sections where the stubs are actually
called. This reduces the distance between the PCREL location and the
stub entry so that the relocations can be fulfilled.

The kernel module loader will allocate as much memory in front
of the code code segment as we return in the sechdrs.sh_entsize field.

Tested with 32- and 64bit kernels.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/arch/parisc/include/asm/module.h b/arch/parisc/include/asm/module.h
index c2cb49e..1f41234 100644
--- a/arch/parisc/include/asm/module.h
+++ b/arch/parisc/include/asm/module.h
@@ -23,8 +23,10 @@ struct mod_arch_specific
 {
 	unsigned long got_offset, got_count, got_max;
 	unsigned long fdesc_offset, fdesc_count, fdesc_max;
-	unsigned long stub_offset, stub_count, stub_max;
-	unsigned long init_stub_offset, init_stub_count, init_stub_max;
+	struct {
+		unsigned long stub_offset;
+		unsigned int stub_entries;
+		} *section;
 	int unwind_section;
 	struct unwind_table *unwind;
 };
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index 44138c3..aaa0879 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -6,6 +6,7 @@
  *
  *    Linux/PA-RISC Project (http://www.parisc-linux.org/)
  *    Copyright (C) 2003 Randolph Chung <tausq at debian . org>
+ *    Copyright (C) 2008 Helge Deller <deller@gmx.de>
  *
  *
  *    This program is free software; you can redistribute it and/or modify
@@ -24,6 +25,20 @@
  *
  *
  *    Notes:
+ *    - PLT stub handling
+ *      On 32bit (and sometimes 64bit) and with big kernel modules like xfs or
+ *      ipv6 the relocation types R_PARISC_PCREL17F and R_PARISC_PCREL22F may
+ *      fail to reach its PLT stub if we only create one big stub array for
+ *      all sections at the beginning of the core or init section.
+ *      Instead we now insert individual PLT stub entries directly in front of
+ *      of the code sections where the stubs are actually called.
+ *      This reduces the distance between the PCREL location and the stub entry
+ *      so that the relocations can be fulfilled.
+ *      In module_frob_arch_sections() we calculate how many bytes we need for
+ *      the stubs of each section and return this in the sh_entsize field. The
+ *      kernel module layouter will then later reserve this amount of bytes
+ *      in front of the code sections for us.
+ *
  *    - SEGREL32 handling
  *      We are not doing SEGREL32 handling correctly. According to the ABI, we
  *      should do a value offset, like this:
@@ -58,9 +73,13 @@
 #define DEBUGP(fmt...)
 #endif
 
+#define RELOC_REACHABLE(val, bits) \
+	(( ( !((val) & (1<<((bits)-1))) && ((val)>>(bits)) != 0 )  ||	\
+	     ( ((val) & (1<<((bits)-1))) && ((val)>>(bits)) != (((__typeof__(val))(~0))>>((bits)+2)))) ? \
+	0 : 1)
+
 #define CHECK_RELOC(val, bits) \
-	if ( ( !((val) & (1<<((bits)-1))) && ((val)>>(bits)) != 0 )  ||	\
-	     ( ((val) & (1<<((bits)-1))) && ((val)>>(bits)) != (((__typeof__(val))(~0))>>((bits)+2)))) { \
+	if (!RELOC_REACHABLE(val, bits)) { \
 		printk(KERN_ERR "module %s relocation of symbol %s is out of range (0x%lx in %d bits)\n", \
 		me->name, strtab + sym->st_name, (unsigned long)val, bits); \
 		return -ENOEXEC;			\
@@ -92,13 +111,6 @@ static inline int in_local(struct module *me, void *loc)
 	return in_init(me, loc) || in_core(me, loc);
 }
 
-static inline int in_local_section(struct module *me, void *loc, void *dot)
-{
-	return (in_init(me, loc) && in_init(me, dot)) ||
-		(in_core(me, loc) && in_core(me, dot));
-}
-
-
 #ifndef CONFIG_64BIT
 struct got_entry {
 	Elf32_Addr addr;
@@ -258,6 +270,9 @@ static inline unsigned long count_stubs(const Elf_Rela *rela, unsigned long n)
 /* Free memory returned from module_alloc */
 void module_free(struct module *mod, void *module_region)
 {
+	kfree(mod->arch.section);
+	mod->arch.section = NULL;
+
 	vfree(module_region);
 	/* FIXME: If module_region == mod->init_region, trim exception
            table entries. */
@@ -269,12 +284,18 @@ int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 			      CONST char *secstrings,
 			      struct module *me)
 {
-	unsigned long gots = 0, fdescs = 0, stubs = 0, init_stubs = 0;
+	unsigned long gots = 0, fdescs = 0, len;
 	unsigned int i;
 
+	len = hdr->e_shnum * sizeof(me->arch.section[0]);
+	me->arch.section = kzalloc(len, GFP_KERNEL);
+	if (!me->arch.section)
+		return -ENOMEM;
+
 	for (i = 1; i < hdr->e_shnum; i++) {
-		const Elf_Rela *rels = (void *)hdr + sechdrs[i].sh_offset;
+		const Elf_Rela *rels = (void *)sechdrs[i].sh_addr;
 		unsigned long nrels = sechdrs[i].sh_size / sizeof(*rels);
+		unsigned int count, s;
 
 		if (strncmp(secstrings + sechdrs[i].sh_name,
 			    ".PARISC.unwind", 14) == 0)
@@ -290,11 +311,29 @@ int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 		 */
 		gots += count_gots(rels, nrels);
 		fdescs += count_fdescs(rels, nrels);
-		if(strncmp(secstrings + sechdrs[i].sh_name,
-			   ".rela.init", 10) == 0)
-			init_stubs += count_stubs(rels, nrels);
-		else
-			stubs += count_stubs(rels, nrels);
+
+		/* XXX: By sorting the relocs and finding duplicate entries
+		 *  we could reduce the number of necessary stubs and save
+		 *  some memory. */
+		count = count_stubs(rels, nrels);
+		if (!count)
+			continue;
+
+		/* so we need relocation stubs. reserve necessary memory. */
+		/* sh_info gives the section for which we need to add stubs. */
+		s = sechdrs[i].sh_info;
+
+		/* each code section should only have one relocation section */
+		WARN_ON(me->arch.section[s].stub_entries);
+
+		/* store number of stubs we need for this section */
+		me->arch.section[s].stub_entries += count;
+
+		/* tell section layouter how much space we need for all stubs
+		 * of this section (including one additional for correct
+		 * alignment of the stubs) */
+		sechdrs[s].sh_entsize = sizeof(struct stub_entry) *
+			(me->arch.section[s].stub_entries + 1);
 	}
 
 	/* align things a bit */
@@ -306,18 +345,8 @@ int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 	me->arch.fdesc_offset = me->core_size;
 	me->core_size += fdescs * sizeof(Elf_Fdesc);
 
-	me->core_size = ALIGN(me->core_size, 16);
-	me->arch.stub_offset = me->core_size;
-	me->core_size += stubs * sizeof(struct stub_entry);
-
-	me->init_size = ALIGN(me->init_size, 16);
-	me->arch.init_stub_offset = me->init_size;
-	me->init_size += init_stubs * sizeof(struct stub_entry);
-
 	me->arch.got_max = gots;
 	me->arch.fdesc_max = fdescs;
-	me->arch.stub_max = stubs;
-	me->arch.init_stub_max = init_stubs;
 
 	return 0;
 }
@@ -380,23 +409,27 @@ enum elf_stub_type {
 };
 
 static Elf_Addr get_stub(struct module *me, unsigned long value, long addend,
-	enum elf_stub_type stub_type, int init_section)
+	enum elf_stub_type stub_type, Elf_Addr loc0, unsigned int targetsec)
 {
-	unsigned long i;
 	struct stub_entry *stub;
 
-	if(init_section) {
-		i = me->arch.init_stub_count++;
-		BUG_ON(me->arch.init_stub_count > me->arch.init_stub_max);
-		stub = me->module_init + me->arch.init_stub_offset + 
-			i * sizeof(struct stub_entry);
-	} else {
-		i = me->arch.stub_count++;
-		BUG_ON(me->arch.stub_count > me->arch.stub_max);
-		stub = me->module_core + me->arch.stub_offset + 
-			i * sizeof(struct stub_entry);
+	/* initialize stub_offset to point in front of the section */
+	if (!me->arch.section[targetsec].stub_offset) {
+		loc0 -= (me->arch.section[targetsec].stub_entries + 1) *
+				sizeof(struct stub_entry);
+		/* get correct alignment for the stubs */
+		loc0 = ALIGN(loc0, sizeof(struct stub_entry));
+		me->arch.section[targetsec].stub_offset = loc0;
 	}
 
+	/* get address of stub entry */
+	stub = (void *) me->arch.section[targetsec].stub_offset;
+	me->arch.section[targetsec].stub_offset += sizeof(struct stub_entry);
+
+	/* do not write outside available stub area */
+	BUG_ON(0 == me->arch.section[targetsec].stub_entries--);
+
+
 #ifndef CONFIG_64BIT
 /* for 32-bit the stub looks like this:
  * 	ldil L'XXX,%r1
@@ -489,15 +522,19 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 	Elf32_Addr val;
 	Elf32_Sword addend;
 	Elf32_Addr dot;
+	Elf_Addr loc0;
+	unsigned int targetsec = sechdrs[relsec].sh_info;
 	//unsigned long dp = (unsigned long)$global$;
 	register unsigned long dp asm ("r27");
 
 	DEBUGP("Applying relocate section %u to %u\n", relsec,
-	       sechdrs[relsec].sh_info);
+	       targetsec);
 	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
 		/* This is where to make the change */
-		loc = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+		loc = (void *)sechdrs[targetsec].sh_addr
 		      + rel[i].r_offset;
+		/* This is the start of the target section */
+		loc0 = sechdrs[targetsec].sh_addr;
 		/* This is the symbol it is referring to */
 		sym = (Elf32_Sym *)sechdrs[symindex].sh_addr
 			+ ELF32_R_SYM(rel[i].r_info);
@@ -569,19 +606,32 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			break;
 		case R_PARISC_PCREL17F:
 			/* 17-bit PC relative address */
-			val = get_stub(me, val, addend, ELF_STUB_GOT, in_init(me, loc));
+			/* calculate direct call offset */
+			val += addend;
 			val = (val - dot - 8)/4;
-			CHECK_RELOC(val, 17)
+			if (!RELOC_REACHABLE(val, 17)) {
+				/* direct distance too far, create
+				 * stub entry instead */
+				val = get_stub(me, sym->st_value, addend,
+					ELF_STUB_DIRECT, loc0, targetsec);
+				val = (val - dot - 8)/4;
+				CHECK_RELOC(val, 17);
+			}
 			*loc = (*loc & ~0x1f1ffd) | reassemble_17(val);
 			break;
 		case R_PARISC_PCREL22F:
 			/* 22-bit PC relative address; only defined for pa20 */
-			val = get_stub(me, val, addend, ELF_STUB_GOT, in_init(me, loc));
-			DEBUGP("STUB FOR %s loc %lx+%lx at %lx\n", 
-			       strtab + sym->st_name, (unsigned long)loc, addend, 
-			       val)
+			/* calculate direct call offset */
+			val += addend;
 			val = (val - dot - 8)/4;
-			CHECK_RELOC(val, 22);
+			if (!RELOC_REACHABLE(val, 22)) {
+				/* direct distance too far, create
+				 * stub entry instead */
+				val = get_stub(me, sym->st_value, addend,
+					ELF_STUB_DIRECT, loc0, targetsec);
+				val = (val - dot - 8)/4;
+				CHECK_RELOC(val, 22);
+			}
 			*loc = (*loc & ~0x3ff1ffd) | reassemble_22(val);
 			break;
 
@@ -610,13 +660,17 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 	Elf64_Addr val;
 	Elf64_Sxword addend;
 	Elf64_Addr dot;
+	Elf_Addr loc0;
+	unsigned int targetsec = sechdrs[relsec].sh_info;
 
 	DEBUGP("Applying relocate section %u to %u\n", relsec,
-	       sechdrs[relsec].sh_info);
+	       targetsec);
 	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
 		/* This is where to make the change */
-		loc = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+		loc = (void *)sechdrs[targetsec].sh_addr
 		      + rel[i].r_offset;
+		/* This is the start of the target section */
+		loc0 = sechdrs[targetsec].sh_addr;
 		/* This is the symbol it is referring to */
 		sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
 			+ ELF64_R_SYM(rel[i].r_info);
@@ -672,42 +726,40 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			DEBUGP("PCREL22F Symbol %s loc %p val %lx\n",
 			       strtab + sym->st_name,
 			       loc, val);
+			val += addend;
 			/* can we reach it locally? */
-			if(!in_local_section(me, (void *)val, (void *)dot)) {
-
-				if (in_local(me, (void *)val))
-					/* this is the case where the
-					 * symbol is local to the
-					 * module, but in a different
-					 * section, so stub the jump
-					 * in case it's more than 22
-					 * bits away */
-					val = get_stub(me, val, addend, ELF_STUB_DIRECT,
-						       in_init(me, loc));
-				else if (strncmp(strtab + sym->st_name, "$$", 2)
+			if (in_local(me, (void *)val)) {
+				/* this is the case where the symbol is local
+				 * to the module, but in a different section,
+				 * so stub the jump in case it's more than 22
+				 * bits away */
+				val = (val - dot - 8)/4;
+				if (!RELOC_REACHABLE(val, 22)) {
+					/* direct distance too far, create
+					 * stub entry instead */
+					val = get_stub(me, sym->st_value,
+						addend, ELF_STUB_DIRECT,
+						loc0, targetsec);
+				} else {
+					/* Ok, we can reach it directly. */
+					val = sym->st_value;
+					val += addend;
+				}
+			} else {
+				val = sym->st_value;
+				if (strncmp(strtab + sym->st_name, "$$", 2)
 				    == 0)
 					val = get_stub(me, val, addend, ELF_STUB_MILLI,
-						       in_init(me, loc));
+						       loc0, targetsec);
 				else
 					val = get_stub(me, val, addend, ELF_STUB_GOT,
-						       in_init(me, loc));
+						       loc0, targetsec);
 			}
 			DEBUGP("STUB FOR %s loc %lx, val %lx+%lx at %lx\n", 
 			       strtab + sym->st_name, loc, sym->st_value,
 			       addend, val);
-			/* FIXME: local symbols work as long as the
-			 * core and init pieces aren't separated too
-			 * far.  If this is ever broken, you will trip
-			 * the check below.  The way to fix it would
-			 * be to generate local stubs to go between init
-			 * and core */
-			if((Elf64_Sxword)(val - dot - 8) > 0x800000 -1 ||
-			   (Elf64_Sxword)(val - dot - 8) < -0x800000) {
-				printk(KERN_ERR "Module %s, symbol %s is out of range for PCREL22F relocation\n",
-				       me->name, strtab + sym->st_name);
-				return -ENOEXEC;
-			}
 			val = (val - dot - 8)/4;
+			CHECK_RELOC(val, 22);
 			*loc = (*loc & ~0x3ff1ffd) | reassemble_22(val);
 			break;
 		case R_PARISC_DIR64:
@@ -794,12 +846,8 @@ int module_finalize(const Elf_Ehdr *hdr,
 	addr = (u32 *)entry->addr;
 	printk("INSNS: %x %x %x %x\n",
 	       addr[0], addr[1], addr[2], addr[3]);
-	printk("stubs used %ld, stubs max %ld\n"
-	       "init_stubs used %ld, init stubs max %ld\n"
-	       "got entries used %ld, gots max %ld\n"
+	printk("got entries used %ld, gots max %ld\n"
 	       "fdescs used %ld, fdescs max %ld\n",
-	       me->arch.stub_count, me->arch.stub_max,
-	       me->arch.init_stub_count, me->arch.init_stub_max,
 	       me->arch.got_count, me->arch.got_max,
 	       me->arch.fdesc_count, me->arch.fdesc_max);
 #endif
@@ -829,7 +877,10 @@ int module_finalize(const Elf_Ehdr *hdr,
 				me->name, me->arch.got_count, MAX_GOTS);
 		return -EINVAL;
 	}
-	
+
+	kfree(me->arch.section);
+	me->arch.section = NULL;
+
 	/* no symbol table */
 	if(symhdr == NULL)
 		return 0;

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 2)

[Rusty Russell]

On Tuesday 30 December 2008 07:04:54 Helge Deller wrote:
> This is the second take of the patch series.
> Changes to previous version:
> - new CONFIG_HAVE_MODULE_SECTION_STUBS config option
> - put stub entries of a code section in front of the section
> 
> ____________
> The parisc port (esp. the 32bit kernel) currently lacks the ability to
> load large kernel modules like xfs or ipv6. This is a long outstanding
> bug and has already been reported a few times, e.g.:
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=350482,
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=401439,
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508489
> 
> The symptom is like this:
> # modprobe xfs
> FATAL: Error inserting xfs
> (/lib/modules/2.6.26-1-parisc/kernel/fs/xfs/xfs.ko): Invalid module
> format
> 
> In dmesg:
> module xfs relocation of symbol xfs_btree_read_bufs is out of range
> (0x3ffefffe in 17 bits)
> 
> The reason for the failure is, that the architecture only provides the
> R_PARISC_PCREL17F (for 32bit kernels) and R_PARISC_PCREL22F (for PA2.0
> and 64bit kernels) relocations, which sometimes can't reach the target
> address of the stub entry if the kernel module is too large. Currently
> parisc (like other architectures) creates one big PLT section for all
> stubs at the beginning of the init and core sections.
> 
> The following two patches changes the parisc module loader to put stubs
> for the code sections in front of each section, so that the distance to
> the stubs more easily fits into the available 17/22 bits.

So now any one section has to pass 17 bits to break?  How close are you with
the xfs module?

But it's kind of nasty, overloading sh_entsize further.  Could we instead
do something like add a arch_module_section_size() weak fn which you can
overload?  We'd use that in get_offset() so our layout and size calculations
were correct, and use sh_size everywhere else.

Cheers,
Rusty.

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 2)

[Helge Deller]

Rusty Russell wrote:
> On Tuesday 30 December 2008 07:04:54 Helge Deller wrote:
>> This is the second take of the patch series.
>> Changes to previous version:
>> - new CONFIG_HAVE_MODULE_SECTION_STUBS config option
>> - put stub entries of a code section in front of the section
>>
>> ____________
>> The parisc port (esp. the 32bit kernel) currently lacks the ability to
>> load large kernel modules like xfs or ipv6. This is a long outstanding
>> bug and has already been reported a few times, e.g.:
>> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=350482,
>> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=401439,
>> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508489
>>
>> The symptom is like this:
>> # modprobe xfs
>> FATAL: Error inserting xfs
>> (/lib/modules/2.6.26-1-parisc/kernel/fs/xfs/xfs.ko): Invalid module
>> format
>>
>> In dmesg:
>> module xfs relocation of symbol xfs_btree_read_bufs is out of range
>> (0x3ffefffe in 17 bits)
>>
>> The reason for the failure is, that the architecture only provides the
>> R_PARISC_PCREL17F (for 32bit kernels) and R_PARISC_PCREL22F (for PA2.0
>> and 64bit kernels) relocations, which sometimes can't reach the target
>> address of the stub entry if the kernel module is too large. Currently
>> parisc (like other architectures) creates one big PLT section for all
>> stubs at the beginning of the init and core sections.
>>
>> The following two patches changes the parisc module loader to put stubs
>> for the code sections in front of each section, so that the distance to
>> the stubs more easily fits into the available 17/22 bits.
> 
> So now any one section has to pass 17 bits to break?  How close are you with
> the xfs module?

I did not tested it very much, but xfs is around 1.1M on disk, and ~750K when 
loaded. I think it breaked being 1/4 through the relocations.

> But it's kind of nasty, overloading sh_entsize further.  Could we instead
> do something like add a arch_module_section_size() weak fn which you can
> overload?

Sure. I'll respin the patch.

> We'd use that in get_offset() so our layout and size calculations
> were correct, 

Yes, good.

> and use sh_size everywhere else.

BTW, although the comment states that arches can change section sizes 
in the module_frob_arch_sections() function:
/* Allow arches to frob section contents and sizes.  */
it will break horrible if you do so.
What I found was, that if you change sh_size, at least the module
references / dependency chain will break when running lsmod.

Helge

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 2)

[Rusty Russell]

On Wednesday 31 December 2008 09:32:41 Helge Deller wrote:
> BTW, although the comment states that arches can change section sizes 
> in the module_frob_arch_sections() function:
> /* Allow arches to frob section contents and sizes.  */
> it will break horrible if you do so.
> What I found was, that if you change sh_size, at least the module
> references / dependency chain will break when running lsmod.

Well, you can change the size of NOBITS sections: other things are likely
to be less successful.  You could probably make them smaller, though.

Cheers,
Rusty.

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Helge Deller]

[PATCH 1/2] module.c: fix module loading failure of large kernel modules

When creating the final layout of a kernel module in memory, allow the
module loader to reserve some additional memory in front of a given section.
This is currently only needed for the parisc port which needs to put the
stub entries there to fulfill the 17/22bit PCREL relocations with large
kernel modules like xfs.

Differences of this patch to previous versions:
- added weak funtion arch_module_section_size()
- no kernel config options needed
- no overloading of sh_entsize

Signed-off-by: Helge Deller <deller@gmx.de>

diffstat:
 include/linux/moduleloader.h |    4 ++++
 kernel/module.c              |   16 +++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)


diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index eb10339..f2b1b62 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,10 @@ int module_frob_arch_sections(Elf_Ehdr *hdr,
 			      char *secstrings,
 			      struct module *mod);
 
+/* Additional bytes needed by arch in front of individual sections */
+unsigned int arch_module_section_size(struct module *mod,
+				      unsigned int section);
+
 /* Allocator used for allocating struct module, core sections and init
    sections.  Returns NULL on failure. */
 void *module_alloc(unsigned long size);
diff --git a/kernel/module.c b/kernel/module.c
index 1f4cc00..5b91b17 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1578,11 +1578,21 @@ static int simplify_symbols(Elf_Shdr *sechdrs,
 	return ret;
 }
 
+/* Additional bytes needed by arch in front of individual sections */
+unsigned int __attribute__ ((weak)) arch_module_section_size(
+		struct module *mod, unsigned int section)
+{
+	/* default implementation just returns zero */
+	return 0;
+}
+
 /* Update size with this section: return offset. */
-static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
+static long get_offset(struct module *mod, unsigned int *size,
+		Elf_Shdr *sechdr, unsigned int section)
 {
 	long ret;
 
+	*size += arch_module_section_size(mod, section);
 	ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
 	*size = ret + sechdr->sh_size;
 	return ret;
@@ -1622,7 +1632,7 @@ static void layout_sections(struct module *mod,
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) == 0)
 				continue;
-			s->sh_entsize = get_offset(&mod->core_size, s);
+			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
 		if (m == 0)
@@ -1640,7 +1650,7 @@ static void layout_sections(struct module *mod,
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) != 0)
 				continue;
-			s->sh_entsize = (get_offset(&mod->init_size, s)
+			s->sh_entsize = (get_offset(mod, &mod->init_size, s, i)
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}

[PATCH 2/2] parisc: fix module loading failure of large modules

[Helge Deller]

[PATCH 2/2] parisc: fix module loading failure of large modules (take 3)

On 32bit (and sometimes 64bit) and with big kernel modules like xfs or
ipv6 the relocation types R_PARISC_PCREL17F and R_PARISC_PCREL22F may
fail to reach their PLT stub if we only create one big stub array for
all sections at the beginning of the core or init section.

With this patch we now instead add individual PLT stub entries
directly in front of the code sections where the stubs are actually
called. This reduces the distance between the PCREL location and the
stub entry so that the relocations can be fulfilled.

While calculating the final layout of the kernel module in memory, the
kernel module loader calls arch_module_section_size() to request the
to be reserved amount of memory in front of each individual section.

Tested with 32- and 64bit kernels.

Signed-off-by: Helge Deller <deller@gmx.de>

diffstat:
 include/asm/module.h |    6 -
 kernel/module.c      |  217 +++++++++++++++++++++++++++++++--------------------
 2 files changed, 140 insertions(+), 83 deletions(-)


diff --git a/arch/parisc/include/asm/module.h b/arch/parisc/include/asm/module.h
index c2cb49e..1f41234 100644
--- a/arch/parisc/include/asm/module.h
+++ b/arch/parisc/include/asm/module.h
@@ -23,8 +23,10 @@ struct mod_arch_specific
 {
 	unsigned long got_offset, got_count, got_max;
 	unsigned long fdesc_offset, fdesc_count, fdesc_max;
-	unsigned long stub_offset, stub_count, stub_max;
-	unsigned long init_stub_offset, init_stub_count, init_stub_max;
+	struct {
+		unsigned long stub_offset;
+		unsigned int stub_entries;
+		} *section;
 	int unwind_section;
 	struct unwind_table *unwind;
 };
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index 44138c3..3228007 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -6,6 +6,7 @@
  *
  *    Linux/PA-RISC Project (http://www.parisc-linux.org/)
  *    Copyright (C) 2003 Randolph Chung <tausq at debian . org>
+ *    Copyright (C) 2008 Helge Deller <deller@gmx.de>
  *
  *
  *    This program is free software; you can redistribute it and/or modify
@@ -24,6 +25,19 @@
  *
  *
  *    Notes:
+ *    - PLT stub handling
+ *      On 32bit (and sometimes 64bit) and with big kernel modules like xfs or
+ *      ipv6 the relocation types R_PARISC_PCREL17F and R_PARISC_PCREL22F may
+ *      fail to reach their PLT stub if we only create one big stub array for
+ *      all sections at the beginning of the core or init section.
+ *      Instead we now insert individual PLT stub entries directly in front of
+ *      of the code sections where the stubs are actually called.
+ *      This reduces the distance between the PCREL location and the stub entry
+ *      so that the relocations can be fulfilled.
+ *      While calculating the final layout of the kernel module in memory, the
+ *      kernel module loader calls arch_module_section_size() to request the
+ *      to be reserved amount of memory in front of each individual section.
+ *
  *    - SEGREL32 handling
  *      We are not doing SEGREL32 handling correctly. According to the ABI, we
  *      should do a value offset, like this:
@@ -58,9 +72,13 @@
 #define DEBUGP(fmt...)
 #endif
 
+#define RELOC_REACHABLE(val, bits) \
+	(( ( !((val) & (1<<((bits)-1))) && ((val)>>(bits)) != 0 )  ||	\
+	     ( ((val) & (1<<((bits)-1))) && ((val)>>(bits)) != (((__typeof__(val))(~0))>>((bits)+2)))) ? \
+	0 : 1)
+
 #define CHECK_RELOC(val, bits) \
-	if ( ( !((val) & (1<<((bits)-1))) && ((val)>>(bits)) != 0 )  ||	\
-	     ( ((val) & (1<<((bits)-1))) && ((val)>>(bits)) != (((__typeof__(val))(~0))>>((bits)+2)))) { \
+	if (!RELOC_REACHABLE(val, bits)) { \
 		printk(KERN_ERR "module %s relocation of symbol %s is out of range (0x%lx in %d bits)\n", \
 		me->name, strtab + sym->st_name, (unsigned long)val, bits); \
 		return -ENOEXEC;			\
@@ -92,13 +110,6 @@ static inline int in_local(struct module *me, void *loc)
 	return in_init(me, loc) || in_core(me, loc);
 }
 
-static inline int in_local_section(struct module *me, void *loc, void *dot)
-{
-	return (in_init(me, loc) && in_init(me, dot)) ||
-		(in_core(me, loc) && in_core(me, dot));
-}
-
-
 #ifndef CONFIG_64BIT
 struct got_entry {
 	Elf32_Addr addr;
@@ -258,23 +269,43 @@ static inline unsigned long count_stubs(const Elf_Rela *rela, unsigned long n)
 /* Free memory returned from module_alloc */
 void module_free(struct module *mod, void *module_region)
 {
+	kfree(mod->arch.section);
+	mod->arch.section = NULL;
+
 	vfree(module_region);
 	/* FIXME: If module_region == mod->init_region, trim exception
            table entries. */
 }
 
+
+/* Additional bytes needed in front of individual sections. */
+unsigned int arch_module_section_size(struct module *mod,
+				      unsigned int section)
+{
+	/* size needed for all stubs of this section (including
+	 * one additional for correct alignment of the stubs) */
+	return (mod->arch.section[section].stub_entries + 1)
+		* sizeof(struct stub_entry);
+}
+
 #define CONST 
 int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 			      CONST Elf_Shdr *sechdrs,
 			      CONST char *secstrings,
 			      struct module *me)
 {
-	unsigned long gots = 0, fdescs = 0, stubs = 0, init_stubs = 0;
+	unsigned long gots = 0, fdescs = 0, len;
 	unsigned int i;
 
+	len = hdr->e_shnum * sizeof(me->arch.section[0]);
+	me->arch.section = kzalloc(len, GFP_KERNEL);
+	if (!me->arch.section)
+		return -ENOMEM;
+
 	for (i = 1; i < hdr->e_shnum; i++) {
-		const Elf_Rela *rels = (void *)hdr + sechdrs[i].sh_offset;
+		const Elf_Rela *rels = (void *)sechdrs[i].sh_addr;
 		unsigned long nrels = sechdrs[i].sh_size / sizeof(*rels);
+		unsigned int count, s;
 
 		if (strncmp(secstrings + sechdrs[i].sh_name,
 			    ".PARISC.unwind", 14) == 0)
@@ -290,11 +321,23 @@ int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 		 */
 		gots += count_gots(rels, nrels);
 		fdescs += count_fdescs(rels, nrels);
-		if(strncmp(secstrings + sechdrs[i].sh_name,
-			   ".rela.init", 10) == 0)
-			init_stubs += count_stubs(rels, nrels);
-		else
-			stubs += count_stubs(rels, nrels);
+
+		/* XXX: By sorting the relocs and finding duplicate entries
+		 *  we could reduce the number of necessary stubs and save
+		 *  some memory. */
+		count = count_stubs(rels, nrels);
+		if (!count)
+			continue;
+
+		/* so we need relocation stubs. reserve necessary memory. */
+		/* sh_info gives the section for which we need to add stubs. */
+		s = sechdrs[i].sh_info;
+
+		/* each code section should only have one relocation section */
+		WARN_ON(me->arch.section[s].stub_entries);
+
+		/* store number of stubs we need for this section */
+		me->arch.section[s].stub_entries += count;
 	}
 
 	/* align things a bit */
@@ -306,18 +349,8 @@ int module_frob_arch_sections(CONST Elf_Ehdr *hdr,
 	me->arch.fdesc_offset = me->core_size;
 	me->core_size += fdescs * sizeof(Elf_Fdesc);
 
-	me->core_size = ALIGN(me->core_size, 16);
-	me->arch.stub_offset = me->core_size;
-	me->core_size += stubs * sizeof(struct stub_entry);
-
-	me->init_size = ALIGN(me->init_size, 16);
-	me->arch.init_stub_offset = me->init_size;
-	me->init_size += init_stubs * sizeof(struct stub_entry);
-
 	me->arch.got_max = gots;
 	me->arch.fdesc_max = fdescs;
-	me->arch.stub_max = stubs;
-	me->arch.init_stub_max = init_stubs;
 
 	return 0;
 }
@@ -380,23 +413,27 @@ enum elf_stub_type {
 };
 
 static Elf_Addr get_stub(struct module *me, unsigned long value, long addend,
-	enum elf_stub_type stub_type, int init_section)
+	enum elf_stub_type stub_type, Elf_Addr loc0, unsigned int targetsec)
 {
-	unsigned long i;
 	struct stub_entry *stub;
 
-	if(init_section) {
-		i = me->arch.init_stub_count++;
-		BUG_ON(me->arch.init_stub_count > me->arch.init_stub_max);
-		stub = me->module_init + me->arch.init_stub_offset + 
-			i * sizeof(struct stub_entry);
-	} else {
-		i = me->arch.stub_count++;
-		BUG_ON(me->arch.stub_count > me->arch.stub_max);
-		stub = me->module_core + me->arch.stub_offset + 
-			i * sizeof(struct stub_entry);
+	/* initialize stub_offset to point in front of the section */
+	if (!me->arch.section[targetsec].stub_offset) {
+		loc0 -= (me->arch.section[targetsec].stub_entries + 1) *
+				sizeof(struct stub_entry);
+		/* get correct alignment for the stubs */
+		loc0 = ALIGN(loc0, sizeof(struct stub_entry));
+		me->arch.section[targetsec].stub_offset = loc0;
 	}
 
+	/* get address of stub entry */
+	stub = (void *) me->arch.section[targetsec].stub_offset;
+	me->arch.section[targetsec].stub_offset += sizeof(struct stub_entry);
+
+	/* do not write outside available stub area */
+	BUG_ON(0 == me->arch.section[targetsec].stub_entries--);
+
+
 #ifndef CONFIG_64BIT
 /* for 32-bit the stub looks like this:
  * 	ldil L'XXX,%r1
@@ -489,15 +526,19 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 	Elf32_Addr val;
 	Elf32_Sword addend;
 	Elf32_Addr dot;
+	Elf_Addr loc0;
+	unsigned int targetsec = sechdrs[relsec].sh_info;
 	//unsigned long dp = (unsigned long)$global$;
 	register unsigned long dp asm ("r27");
 
 	DEBUGP("Applying relocate section %u to %u\n", relsec,
-	       sechdrs[relsec].sh_info);
+	       targetsec);
 	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
 		/* This is where to make the change */
-		loc = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+		loc = (void *)sechdrs[targetsec].sh_addr
 		      + rel[i].r_offset;
+		/* This is the start of the target section */
+		loc0 = sechdrs[targetsec].sh_addr;
 		/* This is the symbol it is referring to */
 		sym = (Elf32_Sym *)sechdrs[symindex].sh_addr
 			+ ELF32_R_SYM(rel[i].r_info);
@@ -569,19 +610,32 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			break;
 		case R_PARISC_PCREL17F:
 			/* 17-bit PC relative address */
-			val = get_stub(me, val, addend, ELF_STUB_GOT, in_init(me, loc));
+			/* calculate direct call offset */
+			val += addend;
 			val = (val - dot - 8)/4;
-			CHECK_RELOC(val, 17)
+			if (!RELOC_REACHABLE(val, 17)) {
+				/* direct distance too far, create
+				 * stub entry instead */
+				val = get_stub(me, sym->st_value, addend,
+					ELF_STUB_DIRECT, loc0, targetsec);
+				val = (val - dot - 8)/4;
+				CHECK_RELOC(val, 17);
+			}
 			*loc = (*loc & ~0x1f1ffd) | reassemble_17(val);
 			break;
 		case R_PARISC_PCREL22F:
 			/* 22-bit PC relative address; only defined for pa20 */
-			val = get_stub(me, val, addend, ELF_STUB_GOT, in_init(me, loc));
-			DEBUGP("STUB FOR %s loc %lx+%lx at %lx\n", 
-			       strtab + sym->st_name, (unsigned long)loc, addend, 
-			       val)
+			/* calculate direct call offset */
+			val += addend;
 			val = (val - dot - 8)/4;
-			CHECK_RELOC(val, 22);
+			if (!RELOC_REACHABLE(val, 22)) {
+				/* direct distance too far, create
+				 * stub entry instead */
+				val = get_stub(me, sym->st_value, addend,
+					ELF_STUB_DIRECT, loc0, targetsec);
+				val = (val - dot - 8)/4;
+				CHECK_RELOC(val, 22);
+			}
 			*loc = (*loc & ~0x3ff1ffd) | reassemble_22(val);
 			break;
 
@@ -610,13 +664,17 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 	Elf64_Addr val;
 	Elf64_Sxword addend;
 	Elf64_Addr dot;
+	Elf_Addr loc0;
+	unsigned int targetsec = sechdrs[relsec].sh_info;
 
 	DEBUGP("Applying relocate section %u to %u\n", relsec,
-	       sechdrs[relsec].sh_info);
+	       targetsec);
 	for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) {
 		/* This is where to make the change */
-		loc = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr
+		loc = (void *)sechdrs[targetsec].sh_addr
 		      + rel[i].r_offset;
+		/* This is the start of the target section */
+		loc0 = sechdrs[targetsec].sh_addr;
 		/* This is the symbol it is referring to */
 		sym = (Elf64_Sym *)sechdrs[symindex].sh_addr
 			+ ELF64_R_SYM(rel[i].r_info);
@@ -672,42 +730,40 @@ int apply_relocate_add(Elf_Shdr *sechdrs,
 			DEBUGP("PCREL22F Symbol %s loc %p val %lx\n",
 			       strtab + sym->st_name,
 			       loc, val);
+			val += addend;
 			/* can we reach it locally? */
-			if(!in_local_section(me, (void *)val, (void *)dot)) {
-
-				if (in_local(me, (void *)val))
-					/* this is the case where the
-					 * symbol is local to the
-					 * module, but in a different
-					 * section, so stub the jump
-					 * in case it's more than 22
-					 * bits away */
-					val = get_stub(me, val, addend, ELF_STUB_DIRECT,
-						       in_init(me, loc));
-				else if (strncmp(strtab + sym->st_name, "$$", 2)
+			if (in_local(me, (void *)val)) {
+				/* this is the case where the symbol is local
+				 * to the module, but in a different section,
+				 * so stub the jump in case it's more than 22
+				 * bits away */
+				val = (val - dot - 8)/4;
+				if (!RELOC_REACHABLE(val, 22)) {
+					/* direct distance too far, create
+					 * stub entry instead */
+					val = get_stub(me, sym->st_value,
+						addend, ELF_STUB_DIRECT,
+						loc0, targetsec);
+				} else {
+					/* Ok, we can reach it directly. */
+					val = sym->st_value;
+					val += addend;
+				}
+			} else {
+				val = sym->st_value;
+				if (strncmp(strtab + sym->st_name, "$$", 2)
 				    == 0)
 					val = get_stub(me, val, addend, ELF_STUB_MILLI,
-						       in_init(me, loc));
+						       loc0, targetsec);
 				else
 					val = get_stub(me, val, addend, ELF_STUB_GOT,
-						       in_init(me, loc));
+						       loc0, targetsec);
 			}
 			DEBUGP("STUB FOR %s loc %lx, val %lx+%lx at %lx\n", 
 			       strtab + sym->st_name, loc, sym->st_value,
 			       addend, val);
-			/* FIXME: local symbols work as long as the
-			 * core and init pieces aren't separated too
-			 * far.  If this is ever broken, you will trip
-			 * the check below.  The way to fix it would
-			 * be to generate local stubs to go between init
-			 * and core */
-			if((Elf64_Sxword)(val - dot - 8) > 0x800000 -1 ||
-			   (Elf64_Sxword)(val - dot - 8) < -0x800000) {
-				printk(KERN_ERR "Module %s, symbol %s is out of range for PCREL22F relocation\n",
-				       me->name, strtab + sym->st_name);
-				return -ENOEXEC;
-			}
 			val = (val - dot - 8)/4;
+			CHECK_RELOC(val, 22);
 			*loc = (*loc & ~0x3ff1ffd) | reassemble_22(val);
 			break;
 		case R_PARISC_DIR64:
@@ -794,12 +850,8 @@ int module_finalize(const Elf_Ehdr *hdr,
 	addr = (u32 *)entry->addr;
 	printk("INSNS: %x %x %x %x\n",
 	       addr[0], addr[1], addr[2], addr[3]);
-	printk("stubs used %ld, stubs max %ld\n"
-	       "init_stubs used %ld, init stubs max %ld\n"
-	       "got entries used %ld, gots max %ld\n"
+	printk("got entries used %ld, gots max %ld\n"
 	       "fdescs used %ld, fdescs max %ld\n",
-	       me->arch.stub_count, me->arch.stub_max,
-	       me->arch.init_stub_count, me->arch.init_stub_max,
 	       me->arch.got_count, me->arch.got_max,
 	       me->arch.fdesc_count, me->arch.fdesc_max);
 #endif
@@ -829,7 +881,10 @@ int module_finalize(const Elf_Ehdr *hdr,
 				me->name, me->arch.got_count, MAX_GOTS);
 		return -EINVAL;
 	}
-	
+
+	kfree(me->arch.section);
+	me->arch.section = NULL;
+
 	/* no symbol table */
 	if(symhdr == NULL)
 		return 0;

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Rusty Russell]

On Wednesday 31 December 2008 22:01:18 Helge Deller wrote:
> +/* Additional bytes needed by arch in front of individual sections */
> +unsigned int arch_module_section_size(struct module *mod,
> +				      unsigned int section);
> +
...
> +/* Additional bytes needed by arch in front of individual sections */
> +unsigned int __attribute__ ((weak)) arch_module_section_size(
> +		struct module *mod, unsigned int section)
> +{
> +	/* default implementation just returns zero */
> +	return 0;
> +}

Not quite what I had in mind... let me show you:

/* Bytes needed for a section: default is just the section size. */
unsigned int __attribute__((weak))
arch_module_section_size(struct module *mod, Elf_Shdr *sechdrs, unsigned int sec)
{
	return sechdrs[sec].sh_size;
}

Otherwise I'd have called it "arch_module_extra_size()".

Cheers,
Rusty.

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Helge Deller]

Rusty Russell wrote:
> Not quite what I had in mind... let me show you:
> ...
> Otherwise I'd have called it "arch_module_extra_size()".

Hmm, this needs more thinking then.

So, in summary this would be your proposed change (?):

+/* Bytes needed for a section: default is just the section size. */
+unsigned int __attribute__((weak))
+arch_module_section_size(struct module *mod, Elf_Shdr *sechdrs, unsigned int sec)
+{
+       return sechdrs[sec].sh_size;
+}
+
 /* Update size with this section: return offset. */
-static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
+static long get_offset(struct module *mod, unsigned int *size,
+               Elf_Shdr *sechdr, unsigned int section)
 {
        long ret;

        ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
-       *size = ret + sechdr->sh_size;
+       *size = ret + arch_module_section_size(mod, sechdr, section);
        return ret;
 }


This would mean that I can increase the section size in the arch-specific function
by returning a bigger value than sh_size.
This would give me space at the end of the section, but not at the beginning 
(which is what I need), as sh_entsize (the offset into memory) will stay the 
same as before.
Example: Having an initial value for core_size of zero, the code bits of the 
very first section are still copied into the very first byte in memory, leaving
me no room for the stubs.

The important part of get_offset() is, which value is returned to the caller.
Let's try another example which would work for me:

+static long get_offset(struct module *mod, unsigned int *size,
+               Elf_Shdr *sechdr, unsigned int section)
 {
-       long ret;
+       long ret, sect_size;

+       sect_size = arch_module_section_size(mod, sechdr, section);
+       *size += (sect_size - sechdr->sh_size);
        ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
        *size = ret + sechdr->sh_size;
	return ret;

IMHO, this is hackish and ugly.

A last option for me would be to set core_size to the initial value
of bytes which I would need for section 1 and returning in 
arch_module_section_size() when asked for the size of section 1 the 
sum of sh_size[section 1] + additional_bytes_needed_for_section_2,
and so on...

Any proposals?

Helge

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Linus Torvalds]

On Wed, 31 Dec 2008, Helge Deller wrote:
>
> [PATCH 1/2] module.c: fix module loading failure of large kernel modules
> 
> When creating the final layout of a kernel module in memory, allow the
> module loader to reserve some additional memory in front of a given section.
> This is currently only needed for the parisc port which needs to put the
> stub entries there to fulfill the 17/22bit PCREL relocations with large
> kernel modules like xfs.
> 
> Differences of this patch to previous versions:
> - added weak funtion arch_module_section_size()

This doesn't work.

We've had this bug several times now, and one of them just very recently.

Some gcc versions will inline weak functions if they are in scope - even 
if there is a non-weak function somewhere else. So you MUST NOT have the 
weak definition in the same file (or indirectly called through some inline 
functions in a header file) as the call. Because if you do, then any user 
with the wrong version of gcc will get the weak function semantics, even 
if it was meant to be overridden by something else.

> +/* Additional bytes needed by arch in front of individual sections */
> +unsigned int __attribute__ ((weak)) arch_module_section_size(
> +		struct module *mod, unsigned int section)

We don't write out that whole "__attribute__" crud. We use

	unsigned in __weak arch_module_section_size(struct module *mod, unsigned int section)

instead. But as mentioned, it needs to be in another compilation unit.

		Linus

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Roland Dreier]

 > Some gcc versions will inline weak functions if they are in scope - even 
 > if there is a non-weak function somewhere else. So you MUST NOT have the 
 > weak definition in the same file (or indirectly called through some inline 
 > functions in a header file) as the call. Because if you do, then any user 
 > with the wrong version of gcc will get the weak function semantics, even 
 > if it was meant to be overridden by something else.

Does this mean lib/swiotlb.c is broken now?  It has eg:

	void * __weak swiotlb_alloc_boot(size_t size, unsigned long nslabs)

and then

	void __init
	swiotlb_init_with_default_size(size_t default_size)
	{
...
		io_tlb_start = swiotlb_alloc_boot(bytes, io_tlb_nslabs);

later on in the same file.

(I just notice this because I saw the warning about swiotlb_alloc_boot()
not being __init but calling __alloc_bootmem_low and so I looked at the
code yesterday)

 - R.

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Helge Deller]

Linus Torvalds wrote:
> 
> On Wed, 31 Dec 2008, Helge Deller wrote:
>> [PATCH 1/2] module.c: fix module loading failure of large kernel modules
>>
>> When creating the final layout of a kernel module in memory, allow the
>> module loader to reserve some additional memory in front of a given section.
>> This is currently only needed for the parisc port which needs to put the
>> stub entries there to fulfill the 17/22bit PCREL relocations with large
>> kernel modules like xfs.
>>
>> Differences of this patch to previous versions:
>> - added weak funtion arch_module_section_size()
> 
> This doesn't work.
> 
> We've had this bug several times now, and one of them just very recently.
> 
> Some gcc versions will inline weak functions if they are in scope - even 
> if there is a non-weak function somewhere else. So you MUST NOT have the 
> weak definition in the same file (or indirectly called through some inline 
> functions in a header file) as the call. Because if you do, then any user 
> with the wrong version of gcc will get the weak function semantics, even 
> if it was meant to be overridden by something else.

Ok, that might explain why I saw some strange things in the unwind tables
after that I switched to using the weak function (hppa-crosscompiler, gcc-3.3.4).

>> +/* Additional bytes needed by arch in front of individual sections */
>> +unsigned int __attribute__ ((weak)) arch_module_section_size(
>> +		struct module *mod, unsigned int section)
> 
> We don't write out that whole "__attribute__" crud. We use
> 
> 	unsigned in __weak arch_module_section_size(struct module *mod, unsigned int section)
> 
> instead.

Ok.

> But as mentioned, it needs to be in another compilation unit.

Understood.

Rusty, back to you for an advise on how to continue ;-)
(I assume Rusty is just right now celebrating new-year)

Helge

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Linus Torvalds]

On Wed, 31 Dec 2008, Roland Dreier wrote:
> 
> Does this mean lib/swiotlb.c is broken now?

Yes it does.

> It has eg:
>
> 	void * __weak swiotlb_alloc_boot(size_t size, unsigned long nslabs)
> 
> and then
> 
> 	void __init
> 	swiotlb_init_with_default_size(size_t default_size)
> 	{
> ...
> 		io_tlb_start = swiotlb_alloc_boot(bytes, io_tlb_nslabs);
> 
> later on in the same file.

Good eyes. 

Ingo? This comes from commit b81ea27b2329bf44b30c427800954f845896d476, by 
Ian, through Jeremy.

> (I just notice this because I saw the warning about swiotlb_alloc_boot()
> not being __init but calling __alloc_bootmem_low and so I looked at the
> code yesterday)

Lucky us. What's nasty about this is that most developers probably have 
updated versions of gcc, and are then surprised when some odd user has 
insane behavior that doesn't match the source code - because the compiler 
did something unexpected.

I guess I could make a sparse rule for this, but nobody seems to run or 
care about sparse anyway. Sad.

		Linus

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Linus Torvalds]

On Wed, 31 Dec 2008, Linus Torvalds wrote:
> 
> I guess I could make a sparse rule for this, but nobody seems to run or 
> care about sparse anyway. Sad.

Ho humm. Looks like a sparse rule might be in order. These are just from 
my (smallish) personal config. I haven't checked them all, but I did check 
the first two, and yes, sparse did get it right.

  init/main.c:548:24: warning: Calling weak function 'smp_setup_processor_id' in file scope
  init/main.c:674:24: warning: Calling weak function 'thread_info_cache_init' in file scope
  arch/x86/kernel/e820.c:1364:38: warning: Calling weak function 'machine_specific_memory_setup' in file scope
  arch/x86/kernel/e820.c:1371:20: warning: Calling weak function 'memory_setup' in file scope
  arch/x86/kernel/reboot.c:386:22: warning: Calling weak function 'mach_reboot_fixups' in file scope
  kernel/sched.c:7720:26: warning: Calling weak function 'arch_update_cpu_topology' in file scope
  kernel/sched.c:7808:41: warning: Calling weak function 'arch_update_cpu_topology' in file scope
  kernel/fork.c:229:29: warning: Calling weak function 'arch_dup_task_struct' in file scope
  kernel/fork.c:1325:44: warning: Calling weak function 'idle_regs' in file scope
  kernel/sched_clock.c:158:19: warning: Calling weak function 'sched_clock' in file scope
  kernel/sched_clock.c:207:19: warning: Calling weak function 'sched_clock' in file scope
  kernel/time/timekeeping.c:283:43: warning: Calling weak function 'read_persistent_clock' in file scope
  kernel/time/timekeeping.c:318:43: warning: Calling weak function 'read_persistent_clock' in file scope
  kernel/time/timekeeping.c:353:50: warning: Calling weak function 'read_persistent_clock' in file scope
  mm/vmalloc.c:1645:18: warning: Calling weak function 'vmalloc_sync_all' in file scope
  mm/hugetlb.c:1039:32: warning: Calling weak function 'alloc_bootmem_huge_page' in file scope
  mm/sparse.c:448:29: warning: Calling weak function 'vmemmap_populate_print_last' in file scope
  fs/pipe.c:1069:18: warning: Calling weak function 'sys_pipe2' in file scope
  fs/proc/meminfo.c:145:21: warning: Calling weak function 'arch_report_meminfo' in file scope
  drivers/acpi/pci_irq.c:676:21: warning: Calling weak function 'acpi_unregister_gsi' in file scope
  drivers/char/mem.c:168:23: warning: Calling weak function 'unxlate_dev_mem_ptr' in file scope
  drivers/char/mem.c:172:22: warning: Calling weak function 'unxlate_dev_mem_ptr' in file scope
  drivers/char/mem.c:240:23: warning: Calling weak function 'unxlate_dev_mem_ptr' in file scope
  drivers/char/mem.c:246:22: warning: Calling weak function 'unxlate_dev_mem_ptr' in file scope
  drivers/char/mem.c:318:12: warning: Calling weak function 'map_devmem' in file scope
  drivers/char/mem.c:324:14: warning: Calling weak function 'unmap_devmem' in file scope
  drivers/char/mem.c:349:35: warning: Calling weak function 'phys_mem_access_prot_allowed' in file scope
  drivers/char/mem.c:365:15: warning: Calling weak function 'unmap_devmem' in file scope
  drivers/pci/probe.c:1159:36: warning: Calling weak function 'set_pci_bus_resources_arch_default' in file scope
  drivers/pci/pci.c:1015:24: warning: Calling weak function 'pcibios_disable_device' in file scope
  drivers/pci/pci.c:1043:37: warning: Calling weak function 'pcibios_set_pcie_reset_state' in file scope
  drivers/pci/pci-sysfs.c:897:39: warning: Calling weak function 'pcibios_add_platform_entries' in file scope
  drivers/pci/msi.c:49:27: warning: Calling weak function 'arch_setup_msi_irq' in file scope
  drivers/pci/msi.c:69:25: warning: Calling weak function 'arch_teardown_msi_irq' in file scope
  drivers/pci/msi.c:421:27: warning: Calling weak function 'arch_setup_msi_irqs' in file scope
  drivers/pci/msi.c:492:27: warning: Calling weak function 'arch_setup_msi_irqs' in file scope
  drivers/pci/msi.c:562:29: warning: Calling weak function 'arch_msi_check_device' in file scope
  drivers/pci/msi.c:654:24: warning: Calling weak function 'arch_teardown_msi_irqs' in file scope
  lib/swiotlb.c:141:28: warning: Calling weak function 'swiotlb_phys_to_bus' in file scope
  lib/swiotlb.c:146:41: warning: Calling weak function 'swiotlb_bus_to_phys' in file scope
  lib/swiotlb.c:156:28: warning: Calling weak function 'swiotlb_phys_to_bus' in file scope
  lib/swiotlb.c:167:30: warning: Calling weak function 'swiotlb_phys_to_bus' in file scope
  lib/swiotlb.c:168:28: warning: Calling weak function 'swiotlb_phys_to_bus' in file scope
  lib/swiotlb.c:204:35: warning: Calling weak function 'swiotlb_alloc_boot' in file scope
  lib/swiotlb.c:260:31: warning: Calling weak function 'swiotlb_alloc' in file scope
  lib/swiotlb.c:336:58: warning: Calling weak function 'swiotlb_arch_range_needs_mapping' in file scope
  lib/swiotlb.c:336:58: warning: Calling weak function 'swiotlb_arch_range_needs_mapping' in file scope

I bet there are others that I don't see just because I don't compile the 
code.

The appended trial sparse diff is against sparse -git as of a couple of 
days ago if anybody wants to run it themselves.

The _logical_ fix would be to add "noinline" to the definition of 
"__weak", but that's reported not to help. I think the bug may be in the 
assembler that pre-links the call if it sees it in file scope. Or maybe 
gcc doesn't honor noinline for empty functions. Whatever.

Regardless, it's a damn pain.

			Linus

---
 evaluate.c |    7 +++++++
 parse.c    |    7 ++++---
 symbol.h   |    3 ++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index f976645..0ae6c93 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2744,6 +2744,13 @@ static int evaluate_symbol_call(struct expression *expr)
 	if (ctype->op && ctype->op->evaluate)
 		return ctype->op->evaluate(expr);
 
+	if (ctype->ctype.modifiers & MOD_WEAK) {
+		struct symbol *fn = ctype->ctype.base_type;
+
+		if (fn->stmt || fn->inline_stmt)
+			warning(expr->pos, "Calling weak function '%s' in file scope", show_ident(ctype->ident));
+	}
+
 	if (ctype->ctype.modifiers & MOD_INLINE) {
 		int ret;
 		struct symbol *curr = current_fn;
diff --git a/parse.c b/parse.c
index eb31871..1ea497f 100644
--- a/parse.c
+++ b/parse.c
@@ -289,6 +289,9 @@ static struct init_keyword {
 	{ "word",	NS_KEYWORD,	MOD_LONG,	.op = &mode_spec_op },
 	{ "__word__",	NS_KEYWORD,	MOD_LONG,	.op = &mode_spec_op },
 
+	{ "weak",	NS_KEYWORD,	MOD_WEAK,	.op = &attr_mod_op },
+	{ "__weak__",	NS_KEYWORD,	MOD_WEAK,	.op = &attr_mod_op },
+
 	/* Ignored attributes */
 	{ "nothrow",	NS_KEYWORD,	.op = &ignore_attr_op },
 	{ "__nothrow",	NS_KEYWORD,	.op = &ignore_attr_op },
@@ -317,8 +320,6 @@ static struct init_keyword {
 	{ "__sentinel__",	NS_KEYWORD,	.op = &ignore_attr_op },
 	{ "regparm",	NS_KEYWORD,	.op = &ignore_attr_op },
 	{ "__regparm__",	NS_KEYWORD,	.op = &ignore_attr_op },
-	{ "weak",	NS_KEYWORD,	.op = &ignore_attr_op },
-	{ "__weak__",	NS_KEYWORD,	.op = &ignore_attr_op },
 	{ "alias",	NS_KEYWORD,	.op = &ignore_attr_op },
 	{ "__alias__",	NS_KEYWORD,	.op = &ignore_attr_op },
 	{ "pure",	NS_KEYWORD,	.op = &ignore_attr_op },
@@ -1582,7 +1583,7 @@ static struct statement *start_function(struct symbol *sym)
 	start_function_scope();
 	ret = alloc_symbol(sym->pos, SYM_NODE);
 	ret->ctype = sym->ctype.base_type->ctype;
-	ret->ctype.modifiers &= ~(MOD_STORAGE | MOD_CONST | MOD_VOLATILE | MOD_INLINE | MOD_ADDRESSABLE | MOD_NOCAST | MOD_NODEREF | MOD_ACCESSED | MOD_TOPLEVEL);
+	ret->ctype.modifiers &= ~(MOD_STORAGE | MOD_CONST | MOD_VOLATILE | MOD_INLINE | MOD_ADDRESSABLE | MOD_NOCAST | MOD_NODEREF | MOD_ACCESSED | MOD_TOPLEVEL | MOD_WEAK);
 	ret->ctype.modifiers |= (MOD_AUTO | MOD_REGISTER);
 	bind_symbol(ret, &return_ident, NS_ITERATOR);
 	stmt->ret = ret;
diff --git a/symbol.h b/symbol.h
index c4d7f28..b3fcccd 100644
--- a/symbol.h
+++ b/symbol.h
@@ -185,6 +185,7 @@ struct symbol {
 #define MOD_LONGLONG	0x0800
 
 #define MOD_TYPEDEF	0x1000
+#define MOD_WEAK	0x2000
 
 #define MOD_INLINE	0x40000
 #define MOD_ADDRESSABLE	0x80000
@@ -205,7 +206,7 @@ struct symbol {
 #define MOD_BITWISE	0x80000000
 
 #define MOD_NONLOCAL	(MOD_EXTERN | MOD_TOPLEVEL)
-#define MOD_STORAGE	(MOD_AUTO | MOD_REGISTER | MOD_STATIC | MOD_EXTERN | MOD_INLINE | MOD_TOPLEVEL | MOD_FORCE)
+#define MOD_STORAGE	(MOD_AUTO | MOD_REGISTER | MOD_STATIC | MOD_EXTERN | MOD_INLINE | MOD_TOPLEVEL | MOD_FORCE | MOD_WEAK)
 #define MOD_SIGNEDNESS	(MOD_SIGNED | MOD_UNSIGNED | MOD_EXPLICITLY_SIGNED)
 #define MOD_SPECIFIER	(MOD_CHAR | MOD_SHORT | MOD_LONG | MOD_LONGLONG | MOD_SIGNEDNESS)
 #define MOD_SIZE	(MOD_CHAR | MOD_SHORT | MOD_LONG | MOD_LONGLONG)

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Sam Ravnborg]

> 
> I guess I could make a sparse rule for this, but nobody seems to run or 
> care about sparse anyway. Sad.

There is some increased janitorial effort recently.
Try to grep for [Ss]+parse in the shortlog for the last couple
of months.

	Sam

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[James Bottomley]

On Wed, 2008-12-31 at 09:29 -0800, Linus Torvalds wrote:
> 
> On Wed, 31 Dec 2008, Helge Deller wrote:
> >
> > [PATCH 1/2] module.c: fix module loading failure of large kernel modules
> > 
> > When creating the final layout of a kernel module in memory, allow the
> > module loader to reserve some additional memory in front of a given section.
> > This is currently only needed for the parisc port which needs to put the
> > stub entries there to fulfill the 17/22bit PCREL relocations with large
> > kernel modules like xfs.
> > 
> > Differences of this patch to previous versions:
> > - added weak funtion arch_module_section_size()
> 
> This doesn't work.
> 
> We've had this bug several times now, and one of them just very recently.
> 
> Some gcc versions will inline weak functions if they are in scope - even 
> if there is a non-weak function somewhere else. So you MUST NOT have the 
> weak definition in the same file (or indirectly called through some inline 
> functions in a header file) as the call. Because if you do, then any user 
> with the wrong version of gcc will get the weak function semantics, even 
> if it was meant to be overridden by something else.

Um, but we got told to use all these weak functions instead of the
ARCH_HAS... defines.  They certainly litter the x86 boot code.  The
standard pattern was to put the weak function in the file where it was
used, which is what you're now saying will miscompile.

Which gcc versions are the problem?  Because it's going to be a bit
painful catching and killing all the problems ... it might be better
just to patch the master kernel makefile to refuse to build with the
offending gcc versions.

James

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Andrew Morton]

On Wed, 31 Dec 2008 09:47:19 -0800 (PST) Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> > (I just notice this because I saw the warning about swiotlb_alloc_boot()
> > not being __init but calling __alloc_bootmem_low and so I looked at the
> > code yesterday)
> 
> Lucky us. What's nasty about this is that most developers probably have 
> updated versions of gcc, and are then surprised when some odd user has 
> insane behavior that doesn't match the source code - because the compiler 
> did something unexpected.

Adrian claimed that it was gcc-4.1.0 and 4.1.1 only.  He proposed
banning them: http://lkml.org/lkml/2008/8/5/444

> I guess I could make a sparse rule for this, but nobody seems to run or 
> care about sparse anyway. Sad.

No, there are some people who regularly run sparse and fix stuff.  Pending
things here include:

y:/usr/src/25> grep sparse series 
input-ads7846c-sparse-lock-annotation.patch
hugetlb-fix-sparse-warnings.patch
lib-fix-sparse-shadowed-variable-warning.patch
lib-proportionsc-trivial-sparse-lock-annotation.patch
nvidia-fix-sparse-warnings.patch
viafb-fix-sparse-warnings.patch
pm3fb-fix-sparse-warning.patch
neofb-fix-sparse-warnings.patch
i810-fix-sparse-warnings.patch
intelfb-fix-sparse-warnings.patch

Which is a good way for it to be used.

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Linus Torvalds]

On Wed, 31 Dec 2008, Andrew Morton wrote:
> 
> Adrian claimed that it was gcc-4.1.0 and 4.1.1 only.  He proposed
> banning them: http://lkml.org/lkml/2008/8/5/444

If it really is just those releases, then yes, considering the number of 
cases we apparently have, and considering how ugly it is in some cases to 
move the weak function anywhere else, maybe banning those versions is the 
proper thing to do.

It probably won't hurt very many people - yeah, some people will be forced 
to upgrade, but I have this memory of early 4.1 having had other bugs 
anyway, so it's probably a good idea.

		Linus

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 31 Dec 2008 13:22:53 -0800 (PST)

> On Wed, 31 Dec 2008, Andrew Morton wrote:
> > 
> > Adrian claimed that it was gcc-4.1.0 and 4.1.1 only.  He proposed
> > banning them: http://lkml.org/lkml/2008/8/5/444
> 
> If it really is just those releases, then yes, considering the number of 
> cases we apparently have, and considering how ugly it is in some cases to 
> move the weak function anywhere else, maybe banning those versions is the 
> proper thing to do.
> 
> It probably won't hurt very many people - yeah, some people will be forced 
> to upgrade, but I have this memory of early 4.1 having had other bugs 
> anyway, so it's probably a good idea.

I think this is probably the best way to handle this.

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Rusty Russell]

On Thursday 01 January 2009 03:59:36 Linus Torvalds wrote:
> 
> On Wed, 31 Dec 2008, Helge Deller wrote:
> >
> > [PATCH 1/2] module.c: fix module loading failure of large kernel modules
> > 
> > When creating the final layout of a kernel module in memory, allow the
> > module loader to reserve some additional memory in front of a given section.
> > This is currently only needed for the parisc port which needs to put the
> > stub entries there to fulfill the 17/22bit PCREL relocations with large
> > kernel modules like xfs.
> > 
> > Differences of this patch to previous versions:
> > - added weak funtion arch_module_section_size()
> 
> This doesn't work.
> 
> We've had this bug several times now, and one of them just very recently.
> 
> Some gcc versions will inline weak functions if they are in scope

Ah, someone hit this elsewhere and thought this was an arch-specific bug.
Any chance we can just kill those compiler versions and move on with our
lives?  4.1.3 definitely doesn't have the problem.

> We don't write out that whole "__attribute__" crud.

I think what you mean is "I prefer __weak".  Which is fine, and not
trivially disprovable by grep.

It's a gratuitous kernelism, but it's an inoffensive one.
Rusty.

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Rusty Russell]

On Thursday 01 January 2009 00:43:56 Helge Deller wrote:
> Rusty Russell wrote:
> > Not quite what I had in mind... let me show you:
> > ...
> > Otherwise I'd have called it "arch_module_extra_size()".
> 
> This would mean that I can increase the section size in the arch-specific function
> by returning a bigger value than sh_size.
> This would give me space at the end of the section, but not at the beginning 
> (which is what I need), as sh_entsize (the offset into memory) will stay the 
> same as before.

Ah, OK.  I assumed that extra room at the end was sufficient.

So I've taken your previous patch and renamed the function to
arch_mod_section_prepend().  If we decide not to kill those bad gcc versions,
I'll stick it in some other random file (lib/weak.c?  Blech...)

BTW, this is very late for this merge window.  I would have liked to see
this a month ago :(

Subject: module: fix module loading failure of large kernel modules for parisc
Date: Wed, 31 Dec 2008 12:31:18 +0100
From: Helge Deller <deller@gmx.de>

When creating the final layout of a kernel module in memory, allow the
module loader to reserve some additional memory in front of a given section.
This is currently only needed for the parisc port which needs to put the
stub entries there to fulfill the 17/22bit PCREL relocations with large
kernel modules like xfs.

Signed-off-by: Helge Deller <deller@gmx.de>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (renamed fn)

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -12,6 +12,9 @@ int module_frob_arch_sections(Elf_Ehdr *
 			      Elf_Shdr *sechdrs,
 			      char *secstrings,
 			      struct module *mod);
+
+/* Additional bytes needed by arch in front of individual sections */
+unsigned int arch_mod_section_prepend(struct module *mod, unsigned int section);
 
 /* Allocator used for allocating struct module, core sections and init
    sections.  Returns NULL on failure. */
diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1588,11 +1588,21 @@ static int simplify_symbols(Elf_Shdr *se
 	return ret;
 }
 
+/* Additional bytes needed by arch in front of individual sections */
+unsigned int __weak arch_mod_section_prepend(struct module *mod,
+					     unsigned int section)
+{
+	/* default implementation just returns zero */
+	return 0;
+}
+
 /* Update size with this section: return offset. */
-static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
+static long get_offset(struct module *mod, unsigned int *size,
+		       Elf_Shdr *sechdr, unsigned int section)
 {
 	long ret;
 
+	*size += arch_mod_section_prepend(mod, section);
 	ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
 	*size = ret + sechdr->sh_size;
 	return ret;
@@ -1632,7 +1642,7 @@ static void layout_sections(struct modul
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) == 0)
 				continue;
-			s->sh_entsize = get_offset(&mod->core_size, s);
+			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
 		if (m == 0)
@@ -1650,7 +1660,7 @@ static void layout_sections(struct modul
 			    || strncmp(secstrings + s->sh_name,
 				       ".init", 5) != 0)
 				continue;
-			s->sh_entsize = (get_offset(&mod->init_size, s)
+			s->sh_entsize = (get_offset(mod, &mod->init_size, s, i)
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Jeremy Fitzhardinge]

Linus Torvalds wrote:
> Some gcc versions will inline weak functions if they are in scope - even 
> if there is a non-weak function somewhere else. So you MUST NOT have the 
> weak definition in the same file (or indirectly called through some inline 
> functions in a header file) as the call. Because if you do, then any user 
> with the wrong version of gcc will get the weak function semantics, even 
> if it was meant to be overridden by something else.
>   

Yes.  I think this behaviour is considered to be desperately broken by 
the gcc developers and has been fixed in all recent gccs.  There are a 
couple of broken versions, and there have been patches floating around 
to just refuse to use them; otherwise __weak is effectively unusable.  
(Ah, I see Adrian has posted it again and everyone agrees with it.)

On the other hand, I have seen a couple of instances of "inline __weak" 
which is insane, but I don't know if gcc does anything crazy with it, or 
if its common enough to bother warning about.

    J

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Helge Deller]

Rusty Russell wrote:
> On Thursday 01 January 2009 00:43:56 Helge Deller wrote:
>> Rusty Russell wrote:
>>> Not quite what I had in mind... let me show you:
>>> ...
>>> Otherwise I'd have called it "arch_module_extra_size()".
>> This would mean that I can increase the section size in the arch-specific function
>> by returning a bigger value than sh_size.
>> This would give me space at the end of the section, but not at the beginning 
>> (which is what I need), as sh_entsize (the offset into memory) will stay the 
>> same as before.
> 
> Ah, OK.  I assumed that extra room at the end was sufficient.
> 
> So I've taken your previous patch and renamed the function to
> arch_mod_section_prepend().  

Thanks a lot !!!!
Patch below is ok and I'll adjust the parisc-specific patch
acordingly.

> If we decide not to kill those bad gcc versions,
> I'll stick it in some other random file (lib/weak.c?  Blech...)

Andrew has pulled in the patch from Adrian in his -mm tree, so we should
be safe now.

> BTW, this is very late for this merge window.  I would have liked to see
> this a month ago :(

Yes, sorry, I didn't had a patch available earlier.
I hope it's still in-time though, esp. as this will fix one of our longest
outstanding bugs on parisc and is quite important for us.

Thanks,
Helge


> Subject: module: fix module loading failure of large kernel modules for parisc
> Date: Wed, 31 Dec 2008 12:31:18 +0100
> From: Helge Deller <deller@gmx.de>
> 
> When creating the final layout of a kernel module in memory, allow the
> module loader to reserve some additional memory in front of a given section.
> This is currently only needed for the parisc port which needs to put the
> stub entries there to fulfill the 17/22bit PCREL relocations with large
> kernel modules like xfs.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (renamed fn)
> 
> diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
> --- a/include/linux/moduleloader.h
> +++ b/include/linux/moduleloader.h
> @@ -12,6 +12,9 @@ int module_frob_arch_sections(Elf_Ehdr *
>  			      Elf_Shdr *sechdrs,
>  			      char *secstrings,
>  			      struct module *mod);
> +
> +/* Additional bytes needed by arch in front of individual sections */
> +unsigned int arch_mod_section_prepend(struct module *mod, unsigned int section);
>  
>  /* Allocator used for allocating struct module, core sections and init
>     sections.  Returns NULL on failure. */
> diff --git a/kernel/module.c b/kernel/module.c
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1588,11 +1588,21 @@ static int simplify_symbols(Elf_Shdr *se
>  	return ret;
>  }
>  
> +/* Additional bytes needed by arch in front of individual sections */
> +unsigned int __weak arch_mod_section_prepend(struct module *mod,
> +					     unsigned int section)
> +{
> +	/* default implementation just returns zero */
> +	return 0;
> +}
> +
>  /* Update size with this section: return offset. */
> -static long get_offset(unsigned int *size, Elf_Shdr *sechdr)
> +static long get_offset(struct module *mod, unsigned int *size,
> +		       Elf_Shdr *sechdr, unsigned int section)
>  {
>  	long ret;
>  
> +	*size += arch_mod_section_prepend(mod, section);
>  	ret = ALIGN(*size, sechdr->sh_addralign ?: 1);
>  	*size = ret + sechdr->sh_size;
>  	return ret;
> @@ -1632,7 +1642,7 @@ static void layout_sections(struct modul
>  			    || strncmp(secstrings + s->sh_name,
>  				       ".init", 5) == 0)
>  				continue;
> -			s->sh_entsize = get_offset(&mod->core_size, s);
> +			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
>  			DEBUGP("\t%s\n", secstrings + s->sh_name);
>  		}
>  		if (m == 0)
> @@ -1650,7 +1660,7 @@ static void layout_sections(struct modul
>  			    || strncmp(secstrings + s->sh_name,
>  				       ".init", 5) != 0)
>  				continue;
> -			s->sh_entsize = (get_offset(&mod->init_size, s)
> +			s->sh_entsize = (get_offset(mod, &mod->init_size, s, i)
>  					 | INIT_OFFSET_MASK);
>  			DEBUGP("\t%s\n", secstrings + s->sh_name);
>  		}
> 

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Wed, 31 Dec 2008, Andrew Morton wrote:
> > 
> > Adrian claimed that it was gcc-4.1.0 and 4.1.1 only.  He proposed
> > banning them: http://lkml.org/lkml/2008/8/5/444
> 
> If it really is just those releases, then yes, considering the number of 
> cases we apparently have, and considering how ugly it is in some cases 
> to move the weak function anywhere else, maybe banning those versions is 
> the proper thing to do.
> 
> It probably won't hurt very many people - yeah, some people will be 
> forced to upgrade, but I have this memory of early 4.1 having had other 
> bugs anyway, so it's probably a good idea.

That would be _really_ nice to do IMHO: in many cases putting the __weak 
definition into same-file scope with a call site is a natural approach. I 
think that's how we ended up having so many instances of that bug. When 
you use __weak as a 'default implementation' for some function, then it's 
very natural to put it into the same file that also uses it.

It goes into separate, inactive scope only in a few special cases: such as 
when it's some library function that can be overriden by the architecture. 
But if it's some non-libray kernel code then the usage site is close to 
the definition site.

If you look at most of the __weak fixes they IMO actually turned clean 
code into less clean code: they detached some natural clustering of 
definition and callsite.

And __weak is very elegant IMO, it can avoid a lot of #ifdefs and can be 
used to self-document architecture interfaces - so it would be nice to 
make it always work, regardless of the callsite's scope.

	Ingo

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Andrew Morton]

On Thu, 1 Jan 2009 15:24:01 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > 
> > 
> > On Wed, 31 Dec 2008, Andrew Morton wrote:
> > > 
> > > Adrian claimed that it was gcc-4.1.0 and 4.1.1 only.  He proposed
> > > banning them: http://lkml.org/lkml/2008/8/5/444
> > 
> > If it really is just those releases, then yes, considering the number of 
> > cases we apparently have, and considering how ugly it is in some cases 
> > to move the weak function anywhere else, maybe banning those versions is 
> > the proper thing to do.
> > 
> > It probably won't hurt very many people - yeah, some people will be 
> > forced to upgrade, but I have this memory of early 4.1 having had other 
> > bugs anyway, so it's probably a good idea.
> 
> That would be _really_ nice to do IMHO

I wonder if we should do it in -stable too.  Probably yes.

Re: [PATCH] parisc: fix module loading failure of large kernel modules (take 4)

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> > I guess I could make a sparse rule for this, but nobody seems to run 
> > or care about sparse anyway. Sad.
> 
> There is some increased janitorial effort recently. Try to grep for 
> [Ss]+parse in the shortlog for the last couple of months.

in the x86 space there's a lot of Sparse activity as well: a number of 
developers do systematic runs of Sparse and keep the tree Sparse-clean.

The moment this spreads to a critical mass of subsystems we can start 
automating it. (as i was able to automate the checking of -Werr builds on 
x86, with arbitrary random .config's)

	Ingo

[PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 31 Dec 2008 13:22:53 -0800 (PST)
> 
> > On Wed, 31 Dec 2008, Andrew Morton wrote:
> > > 
> > > Adrian claimed that it was gcc-4.1.0 and 4.1.1 only.  He proposed
> > > banning them: http://lkml.org/lkml/2008/8/5/444
> > 
> > If it really is just those releases, then yes, considering the number 
> > of cases we apparently have, and considering how ugly it is in some 
> > cases to move the weak function anywhere else, maybe banning those 
> > versions is the proper thing to do.
> > 
> > It probably won't hurt very many people - yeah, some people will be 
> > forced to upgrade, but I have this memory of early 4.1 having had 
> > other bugs anyway, so it's probably a good idea.
> 
> I think this is probably the best way to handle this.

okay - to move this matter from the discussion-space to the 
solution-space, how about the patch below? (tested on x86 with a 
non-affected compiler version.)

	Ingo

-------------->
>From e6346e5ab54dcf12888a79dfd5402f5de09b3fad Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Fri, 2 Jan 2009 12:46:22 +0100
Subject: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

Impact: fix crashes that can trigger if built with GCC 4.1.0 or 4.1.1

GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
by inlining __weak functions into same-file call sites - breaking the
kernel if the __weak symbol is overriden later on.

In the past we tried to work around this problem by artificially
isolating call site and definition site - but these bugs tend to
pop up regularly:

 43a2563: sparseirq: move __weak symbols into separate compilation unit
 13a0c3c: sparseirq: work around compiler optimizing away __weak functions

And Linus has extended Sparse to report same-file callsites for __weak
symbols - which gave two dozen hits.

We have not found a clean way to work around this bug on the source
code level (noinline and explicit barrier()s are ignored by GCC), so we do
not allow these compilers (which are quite rare these days, have other bugs
and are superceded by the 4.1.2 bugfix release anyway).

Kernel builds under gcc 410 or 411 will now fail with this error message:

  Sorry, your compiler is too old, too buggy or not recognized.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/compiler.h |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index ea7c6be..dd558ce 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -36,12 +36,25 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 
 #ifdef __KERNEL__
 
-#if __GNUC__ >= 4
+/*
+ * GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
+ * by inlining __weak functions into same-file call sites - breaking the
+ * kernel if the __weak symbol is overriden later on.
+ *
+ * We have not found a clean way to work around this bug on the source
+ * code level, so we do not allow these compilers (which are quite
+ * rare these days, have other bugs and are superceded by the 4.1.2
+ * bugfix release anyway):
+ */
+#define gcc41_inlining_bug \
+	(__GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1)
+
+#if __GNUC__ >= 4 && !gcc41_inlining_bug
 # include <linux/compiler-gcc4.h>
 #elif __GNUC__ == 3 && __GNUC_MINOR__ >= 2
 # include <linux/compiler-gcc3.h>
 #else
-# error Sorry, your compiler is too old/not recognized.
+# error Sorry, your compiler is too old, too buggy or not recognized.
 #endif
 
 #define notrace __attribute__((no_instrument_function))

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Bartlomiej Zolnierkiewicz]

On Friday 02 January 2009, Ingo Molnar wrote:
> 
> * David Miller <davem@davemloft.net> wrote:
> 
> > From: Linus Torvalds <torvalds@linux-foundation.org>
> > Date: Wed, 31 Dec 2008 13:22:53 -0800 (PST)
> > 
> > > On Wed, 31 Dec 2008, Andrew Morton wrote:
> > > > 
> > > > Adrian claimed that it was gcc-4.1.0 and 4.1.1 only.  He proposed
> > > > banning them: http://lkml.org/lkml/2008/8/5/444
> > > 
> > > If it really is just those releases, then yes, considering the number 
> > > of cases we apparently have, and considering how ugly it is in some 
> > > cases to move the weak function anywhere else, maybe banning those 
> > > versions is the proper thing to do.
> > > 
> > > It probably won't hurt very many people - yeah, some people will be 
> > > forced to upgrade, but I have this memory of early 4.1 having had 
> > > other bugs anyway, so it's probably a good idea.
> > 
> > I think this is probably the best way to handle this.
> 
> okay - to move this matter from the discussion-space to the 
> solution-space, how about the patch below? (tested on x86 with a 
> non-affected compiler version.)

...or we can just merge Adrian's patch from June 2008 which also fixes
the issue nicely.

OTOH your patch has an advantage of addressing the problem in the more
appropriate place (include/linux/compiler.h) and from what I see allows
us to remove previous gcc 4.1.0 check from init/main.c?

> 	Ingo
> 
> -------------->
> From e6346e5ab54dcf12888a79dfd5402f5de09b3fad Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <mingo@elte.hu>
> Date: Fri, 2 Jan 2009 12:46:22 +0100
> Subject: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1
> 
> Impact: fix crashes that can trigger if built with GCC 4.1.0 or 4.1.1
> 
> GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
> by inlining __weak functions into same-file call sites - breaking the
> kernel if the __weak symbol is overriden later on.
> 
> In the past we tried to work around this problem by artificially
> isolating call site and definition site - but these bugs tend to
> pop up regularly:
> 
>  43a2563: sparseirq: move __weak symbols into separate compilation unit
>  13a0c3c: sparseirq: work around compiler optimizing away __weak functions
> 
> And Linus has extended Sparse to report same-file callsites for __weak
> symbols - which gave two dozen hits.
> 
> We have not found a clean way to work around this bug on the source
> code level (noinline and explicit barrier()s are ignored by GCC), so we do
> not allow these compilers (which are quite rare these days, have other bugs
> and are superceded by the 4.1.2 bugfix release anyway).
> 
> Kernel builds under gcc 410 or 411 will now fail with this error message:
> 
>   Sorry, your compiler is too old, too buggy or not recognized.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  include/linux/compiler.h |   17 +++++++++++++++--
>  1 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index ea7c6be..dd558ce 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -36,12 +36,25 @@ extern void __chk_io_ptr(const volatile void __iomem *);
>  
>  #ifdef __KERNEL__
>  
> -#if __GNUC__ >= 4
> +/*
> + * GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
> + * by inlining __weak functions into same-file call sites - breaking the
> + * kernel if the __weak symbol is overriden later on.
> + *
> + * We have not found a clean way to work around this bug on the source
> + * code level, so we do not allow these compilers (which are quite
> + * rare these days, have other bugs and are superceded by the 4.1.2
> + * bugfix release anyway):
> + */
> +#define gcc41_inlining_bug \
> +	(__GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1)
> +
> +#if __GNUC__ >= 4 && !gcc41_inlining_bug
>  # include <linux/compiler-gcc4.h>
>  #elif __GNUC__ == 3 && __GNUC_MINOR__ >= 2
>  # include <linux/compiler-gcc3.h>
>  #else
> -# error Sorry, your compiler is too old/not recognized.
> +# error Sorry, your compiler is too old, too buggy or not recognized.
>  #endif
>  
>  #define notrace __attribute__((no_instrument_function))

[PATCH] kbuild: Remove gcc 4.1.0 quirk from init/main.c

[Ingo Molnar]

* Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> On Friday 02 January 2009, Ingo Molnar wrote:
> > 
> > * David Miller <davem@davemloft.net> wrote:
> > 
> > > From: Linus Torvalds <torvalds@linux-foundation.org>
> > > Date: Wed, 31 Dec 2008 13:22:53 -0800 (PST)
> > > 
> > > > On Wed, 31 Dec 2008, Andrew Morton wrote:
> > > > > 
> > > > > Adrian claimed that it was gcc-4.1.0 and 4.1.1 only.  He proposed
> > > > > banning them: http://lkml.org/lkml/2008/8/5/444
> > > > 
> > > > If it really is just those releases, then yes, considering the number 
> > > > of cases we apparently have, and considering how ugly it is in some 
> > > > cases to move the weak function anywhere else, maybe banning those 
> > > > versions is the proper thing to do.
> > > > 
> > > > It probably won't hurt very many people - yeah, some people will be 
> > > > forced to upgrade, but I have this memory of early 4.1 having had 
> > > > other bugs anyway, so it's probably a good idea.
> > > 
> > > I think this is probably the best way to handle this.
> > 
> > okay - to move this matter from the discussion-space to the 
> > solution-space, how about the patch below? (tested on x86 with a 
> > non-affected compiler version.)
> 
> ...or we can just merge Adrian's patch from June 2008 which also fixes 
> the issue nicely.

didnt know about that patch, but yeah, sure.

> OTOH your patch has an advantage of addressing the problem in the more 
> appropriate place (include/linux/compiler.h) and from what I see allows 
> us to remove previous gcc 4.1.0 check from init/main.c?

Good spotting - find followup cleanup patch below.

	Ingo

------------------->
>From d23cbaaa342e5555a919a543095d656415a55950 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Fri, 2 Jan 2009 16:16:16 +0100
Subject: [PATCH] kbuild: Remove gcc 4.1.0 quirk from init/main.c

Impact: cleanup

We now have a cleaner check for gcc 4.1.0/4.1.1 trouble in
include/linux/compiler.h, so remove the 4.1.0 quirk from
init/main.c.

Reported-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 init/main.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/init/main.c b/init/main.c
index 2a7ce0f..5ced153 100644
--- a/init/main.c
+++ b/init/main.c
@@ -75,15 +75,6 @@
 #include <asm/smp.h>
 #endif
 
-/*
- * This is one of the first .c files built. Error out early if we have compiler
- * trouble.
- */
-
-#if __GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ == 0
-#warning gcc-4.1.0 is known to miscompile the kernel.  A different compiler version is recommended.
-#endif
-
 static int kernel_init(void *);
 
 extern void init_IRQ(void);

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Linus Torvalds]

On Fri, 2 Jan 2009, Ingo Molnar wrote:
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -36,12 +36,25 @@ extern void __chk_io_ptr(const volatile void __iomem *);
>  
>  #ifdef __KERNEL__
>  
> -#if __GNUC__ >= 4
> +/*
> + * GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
> + * by inlining __weak functions into same-file call sites - breaking the
> + * kernel if the __weak symbol is overriden later on.
> + *
> + * We have not found a clean way to work around this bug on the source
> + * code level, so we do not allow these compilers (which are quite
> + * rare these days, have other bugs and are superceded by the 4.1.2
> + * bugfix release anyway):
> + */
> +#define gcc41_inlining_bug \
> +	(__GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1)
> +
> +#if __GNUC__ >= 4 && !gcc41_inlining_bug
>  # include <linux/compiler-gcc4.h>

I think this is wrong.

Just move the check into <linux/compiler-gcc4.h>

It makes no sense to do stuff that is specific to gcc4 in the general gcc 
header file. It seems you did this just in order to re-use a (bad) generic 
error case. 

			Linus

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Linus Torvalds]

On Fri, 2 Jan 2009, Linus Torvalds wrote:
> 
> I think this is wrong.
> 
> Just move the check into <linux/compiler-gcc4.h>

In fact, looking at that whole mess, I redid it all. It was disgusting how 
conditionals in gcc4.h needed to double-check that __GNUC__ really was 4 
(rather than something bigger), which largely negated the whole nice clean 
compiler version separation.

I pushed out my preferred version, which fixes up the whole thing. The 
gcc4 header file now only gets included for __GNUC__ == 4, and when we 
ever see a __GNUC__ of 5, it will automatically DTRT and try to include 
<linux/compiler-gcc5.h> instead of #4.

And then the check in gcc4.h for 4.1.0 and 4.1.1 is much simplified.  The 
two patches I pushed out add functionality, but don't actually add any new 
lines (the first simplification patch removes more lines than it adds, and 
the second one that adds the __weak bug test adds as mahy lines as the 
cleanup removed).

And it all looks more logical too, imho.

		Linus

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Fri, 2 Jan 2009, Ingo Molnar wrote:
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -36,12 +36,25 @@ extern void __chk_io_ptr(const volatile void __iomem *);
> >  
> >  #ifdef __KERNEL__
> >  
> > -#if __GNUC__ >= 4
> > +/*
> > + * GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
> > + * by inlining __weak functions into same-file call sites - breaking the
> > + * kernel if the __weak symbol is overriden later on.
> > + *
> > + * We have not found a clean way to work around this bug on the source
> > + * code level, so we do not allow these compilers (which are quite
> > + * rare these days, have other bugs and are superceded by the 4.1.2
> > + * bugfix release anyway):
> > + */
> > +#define gcc41_inlining_bug \
> > +	(__GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1)
> > +
> > +#if __GNUC__ >= 4 && !gcc41_inlining_bug
> >  # include <linux/compiler-gcc4.h>
> 
> I think this is wrong.
> 
> Just move the check into <linux/compiler-gcc4.h>
> 
> It makes no sense to do stuff that is specific to gcc4 in the general 
> gcc header file. It seems you did this just in order to re-use a (bad) 
> generic error case.

yeah. I first hacked the generic check then saw how ugly the end result 
was and moved it one level higher. Which was less ugly than where it came 
from and not much worse than the starting point (so it passed my filters) 
but still not clean enough (so it didnt pass your filters).

How about the patch below instead? It cleans up the generic check by 
splitting all the per-major-version checks out into gcc4 and gcc3.

(still untested)

	Ingo

------------->
>From bb951ce0794f0e5974b834eb14e974a0a2c119db Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Fri, 2 Jan 2009 12:46:22 +0100
Subject: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

Impact: fix crashes that can trigger if built with GCC 4.1.0 or 4.1.1

GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
by inlining __weak functions into same-file call sites - breaking the
kernel if the __weak symbol is overriden later on.

In the past we tried to work around this problem by artificially
isolating call site and definition site - but these bugs tend to
pop up regularly:

 43a2563: sparseirq: move __weak symbols into separate compilation unit
 13a0c3c: sparseirq: work around compiler optimizing away __weak functions

And Linus has extended Sparse to report same-file callsites for __weak
symbols - which gave two dozen hits.

We have not found a clean way to work around this bug on the source
code level (noinline and explicit barrier()s are ignored by GCC), so we do
not allow these compilers (which are quite rare these days, have other bugs
and are superceded by the 4.1.2 bugfix release anyway).

Kernel builds under gcc 410 or 411 will now fail with this error message:

  Sorry, GCC 4.1.0/4.1.1 are too buggy to build the kernel - please
  upgrade to 4.1.2 or later versions.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/compiler-gcc3.h |    4 ++++
 include/linux/compiler-gcc4.h |   14 ++++++++++++++
 include/linux/compiler.h      |    4 ++--
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/linux/compiler-gcc3.h b/include/linux/compiler-gcc3.h
index e5eb795..a929a6d 100644
--- a/include/linux/compiler-gcc3.h
+++ b/include/linux/compiler-gcc3.h
@@ -2,6 +2,10 @@
 #error "Please don't include <linux/compiler-gcc3.h> directly, include <linux/compiler.h> instead."
 #endif
 
+#if __GNUC_MINOR__ < 2
+# error Sorry, your compiler is too old - please upgrade it.
+#endif
+
 /* These definitions are for GCC v3.x.  */
 #include <linux/compiler-gcc.h>
 
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 974f5b7..b1edc9d 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -2,6 +2,20 @@
 #error "Please don't include <linux/compiler-gcc4.h> directly, include <linux/compiler.h> instead."
 #endif
 
+/*
+ * GCC 4.1.0 and 4.1.1 has a bug that can miscompile __weak symbols,
+ * by inlining __weak functions into same-file call sites - breaking the
+ * kernel if the __weak symbol is overriden later on.
+ *
+ * We have not found a clean way to work around this bug on the source
+ * code level, so we do not allow these compilers (which are quite
+ * rare these days, have other bugs and are superceded by the 4.1.2
+ * bugfix release anyway):
+ */
+#if __GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1
+# error Sorry, GCC 4.1.0/4.1.1 are too buggy to build the kernel - please upgrade to 4.1.2 or later versions.
+#endif
+
 /* These definitions are for GCC v4.x.  */
 #include <linux/compiler-gcc.h>
 
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index ea7c6be..18edc7a 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -38,10 +38,10 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 
 #if __GNUC__ >= 4
 # include <linux/compiler-gcc4.h>
-#elif __GNUC__ == 3 && __GNUC_MINOR__ >= 2
+#elif __GNUC__ == 3
 # include <linux/compiler-gcc3.h>
 #else
-# error Sorry, your compiler is too old/not recognized.
+# error Sorry, your compiler is too old or not recognized.
 #endif
 
 #define notrace __attribute__((no_instrument_function))

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Fri, 2 Jan 2009, Linus Torvalds wrote:
> > 
> > I think this is wrong.
> > 
> > Just move the check into <linux/compiler-gcc4.h>
> 
> In fact, looking at that whole mess, I redid it all. It was disgusting 
> how conditionals in gcc4.h needed to double-check that __GNUC__ really 
> was 4 (rather than something bigger), which largely negated the whole 
> nice clean compiler version separation.
> 
> I pushed out my preferred version, which fixes up the whole thing. The 
> gcc4 header file now only gets included for __GNUC__ == 4, and when we 
> ever see a __GNUC__ of 5, it will automatically DTRT and try to include 
> <linux/compiler-gcc5.h> instead of #4.
> 
> And then the check in gcc4.h for 4.1.0 and 4.1.1 is much simplified.  
> The two patches I pushed out add functionality, but don't actually add 
> any new lines (the first simplification patch removes more lines than it 
> adds, and the second one that adds the __weak bug test adds as mahy 
> lines as the cleanup removed).
> 
> And it all looks more logical too, imho.

yeah, agreed, much cleaner.

The gcc3 check for gcc 3.0 and 3.1 can be pushed into gcc3 as well - not 
sure whether you fixed that. (i fixed it in the patch i just sent - but i 
didnt notice the gcc5 mess in gcc4.h that you fixed)

	Ingo

[PATCH] Disallow gcc versions 3.{0,1}

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > And it all looks more logical too, imho.
> 
> yeah, agreed, much cleaner.
> 
> The gcc3 check for gcc 3.0 and 3.1 can be pushed into gcc3 as well - not 
> sure whether you fixed that. (i fixed it in the patch i just sent - but 
> i didnt notice the gcc5 mess in gcc4.h that you fixed)

ok, i see them now:

 f9d1425: Disallow gcc versions 4.1.{0,1}
 f153b82: Sanitize gcc version header includes

agreed, much cleaner.

One small issue - i think this now allows gcc 3.0 and 3.1 again - which we 
didnt before. Dont we need the patch below - am i missing something?

	Ingo

------------------->
>From 1458f25412a838968e845ec0bc1b18db70cba7cd Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Fri, 2 Jan 2009 18:53:14 +0100
Subject: [PATCH] Disallow gcc versions 3.{0,1}

GCC 3.0 and 3.1 are too old to build a working kernel.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/compiler-gcc3.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/compiler-gcc3.h b/include/linux/compiler-gcc3.h
index 2befe65..8005eff 100644
--- a/include/linux/compiler-gcc3.h
+++ b/include/linux/compiler-gcc3.h
@@ -2,6 +2,10 @@
 #error "Please don't include <linux/compiler-gcc3.h> directly, include <linux/compiler.h> instead."
 #endif
 
+#if __GNUC_MINOR__ < 2
+# error Sorry, your compiler is too old - please upgrade it.
+#endif
+
 #if __GNUC_MINOR__ >= 3
 # define __used			__attribute__((__used__))
 #else

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Sam Ravnborg]

On Fri, Jan 02, 2009 at 09:38:14AM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 2 Jan 2009, Linus Torvalds wrote:
> > 
> > I think this is wrong.
> > 
> > Just move the check into <linux/compiler-gcc4.h>
> 
> In fact, looking at that whole mess, I redid it all. It was disgusting how 
> conditionals in gcc4.h needed to double-check that __GNUC__ really was 4 
> (rather than something bigger), which largely negated the whole nice clean 
> compiler version separation.
> 
> I pushed out my preferred version, which fixes up the whole thing. The 
> gcc4 header file now only gets included for __GNUC__ == 4, and when we 
> ever see a __GNUC__ of 5, it will automatically DTRT and try to include 
> <linux/compiler-gcc5.h> instead of #4.
> 
> And then the check in gcc4.h for 4.1.0 and 4.1.1 is much simplified.  The 
> two patches I pushed out add functionality, but don't actually add any new 
> lines (the first simplification patch removes more lines than it adds, and 
> the second one that adds the __weak bug test adds as mahy lines as the 
> cleanup removed).
> 
> And it all looks more logical too, imho.

Bugger....
Now I cannot do cross compile for: alpha, arm, m68k and sparc.

Not that I actually try to run these beasts but just being able
to do cross compile has served me well.

Last I looked (only few days ago) crosstool-ng did not support sparc :-(

	Sam

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Linus Torvalds]

On Fri, 2 Jan 2009, Ingo Molnar wrote:
> 
> The gcc3 check for gcc 3.0 and 3.1 can be pushed into gcc3 as well - not 
> sure whether you fixed that. (i fixed it in the patch i just sent - but i 
> didnt notice the gcc5 mess in gcc4.h that you fixed)

I just dropped it. Gcc 3.0 and 3.1 are so old that we simply don't care. 

Nobody who has compiled a kernel in the last two years can possibly have 
those versions: we've failed the build on them since December 2006. And 
there obviously won't be any new users either. So consider it a 
generational garbage collection event.

Of course, if we really want to check for old compiler versions, we can 
add the check back (to the gcc3 header file where it belongs), but it does 
seem to be entirely historical.

			Linus

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Fri, 2 Jan 2009, Ingo Molnar wrote:
> > 
> > The gcc3 check for gcc 3.0 and 3.1 can be pushed into gcc3 as well - not 
> > sure whether you fixed that. (i fixed it in the patch i just sent - but i 
> > didnt notice the gcc5 mess in gcc4.h that you fixed)
> 
> I just dropped it. Gcc 3.0 and 3.1 are so old that we simply don't care.

we didnt support it for quite some time. If you look at that ugly version 
check you replaced:

 #if __GNUC__ >= 4 && !gcc41_inlining_bug
 # include <linux/compiler-gcc4.h>
 #elif __GNUC__ == 3 && __GNUC_MINOR__ >= 2
 # include <linux/compiler-gcc3.h>
 #else
 # error Sorry, your compiler is too old, too buggy or not recognized.
 #endif

it has a hidden 'gcc 3.0 / 3.1 is not supported' condition in it. So what 
i tried to point out that your patch causes a regression here - we dont 
filter out gcc 3.0/3.1 out anymore.

	Ingo

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Linus Torvalds]

On Fri, 2 Jan 2009, Sam Ravnborg wrote:
> 
> Bugger....
> Now I cannot do cross compile for: alpha, arm, m68k and sparc.
> 
> Not that I actually try to run these beasts but just being able
> to do cross compile has served me well.

We _could_ make a "CONFIG_COMPILE_ONLY" check, but wouldn't it be even 
nicer to make sure the cross-compiles are something that might actually be 
expected to work?

I realize that cross-tools tend to lag a bit - the pressure to maintain 
them tends to be much lower - but I was sure we had somebody who did a 
reasonable cross-compiler toolchain. Is gcc-4.1 really the most modern 
thing that is easily available?

			Linus

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Linus Torvalds]

On Fri, 2 Jan 2009, Ingo Molnar wrote:
> 
> So what i tried to point out that your patch causes a regression here - 
> we dont filter out gcc 3.0/3.1 out anymore.

What I tried to tell you was that we don't care - nobody can have it 
anyway. If somebody tried to compile the kernel with it, we've already 
refused for the last 2+ years. So there are no old users out there. And 
there certainly aren't any new ones.

			Linus

Re: [PATCH] kbuild: Remove gcc 4.1.0 quirk from init/main.c

[Sam Ravnborg]

> From d23cbaaa342e5555a919a543095d656415a55950 Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <mingo@elte.hu>
> Date: Fri, 2 Jan 2009 16:16:16 +0100
> Subject: [PATCH] kbuild: Remove gcc 4.1.0 quirk from init/main.c
> 
> Impact: cleanup
> 
> We now have a cleaner check for gcc 4.1.0/4.1.1 trouble in
> include/linux/compiler.h, so remove the 4.1.0 quirk from
> init/main.c.
> 
> Reported-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>

This should be applied despite Linus' different approach in compiler.h.

Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  init/main.c |    9 ---------
>  1 files changed, 0 insertions(+), 9 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index 2a7ce0f..5ced153 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -75,15 +75,6 @@
>  #include <asm/smp.h>
>  #endif
>  
> -/*
> - * This is one of the first .c files built. Error out early if we have compiler
> - * trouble.
> - */
> -
> -#if __GNUC__ == 4 && __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ == 0
> -#warning gcc-4.1.0 is known to miscompile the kernel.  A different compiler version is recommended.
> -#endif
> -
>  static int kernel_init(void *);
>  
>  extern void init_IRQ(void);

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Linus Torvalds]

On Fri, 2 Jan 2009, Linus Torvalds wrote:
> 
> What I tried to tell you was that we don't care - nobody can have it 
> anyway. If somebody tried to compile the kernel with it, we've already 
> refused for the last 2+ years. So there are no old users out there. And 
> there certainly aren't any new ones.

.. but we cetrtainly can add the check back easily enough if you really 
want to, of course. I just don't think there is much point.

			Linus

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> > And it all looks more logical too, imho.
> 
> Bugger....
> Now I cannot do cross compile for: alpha, arm, m68k and sparc.

hm, i just did a successful cross-build from x86 to alpha:

  KSYM    .tmp_kallsyms2.S
  AS      .tmp_kallsyms2.o
  LD      vmlinux
  SYSMAP  System.map
  SYSMAP  .tmp_System.map

  phoenix:~/linux/linux> head -6 /dev/shm/tip/build/.config 
  #
  # Automatically generated make config: don't edit
  # Linux kernel version: 2.6.28
  # Fri Jan  2 19:28:14 2009
  #
  CONFIG_ALPHA=y

with these commits present:

  f9d1425: Disallow gcc versions 4.1.{0,1}
  f153b82: Sanitize gcc version header includes

what type of cross-build breakage do they cause?

	Ingo

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Sam Ravnborg]

On Fri, Jan 02, 2009 at 10:04:27AM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 2 Jan 2009, Sam Ravnborg wrote:
> > 
> > Bugger....
> > Now I cannot do cross compile for: alpha, arm, m68k and sparc.
> > 
> > Not that I actually try to run these beasts but just being able
> > to do cross compile has served me well.
> 
> We _could_ make a "CONFIG_COMPILE_ONLY" check, but wouldn't it be even 
> nicer to make sure the cross-compiles are something that might actually be 
> expected to work?
> 
> I realize that cross-tools tend to lag a bit - the pressure to maintain 
> them tends to be much lower - but I was sure we had somebody who did a 
> reasonable cross-compiler toolchain. Is gcc-4.1 really the most modern 
> thing that is easily available?

Asked google and it found following page:

    http://www.kernel.org/pub/tools/crosstool/

Architecuter   OK?    gcc version
--------------------------------- 
alpha          No     gcc 4.0.2
arm            Yes    gcc 3.4.5
ia64           Yes    gcc 3.4.5
m68k           Yes    gcc 3.4.5
mips           Yes    gcc 3.4.5
sh4            Yes    gcc 3.4.5
sparc          Yes    gcc 3.4.5
sparc64        Yes    gcc 3.4.5
x86_64         Yes    gcc 3.4.5


So from this list of tool chains we can continue to do cross builds
of all except alpha.
But the gcc version is getting ancient..

Why it shall be so hard to do cross build toolchains is above my
imagination but then I also never looked at what it involes.

Added Vegard that maintain these pages.

	Sam

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Randy Dunlap]

Sam Ravnborg wrote:
> On Fri, Jan 02, 2009 at 10:04:27AM -0800, Linus Torvalds wrote:
>>
>> On Fri, 2 Jan 2009, Sam Ravnborg wrote:
>>> Bugger....
>>> Now I cannot do cross compile for: alpha, arm, m68k and sparc.
>>>
>>> Not that I actually try to run these beasts but just being able
>>> to do cross compile has served me well.
>> We _could_ make a "CONFIG_COMPILE_ONLY" check, but wouldn't it be even 
>> nicer to make sure the cross-compiles are something that might actually be 
>> expected to work?
>>
>> I realize that cross-tools tend to lag a bit - the pressure to maintain 
>> them tends to be much lower - but I was sure we had somebody who did a 
>> reasonable cross-compiler toolchain. Is gcc-4.1 really the most modern 
>> thing that is easily available?
> 
> Asked google and it found following page:
> 
>     http://www.kernel.org/pub/tools/crosstool/
> 
> Architecuter   OK?    gcc version
> --------------------------------- 
> alpha          No     gcc 4.0.2
> arm            Yes    gcc 3.4.5
> ia64           Yes    gcc 3.4.5
> m68k           Yes    gcc 3.4.5
> mips           Yes    gcc 3.4.5
> sh4            Yes    gcc 3.4.5
> sparc          Yes    gcc 3.4.5
> sparc64        Yes    gcc 3.4.5
> x86_64         Yes    gcc 3.4.5
> 
> 
> So from this list of tool chains we can continue to do cross builds
> of all except alpha.
> But the gcc version is getting ancient..
> 
> Why it shall be so hard to do cross build toolchains is above my
> imagination but then I also never looked at what it involes.
> 
> Added Vegard that maintain these pages.

I doubt that these are any more recent, but the filenames don't say:

http://userweb.kernel.org/~akpm/cross-compilers/


Yes, it is an ongoing problem.

-- 
~Randy

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Sam Ravnborg]

On Fri, Jan 02, 2009 at 07:22:48PM +0100, Ingo Molnar wrote:
> 
> * Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > > And it all looks more logical too, imho.
> > 
> > Bugger....
> > Now I cannot do cross compile for: alpha, arm, m68k and sparc.
> 
> hm, i just did a successful cross-build from x86 to alpha:
> 
>   KSYM    .tmp_kallsyms2.S
>   AS      .tmp_kallsyms2.o
>   LD      vmlinux
>   SYSMAP  System.map
>   SYSMAP  .tmp_System.map
> 
>   phoenix:~/linux/linux> head -6 /dev/shm/tip/build/.config 
>   #
>   # Automatically generated make config: don't edit
>   # Linux kernel version: 2.6.28
>   # Fri Jan  2 19:28:14 2009
>   #
>   CONFIG_ALPHA=y
> 
> with these commits present:
> 
>   f9d1425: Disallow gcc versions 4.1.{0,1}
>   f153b82: Sanitize gcc version header includes
> 
> what type of cross-build breakage do they cause?

The default gcc made by demo-alpha with crosstool is version 4.1.0:

 /opt/crosstool/gcc-4.1.0-glibc-2.3.6/alpha-unknown-linux-gnu/bin/alpha-unknown-linux-gnu-gcc --version
alpha-unknown-linux-gnu-gcc (GCC) 4.1.0
Copyright (C) 2006 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

	Sam

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> On Fri, Jan 02, 2009 at 07:22:48PM +0100, Ingo Molnar wrote:
> > 
> > * Sam Ravnborg <sam@ravnborg.org> wrote:
> > 
> > > > And it all looks more logical too, imho.
> > > 
> > > Bugger....
> > > Now I cannot do cross compile for: alpha, arm, m68k and sparc.
> > 
> > hm, i just did a successful cross-build from x86 to alpha:
> > 
> >   KSYM    .tmp_kallsyms2.S
> >   AS      .tmp_kallsyms2.o
> >   LD      vmlinux
> >   SYSMAP  System.map
> >   SYSMAP  .tmp_System.map
> > 
> >   phoenix:~/linux/linux> head -6 /dev/shm/tip/build/.config 
> >   #
> >   # Automatically generated make config: don't edit
> >   # Linux kernel version: 2.6.28
> >   # Fri Jan  2 19:28:14 2009
> >   #
> >   CONFIG_ALPHA=y
> > 
> > with these commits present:
> > 
> >   f9d1425: Disallow gcc versions 4.1.{0,1}
> >   f153b82: Sanitize gcc version header includes
> > 
> > what type of cross-build breakage do they cause?
> 
> The default gcc made by demo-alpha with crosstool is version 4.1.0:
> 
>  /opt/crosstool/gcc-4.1.0-glibc-2.3.6/alpha-unknown-linux-gnu/bin/alpha-unknown-linux-gnu-gcc --version
> alpha-unknown-linux-gnu-gcc (GCC) 4.1.0
> Copyright (C) 2006 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

ah, ok. I build cross-compilers from scratch, so i have 4.3.3-pre:

 phoenix:~/linux/linux> /opt/crossgcc/cross/bin/alpha-linux-gcc -v
 [...]
 gcc version 4.3.3 20081123 (prerelease) (GCC) 

	Ingo

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Al Viro]

On Fri, Jan 02, 2009 at 10:04:27AM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 2 Jan 2009, Sam Ravnborg wrote:
> > 
> > Bugger....
> > Now I cannot do cross compile for: alpha, arm, m68k and sparc.
> > 
> > Not that I actually try to run these beasts but just being able
> > to do cross compile has served me well.
> 
> We _could_ make a "CONFIG_COMPILE_ONLY" check, but wouldn't it be even 
> nicer to make sure the cross-compiles are something that might actually be 
> expected to work?
> 
> I realize that cross-tools tend to lag a bit - the pressure to maintain 
> them tends to be much lower - but I was sure we had somebody who did a 
> reasonable cross-compiler toolchain. Is gcc-4.1 really the most modern 
> thing that is easily available?

FWIW, I'm using 4.3 on all targets at the moment.  See
git://git.kernel.org/pub/scm/linux/kernel/git/viro/toolchain.git/
for fedora-based variant of that sucker.  And yes, it does include
cross-to-sparc; all but sh/sh64, in fact (sh had serious compiler
breakage around 4.3.0 and backporting from -HEAD was far beyond
what I considered reasonable at that point).

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Detlef Riekenberg]

* Ingo Molnar wrote:
> * Sam Ravnborg <sam@ravnborg.org> wrote:
 
> > > what type of cross-build breakage do they cause?
> > 
> > The default gcc made by demo-alpha with crosstool is version 4.1.0:
> > 
> >  /opt/crosstool/gcc-4.1.0-glibc-2.3.6/alpha-unknown-linux-gnu/bin/alpha-unknown-linux-gnu-gcc --version
> > alpha-unknown-linux-gnu-gcc (GCC) 4.1.0
> > Copyright (C) 2006 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions.  There is NO
> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> ah, ok. I build cross-compilers from scratch, so i have 4.3.3-pre:
> 
>  phoenix:~/linux/linux> /opt/crossgcc/cross/bin/alpha-linux-gcc -v
>  [...]
>  gcc version 4.3.3 20081123 (prerelease) (GCC) 
> 
> 	Ingo

I forwarded the thread to Dan Kegel to make him aware, that the used gcc
version in crosstools makes problems.


-- 
 
Bye bye ... Detlef

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Andi Kleen]

Al Viro <viro@ZenIV.linux.org.uk> writes:
>
> FWIW, I'm using 4.3 on all targets at the moment.  See
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/toolchain.git/
> for fedora-based variant of that sucker.  And yes, it does include
> cross-to-sparc; all but sh/sh64, in fact (sh had serious compiler
> breakage around 4.3.0 and backporting from -HEAD was far beyond
> what I considered reasonable at that point).

The full opensuse distribution also has a couple of spec files for
generating cross compilers from recent versions. These are for
icecream, but can be relatively easily adapted (or the binaries
reused)

In general I don't think building cross compilers is as hard
as it used to be, so it can be reasonably done without any support
scripts too.

Really no excuse to still use the old crosstool crap :)

-Andi

-- 
ak@linux.intel.com

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Willy Tarreau]

On Fri, Jan 02, 2009 at 10:05:47AM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 2 Jan 2009, Ingo Molnar wrote:
> > 
> > So what i tried to point out that your patch causes a regression here - 
> > we dont filter out gcc 3.0/3.1 out anymore.
> 
> What I tried to tell you was that we don't care - nobody can have it 
> anyway. If somebody tried to compile the kernel with it, we've already 
> refused for the last 2+ years. So there are no old users out there. And 
> there certainly aren't any new ones.

Yes some people (including me) still have them and are happy that there
is a check to prevent accidental builds with an inappropriate compiler.

Willy

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Linus Torvalds]

On Fri, 2 Jan 2009, Willy Tarreau wrote:
> 
> Yes some people (including me) still have them and are happy that there
> is a check to prevent accidental builds with an inappropriate compiler.

Heh, ok. If people really do actively have that version, let's add the 
check back in.

		Linus

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Benjamin Herrenschmidt]

> 
> Bugger....
> Now I cannot do cross compile for: alpha, arm, m68k and sparc.
> 
> Not that I actually try to run these beasts but just being able
> to do cross compile has served me well.
> 
> Last I looked (only few days ago) crosstool-ng did not support sparc :-(

There are also various variants of 4.1.1 around with the fix for
that inline bug backported (I think that's the case of the compiler
that ships with the Cell SDK for example).

What a f. mess :-(

Cheers,
Ben.

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Sam Ravnborg]

On Sat, Jan 03, 2009 at 09:27:33AM +1100, Benjamin Herrenschmidt wrote:
> 
> > 
> > Bugger....
> > Now I cannot do cross compile for: alpha, arm, m68k and sparc.
> > 
> > Not that I actually try to run these beasts but just being able
> > to do cross compile has served me well.
> > 
> > Last I looked (only few days ago) crosstool-ng did not support sparc :-(
> 
> There are also various variants of 4.1.1 around with the fix for
> that inline bug backported (I think that's the case of the compiler
> that ships with the Cell SDK for example).
> 

We could add a feature check during the early phase of the
kernel build so we know if gcc has this bug or not.

But this would be the first time to do so IIRC - today we only
do some limited check to see if certain options are supported.

	Sam

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Al Viro]

On Fri, Jan 02, 2009 at 08:14:58PM +0100, Andi Kleen wrote:
> Al Viro <viro@ZenIV.linux.org.uk> writes:
> >
> > FWIW, I'm using 4.3 on all targets at the moment.  See
> > git://git.kernel.org/pub/scm/linux/kernel/git/viro/toolchain.git/
> > for fedora-based variant of that sucker.  And yes, it does include
> > cross-to-sparc; all but sh/sh64, in fact (sh had serious compiler
> > breakage around 4.3.0 and backporting from -HEAD was far beyond
> > what I considered reasonable at that point).
> 
> The full opensuse distribution also has a couple of spec files for
> generating cross compilers from recent versions. These are for
> icecream, but can be relatively easily adapted (or the binaries
> reused)
> 
> In general I don't think building cross compilers is as hard
> as it used to be, so it can be reasonably done without any support
> scripts too.

*snort*

Well, the only support script here is "call rpmbuild with the right
arguments" (and kmk, which is about running kernel cross-build
conveniently and has nothing to do with building cross-toolchain
itself).

I'll need to update that to more current gcc anyway (and see if that
takes care of sh/sh64), so if you can dig those .spec out... throw
them my way and I'll add them to repository.

How well do they handle the targets for which you have no glibc-dev
binary rpms, BTW?  I needed headers for ia64 and ppc64; both fortunately
are among the generally supported targets...

Re: [PATCH] kbuild: Disallow GCC 4.1.0 / 4.1.1

[Krzysztof Halasa]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> I realize that cross-tools tend to lag a bit - the pressure to maintain 
> them tends to be much lower - but I was sure we had somebody who did a 
> reasonable cross-compiler toolchain. Is gcc-4.1 really the most modern 
> thing that is easily available?

Not sure about crosstool and other platforms but I'm using gcc 4.3.2+
for ARM (on x86_64) and building it is straightforward. Just unpack
binutils, gcc (and optionally glibc); configure; make; make install.
Prerequisites: kernel headers and minimal glibc installed.
(Some recent glibc versions require a patch or two).
-- 
Krzysztof Halasa