remove modpost false warnings on ARM

PATCH In review David Brownell 4 participants 6 messages Jan 12, 2007 – Feb 16, 2007 Kbuild

Share & tools

David Brownell

author

2007-01-12 16:31 UTC #0 lore raw

PATCH 1/1 remove modpost false warnings on ARM

This patch stops "modpost" from issuing erroneous modpost warnings on ARM builds, which it's been doing simce since maybe last summer. A canonical example would be driver method table entries:

WARNING: <path> - Section mismatch: reference to .exit.text:<name>_remove

	from .data after '$d' (at offset 0x4)

That "$d" symbol is generated by tools conformant with ARM ABI specs; in this case, it's a relocation in the start of a "<name>_driver" struct. The erroneous warnings appear to be issued because "modpost" whitelists references from "<name>_driver" data into init and exit sections ... but does NOT whitelist them from "$d" (and can't).

This patch prevents the modpost symbol lookup code from ever returning those symbols, so it will return a whitelisted symbol instead.

Now to revert various code-bloating "fixes" that got merged because of this modpost bug....

Signed-off-by David Brownell <[email protected]>

---
Likely this patch can be improved on, but there's another issue. It seems to me that these modpost checks are wrong:

* Lingering pointers that point into sections modprobe removes are *always* unsafe ... including probe() methods marked "__init" on hotpluggable busses. Trivial fix: use __devinit instead; or maybe platform_driver_probe().
* Lingering pointers that point into sections that aren't removed are *never* unsafe ... including this remove() method case, since module unloading is configured and the __exit stuff must stay.

Whitelisting the former means not reporting potential oopsing cases; dangerous. Whereas even *checking* the latter is a waste of effort.

scripts/mod/modpost.c +24 -8

Index: at91/scripts/mod/modpost.c
===================================================================
@@ -679,6 +679,26 @@ static Elf_Sym *find_elf_symbol(struct e
 }
 
 /*
+ * If there's no name there, ignore it; likewise, ignore it if it's
+ * one of the magic symbols emitted used by current ARM tools.
+ *
+ * Otherwise if find_symbols_between() returns those symbols, they'll
+ * fail the whitelist tests and cause lots of false alarms ... fixable
+ * only by shrinking __exit and __init sections into __text, bloating
+ * the kernel (which is especially evil on embedded platforms).
+ */
+static int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
+{
+	const char *name = elf->strtab + sym->st_name;
+
+	if (!name || !strlen(name))
+		return 0;
+	if (strcmp(name, "$a") == 0 || strcmp(name, "$d") == 0)
+		return 0;
+	return 1;
+}
+
+/*
  * Find symbols before or equal addr and after addr - in the section sec.
  * If we find two symbols with equal offset prefer one with a valid name.
  * The ELF format may have a better way to detect what type of symbol

@@ -706,16 +726,15 @@ static void find_symbols_between(struct 
 		symsec = secstrings + elf->sechdrs[sym->st_shndx].sh_name;
 		if (strcmp(symsec, sec) != 0)
 			continue;
+		if (!is_valid_name(elf, sym))
+			continue;
 		if (sym->st_value <= addr) {
 			if ((addr - sym->st_value) < beforediff) {
 				beforediff = addr - sym->st_value;
 				*before = sym;
 			}
 			else if ((addr - sym->st_value) == beforediff) {
-				/* equal offset, valid name? */
-				const char *name = elf->strtab + sym->st_name;
-				if (name && strlen(name))
-					*before = sym;
+				*before = sym;
 			}
 		}
 		else

@@ -725,10 +744,7 @@ static void find_symbols_between(struct 
 				*after = sym;
 			}
 			else if ((sym->st_value - addr) == afterdiff) {
-				/* equal offset, valid name? */
-				const char *name = elf->strtab + sym->st_name;
-				if (name && strlen(name))
-					*after = sym;
+				*after = sym;
 			}
 		}
 	}

Russell King

2007-01-12 16:39 UTC #1 lore raw

Quoted 14 lines · David Brownell

Index: at91/scripts/mod/modpost.c
===================================================================

scripts/mod/modpost.c +6 -0

