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.
Index: at91/scripts/mod/modpost.c
===================================================================
--- at91.orig/scripts/mod/modpost.c 2007-01-11 22:51:49.000000000 -0800
+++ at91/scripts/mod/modpost.c 2007-01-12 04:20:00.000000000 -0800
@@ -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;
}
}
}
On Fri, Jan 12, 2007 at 08:31:36AM -0800, David Brownell wrote:
> Index: at91/scripts/mod/modpost.c
> ===================================================================
> --- at91.orig/scripts/mod/modpost.c 2007-01-11 22:51:49.000000000 -0800
> +++ at91/scripts/mod/modpost.c 2007-01-12 04:20:00.000000000 -0800
> @@ -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;
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.)
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Friday 12 January 2007 8:38 am, Russell King wrote:
> 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:
? * 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.
Index: at91/scripts/mod/modpost.c
===================================================================
--- at91.orig/scripts/mod/modpost.c 2006-12-15 10:08:57.000000000 -0800
+++ at91/scripts/mod/modpost.c 2007-01-12 12:09:29.000000000 -0800
@@ -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;
}
}
}
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 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.
Index: at91/scripts/mod/modpost.c
===================================================================
--- at91.orig/scripts/mod/modpost.c 2007-02-15 18:20:15.000000000 -0800
+++ at91/scripts/mod/modpost.c 2007-02-15 18:22:50.000000000 -0800
@@ -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;
}
}
}
On Thu, 2007-02-15 at 19:10 -0800, David Brownell wrote:
> 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.
On Thu, Feb 15, 2007 at 07:10:45PM -0800, David Brownell wrote:
> 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]>
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.