LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [patch 2.6.20-rc4-git] remove modpost false warnings on ARM
@ 2007-01-12 16:31 David Brownell
  2007-01-12 16:38 ` Russell King
  2007-02-16  3:10 ` [patch 2.6.20-git] " David Brownell
  0 siblings, 2 replies; 6+ messages in thread
From: David Brownell @ 2007-01-12 16:31 UTC (permalink / raw)
  To: Linux Kernel list; +Cc: rusty, Russell King

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 <dbrownell@users.sourceforge.net>

---
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;
 			}
 		}
 	}

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch 2.6.20-rc4-git] remove modpost false warnings on ARM
  2007-01-12 16:31 [patch 2.6.20-rc4-git] remove modpost false warnings on ARM David Brownell
@ 2007-01-12 16:38 ` Russell King
  2007-01-12 20:13   ` David Brownell
  2007-02-16  3:10 ` [patch 2.6.20-git] " David Brownell
  1 sibling, 1 reply; 6+ messages in thread
From: Russell King @ 2007-01-12 16:38 UTC (permalink / raw)
  To: David Brownell; +Cc: Linux Kernel list, rusty

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:

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch 2.6.20-rc4-git] remove modpost false warnings on ARM
  2007-01-12 16:38 ` Russell King
@ 2007-01-12 20:13   ` David Brownell
  0 siblings, 0 replies; 6+ messages in thread
From: David Brownell @ 2007-01-12 20:13 UTC (permalink / raw)
  To: Russell King; +Cc: Linux Kernel list, rusty

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 <dbrownell@users.sourceforge.net>
---
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;
 			}
 		}
 	}

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [patch 2.6.20-git] remove modpost false warnings on ARM
  2007-01-12 16:31 [patch 2.6.20-rc4-git] remove modpost false warnings on ARM David Brownell
  2007-01-12 16:38 ` Russell King
@ 2007-02-16  3:10 ` David Brownell
  2007-02-16  3:28   ` Rusty Russell
  2007-02-16  8:26   ` Sam Ravnborg
  1 sibling, 2 replies; 6+ messages in thread
From: David Brownell @ 2007-02-16  3:10 UTC (permalink / raw)
  To: Linux Kernel list
  Cc: Andi Kleen, Andrew Morton, ebiederm, kai, Russell King, sam, rusty

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 <dbrownell@users.sourceforge.net>
---
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;
 			}
 		}
 	}

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch 2.6.20-git] remove modpost false warnings on ARM
  2007-02-16  3:10 ` [patch 2.6.20-git] " David Brownell
@ 2007-02-16  3:28   ` Rusty Russell
  2007-02-16  8:26   ` Sam Ravnborg
  1 sibling, 0 replies; 6+ messages in thread
From: Rusty Russell @ 2007-02-16  3:28 UTC (permalink / raw)
  To: David Brownell
  Cc: Linux Kernel list, Andi Kleen, Andrew Morton, ebiederm, kai,
	Russell King, sam

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.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch 2.6.20-git] remove modpost false warnings on ARM
  2007-02-16  3:10 ` [patch 2.6.20-git] " David Brownell
  2007-02-16  3:28   ` Rusty Russell
@ 2007-02-16  8:26   ` Sam Ravnborg
  1 sibling, 0 replies; 6+ messages in thread
From: Sam Ravnborg @ 2007-02-16  8:26 UTC (permalink / raw)
  To: David Brownell
  Cc: Linux Kernel list, Andi Kleen, Andrew Morton, ebiederm, kai,
	Russell King, 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 <dbrownell@users.sourceforge.net>
Acked-by: Sam Ravnborg <sam@ravnborg.org>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2007-02-16  8:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-12 16:31 [patch 2.6.20-rc4-git] remove modpost false warnings on ARM David Brownell
2007-01-12 16:38 ` Russell King
2007-01-12 20:13   ` David Brownell
2007-02-16  3:10 ` [patch 2.6.20-git] " David Brownell
2007-02-16  3:28   ` Rusty Russell
2007-02-16  8:26   ` Sam Ravnborg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).