@@ -679,6 +679,26 @@ static Elf_Sym *find_elf_symbol(struct e
 }
 
 /*
+ * If there's no name there, ignore it; likewise, ignore it if it's
+ * one of the magic symbols emitted used by current ARM tools.
+ *
+ * Otherwise if find_symbols_between() returns those symbols, they'll
+ * fail the whitelist tests and cause lots of false alarms ... fixable
+ * only by shrinking __exit and __init sections into __text, bloating

scripts/mod/modpost.c static Elf_Sym *find_elf_symbol(struct e · line 697

Quoted 10 lines

scripts/mod/modpost.c +10 -0

+ * the kernel (which is especially evil on embedded platforms).
+ */
+static int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
+{
+	const char *name = elf->strtab + sym->st_name;
+
+	if (!name || !strlen(name))
+		return 0;
+	if (strcmp(name, "$a") == 0 || strcmp(name, "$d") == 0)
+		return 0;

A more correct test would be that found in kallsyms.c:

/*
* This ignores the intensely annoying "mapping symbols" found
* in ARM ELF files: $a, $t and $d.
*/
static inline int is_arm_mapping_symbol(const char *str)

{
        return str[0] == '$' && strchr("atd", str[1])
               && (str[2] == '\0' || str[2] == '.');
}

Suggest that code is re-used here (as well as in other tools such as oprofile, readprofile, etc.)

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

David Brownell

author

2007-01-12 20:13 UTC #2 lore raw

Quoted 1 line · Russell King

A more correct test would be that found in kallsyms.c:

Good point. Updated patch appended.

- Dave

=============== CUT HERE
This patch stops "modpost" from issuing erroneous modpost warnings on ARM builds, which it's been doing simce since maybe last summer. A canonical example would be driver method table entries:

WARNING: <path> - Section mismatch: reference to .exit.text:<name>_remove

	from .data after '$d' (at offset 0x4)

That "$d" symbol is generated by tools conformant with ARM ABI specs; in this case, it's a relocation in the middle of a "<name>_driver" struct. The erroneous warnings appear to be issued because "modpost" whitelists references from "<name>_driver" data into init and exit sections ... but does NOT whitelist them from "$d" (and can't).

This patch prevents the modpost symbol lookup code from ever returning those symbols, so it will return a whitelisted symbol instead.

Now to revert various code-bloating "fixes" that got merged because of this modpost bug....

Signed-off-by David Brownell <[email protected]>

---
Likely this patch can be improved on, but there's another issue. It seems to me that these modpost checks are wrong:

Whitelisting the former means not reporting potential oopsing cases; dangerous. Whereas even *checking* the latter is a waste of effort.

scripts/mod/modpost.c +28 -8

Index: at91/scripts/mod/modpost.c
===================================================================
@@ -655,6 +655,30 @@ static Elf_Sym *find_elf_symbol(struct e
 	return NULL;
 }
 
+static inline int is_arm_mapping_symbol(const char *str)
+{
+	return str[0] == '$' && strchr("atd", str[1])
+	       && (str[2] == '\0' || str[2] == '.');
+}
+
+/*
+ * If there's no name there, ignore it; likewise, ignore it if it's
+ * one of the magic symbols emitted used by current ARM tools.
+ *
+ * Otherwise if find_symbols_between() returns those symbols, they'll
+ * fail the whitelist tests and cause lots of false alarms ... fixable
+ * only by shrinking __exit and __init sections into __text, bloating
+ * the kernel (which is especially evil on embedded platforms).
+ */
+static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
+{
+	const char *name = elf->strtab + sym->st_name;
+
+	if (!name || !strlen(name))
+		return 0;
+	return !is_arm_mapping_symbol(name);
+}
+
 /*
  * Find symbols before or equal addr and after addr - in the section sec.
  * If we find two symbols with equal offset prefer one with a valid name.

@@ -683,16 +707,15 @@ static void find_symbols_between(struct 
 		symsec = secstrings + elf->sechdrs[sym->st_shndx].sh_name;
 		if (strcmp(symsec, sec) != 0)
 			continue;
+		if (!is_valid_name(elf, sym))
+			continue;
 		if (sym->st_value <= addr) {
 			if ((addr - sym->st_value) < beforediff) {
 				beforediff = addr - sym->st_value;
 				*before = sym;
 			}
 			else if ((addr - sym->st_value) == beforediff) {
-				/* equal offset, valid name? */
-				const char *name = elf->strtab + sym->st_name;
-				if (name && strlen(name))
-					*before = sym;
+				*before = sym;
 			}
 		}
 		else

@@ -702,10 +725,7 @@ static void find_symbols_between(struct 
 				*after = sym;
 			}
 			else if ((sym->st_value - addr) == afterdiff) {
-				/* equal offset, valid name? */
-				const char *name = elf->strtab + sym->st_name;
-				if (name && strlen(name))
-					*after = sym;
+				*after = sym;
 			}
 		}
 	}

David Brownell

author

2007-02-16 03:10 UTC #3 lore raw

PATCH 1/1 remove modpost false warnings on ARM

In reply to David Brownell

This patch stops "modpost" from issuing erroneous modpost warnings on ARM builds, which it's been doing since since maybe last summer. A canonical example would be driver method table entries:

WARNING: <path> - Section mismatch: reference to .exit.text:<name>_remove

	from .data after '$d' (at offset 0x4)

That "$d" symbol is generated by tools conformant with ARM ABI specs; in this case it's a symbol **in the middle of** a "<name>_driver" struct.

The erroneous warnings appear to be issued because "modpost" whitelists references from "<name>_driver" data into init and exit sections ... but doesn't know should also include those "$d" mapping symbols, which are not otherwise associated with "<name>_driver" symbols.

This patch prevents the modpost symbol lookup code from ever returning those mapping symbols, so it will return a whitelisted symbol instead. Then things work as expected.

Now to revert various code-bloating "fixes" that got merged because of this modpost bug....

Signed-off-by David Brownell <[email protected]>

---
NOTE: this is a **RESEND** of a patch against 2.6.20-rc4 ... it's past time to merge a fix, please!! I don't see an entry for "modpost" in the MAINTAINERS file, so I'm trying to CC anyone who "git" says has been involved recently...

However, it also seems to me that these modpost checks are wrong:

* Lingering pointers that point into sections that aren't removed are *never* unsafe ... including this remove() method case, since module unloading is configured and the __exit stuff must stay.

Whitelisting the former means not reporting potential oopsing cases; dangerous. Whereas even *checking* the latter is a waste of effort.

scripts/mod/modpost.c +28 -8

Index: at91/scripts/mod/modpost.c
===================================================================
@@ -686,6 +686,30 @@ static Elf_Sym *find_elf_symbol(struct e
 	return NULL;
 }
 
+static inline int is_arm_mapping_symbol(const char *str)
+{
+	return str[0] == '$' && strchr("atd", str[1])
+	       && (str[2] == '\0' || str[2] == '.');
+}
+
+/*
+ * If there's no name there, ignore it; likewise, ignore it if it's
+ * one of the magic symbols emitted used by current ARM tools.
+ *
+ * Otherwise if find_symbols_between() returns those symbols, they'll
+ * fail the whitelist tests and cause lots of false alarms ... fixable
+ * only by merging __exit and __init sections into __text, bloating
+ * the kernel (which is especially evil on embedded platforms).
+ */
+static inline int is_valid_name(struct elf_info *elf, Elf_Sym *sym)
+{
+	const char *name = elf->strtab + sym->st_name;
+
+	if (!name || !strlen(name))
+		return 0;
+	return !is_arm_mapping_symbol(name);
+}
+
 /*
  * Find symbols before or equal addr and after addr - in the section sec.
  * If we find two symbols with equal offset prefer one with a valid name.

@@ -714,16 +738,15 @@ static void find_symbols_between(struct 
 		symsec = secstrings + elf->sechdrs[sym->st_shndx].sh_name;
 		if (strcmp(symsec, sec) != 0)
 			continue;
+		if (!is_valid_name(elf, sym))
+			continue;
 		if (sym->st_value <= addr) {
 			if ((addr - sym->st_value) < beforediff) {
 				beforediff = addr - sym->st_value;
 				*before = sym;
 			}
 			else if ((addr - sym->st_value) == beforediff) {
-				/* equal offset, valid name? */
-				const char *name = elf->strtab + sym->st_name;
-				if (name && strlen(name))
-					*before = sym;
+				*before = sym;
 			}
 		}
 		else

@@ -733,10 +756,7 @@ static void find_symbols_between(struct 
 				*after = sym;
 			}
 			else if ((sym->st_value - addr) == afterdiff) {
-				/* equal offset, valid name? */
-				const char *name = elf->strtab + sym->st_name;
-				if (name && strlen(name))
-					*after = sym;
+				*after = sym;
 			}
 		}
 	}

Rusty Russell

2007-02-16 03:28 UTC #4 lore raw

Quoted 6 lines · David Brownell

This patch stops "modpost" from issuing erroneous modpost warnings on ARM builds, which it's been doing since since maybe last summer. A canonical example would be driver method table entries:
WARNING: <path> - Section mismatch: reference to .exit.text:<name>_remove
	from .data after '$d' (at offset 0x4)

Looks fine to me.

Rusty.

Sam Ravnborg

Acked-by on v1 1/1

2007-02-16 08:26 UTC #5 lore raw

In reply to David Brownell

Quoted 15 lines · David Brownell

This patch stops "modpost" from issuing erroneous modpost warnings on ARM builds, which it's been doing since since maybe last summer. A canonical example would be driver method table entries:
WARNING: <path> - Section mismatch: reference to .exit.text:<name>_remove
	from .data after '$d' (at offset 0x4)
That "$d" symbol is generated by tools conformant with ARM ABI specs; in this case it's a symbol **in the middle of** a "<name>_driver" struct.
The erroneous warnings appear to be issued because "modpost" whitelists references from "<name>_driver" data into init and exit sections ... but doesn't know should also include those "$d" mapping symbols, which are not otherwise associated with "<name>_driver" symbols.

Quoted 8 lines

This patch prevents the modpost symbol lookup code from ever returning those mapping symbols, so it will return a whitelisted symbol instead. Then things work as expected.
Now to revert various code-bloating "fixes" that got merged because of this modpost bug....
Signed-off-by David Brownell <[email protected]>

Acked-by Sam Ravnborg <[email protected]>

And if I get my dev machine operational before akpm merges this I will take care of having it merged.