[PATCH] scripts/mod/modpost.c: Fix bug in number_prefix

[PATCH] scripts/mod/modpost.c: Fix bug in number_prefix

[Rasmus Villemoes]

The function number_prefix() can currently only return 1 if its
argument is the empty string: If line 3 is reached and *sym (now the
second character in the argument) is not '.', 0 is returned. However,
if that character is '.', the first assignment to c is that same '.',
which obviously fails to be a digit.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 scripts/mod/modpost.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 9d9c5b9..336f45f 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -778,9 +778,9 @@ static const char *sech_name(struct elf_info *elf, Elf_Shdr *sechdr)
  */
 static int number_prefix(const char *sym)
 {
-	if (*sym++ == '\0')
+	if (*sym == '\0')
 		return 1;
-	if (*sym != '.')
+	if (*sym++ != '.')
 		return 0;
 	do {
 		char c = *sym++;
-- 
1.9.2

Re: [PATCH] scripts/mod/modpost.c: Fix bug in number_prefix

[Paul Gortmaker]

On 14-06-25 04:46 AM, Rasmus Villemoes wrote:
> The function number_prefix() can currently only return 1 if its
> argument is the empty string: If line 3 is reached and *sym (now the
> second character in the argument) is not '.', 0 is returned. However,
> if that character is '.', the first assignment to c is that same '.',
> which obviously fails to be a digit.

I'd suggest you expand the commit log to actually list the end-user
visible symptom and the use case that this actually fixes, since it
isn't obvious to me at all.

Thanks,
Paul.
--

> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  scripts/mod/modpost.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 9d9c5b9..336f45f 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -778,9 +778,9 @@ static const char *sech_name(struct elf_info *elf, Elf_Shdr *sechdr)
>   */
>  static int number_prefix(const char *sym)
>  {
> -	if (*sym++ == '\0')
> +	if (*sym == '\0')
>  		return 1;
> -	if (*sym != '.')
> +	if (*sym++ != '.')
>  		return 0;
>  	do {
>  		char c = *sym++;
> 

Re: [PATCH] scripts/mod/modpost.c: Fix bug in number_prefix

[Rasmus Villemoes]

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

> On 14-06-25 04:46 AM, Rasmus Villemoes wrote:
>> The function number_prefix() can currently only return 1 if its
>> argument is the empty string: If line 3 is reached and *sym (now the
>> second character in the argument) is not '.', 0 is returned. However,
>> if that character is '.', the first assignment to c is that same '.',
>> which obviously fails to be a digit.
>
> I'd suggest you expand the commit log to actually list the end-user
> visible symptom and the use case that this actually fixes, since it
> isn't obvious to me at all.

Sorry, it isn't obvious to me either. I just stumbled on it reading the
code; the rest of modpost.c is too deep magic for me.

Maybe there are no user-visible symptoms, and maybe that means it is not
worth fixing, in which case I'll just add scripts/mod/modpost.c to my
mental .ocpdignore.

Thanks,
Rasmus

Re: [PATCH] scripts/mod/modpost.c: Fix bug in number_prefix

[Rusty Russell]

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
> Paul Gortmaker <paul.gortmaker@windriver.com> writes:
>
>> On 14-06-25 04:46 AM, Rasmus Villemoes wrote:
>>> The function number_prefix() can currently only return 1 if its
>>> argument is the empty string: If line 3 is reached and *sym (now the
>>> second character in the argument) is not '.', 0 is returned. However,
>>> if that character is '.', the first assignment to c is that same '.',
>>> which obviously fails to be a digit.
>>
>> I'd suggest you expand the commit log to actually list the end-user
>> visible symptom and the use case that this actually fixes, since it
>> isn't obvious to me at all.
>
> Sorry, it isn't obvious to me either. I just stumbled on it reading the
> code; the rest of modpost.c is too deep magic for me.

The function is horribly mis-named which doesn't help ("number_postfix"
would be closer).

And yes, it's completely broken.

Sam, this never worked, and was clearly never tested.  Seems like we
should just rip out the '$' postfix then, or are you aware of platforms
which have been ignoring modpost complaints since 2008?

Cheers,
Rusty.

Re: [PATCH] scripts/mod/modpost.c: Fix bug in number_prefix

[Sam Ravnborg]

On Wed, Jul 09, 2014 at 09:28:43AM +0930, Rusty Russell wrote:
> Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
> > Paul Gortmaker <paul.gortmaker@windriver.com> writes:
> >
> >> On 14-06-25 04:46 AM, Rasmus Villemoes wrote:
> >>> The function number_prefix() can currently only return 1 if its
> >>> argument is the empty string: If line 3 is reached and *sym (now the
> >>> second character in the argument) is not '.', 0 is returned. However,
> >>> if that character is '.', the first assignment to c is that same '.',
> >>> which obviously fails to be a digit.
> >>
> >> I'd suggest you expand the commit log to actually list the end-user
> >> visible symptom and the use case that this actually fixes, since it
> >> isn't obvious to me at all.
> >
> > Sorry, it isn't obvious to me either. I just stumbled on it reading the
> > code; the rest of modpost.c is too deep magic for me.
> 
> The function is horribly mis-named which doesn't help ("number_postfix"
> would be closer).
number_suffix() - I sometimes mix postfix and suffix.

> 
> And yes, it's completely broken.
> 
> Sam, this never worked, and was clearly never tested.  Seems like we
> should just rip out the '$' postfix then, or are you aware of platforms
> which have been ignoring modpost complaints since 2008?
The architectures that uses -ffunction_sections are sometimes
in a situation where they add a number. And maybe in other cases too.

I still see section mismatch warnings ignored but there are much less
of them these days.
And since this has not worked for years lets rip it out.
If we really need this feature in the future then we can add it back.

Rasmus - will you take care and send a patch that removes this?

Thanks,
	Sam

[PATCH] scripts: modpost: Remove numeric suffix pattern matching

[Rasmus Villemoes]

For several years, the pattern "foo$" has effectively been treated as
equivalent to "foo" due to a bug in the (misnamed) helper
number_prefix(). This hasn't been observed to cause any problems, so
remove the broken $ functionality and change all foo$ patterns to foo.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
Sam, was this what you meant? Otherwise, I'd be more than happy to
leave the issue to someone else.

 scripts/mod/modpost.c | 49 ++++++++++---------------------------------------
 1 file changed, 10 insertions(+), 39 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 9d9c5b9..8f5e122 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -772,32 +772,10 @@ static const char *sech_name(struct elf_info *elf, Elf_Shdr *sechdr)
 		sechdr->sh_name;
 }
 
-/* if sym is empty or point to a string
- * like ".[0-9]+" then return 1.
- * This is the optional prefix added by ld to some sections
- */
-static int number_prefix(const char *sym)
-{
-	if (*sym++ == '\0')
-		return 1;
-	if (*sym != '.')
-		return 0;
-	do {
-		char c = *sym++;
-		if (c < '0' || c > '9')
-			return 0;
-	} while (*sym);
-	return 1;
-}
-
 /* The pattern is an array of simple patterns.
  * "foo" will match an exact string equal to "foo"
  * "*foo" will match a string that ends with "foo"
  * "foo*" will match a string that begins with "foo"
- * "foo$" will match a string equal to "foo" or "foo.1"
- *   where the '1' can be any number including several digits.
- *   The $ syntax is for sections where ld append a dot number
- *   to make section name unique.
  */
 static int match(const char *sym, const char * const pat[])
 {
@@ -816,13 +794,6 @@ static int match(const char *sym, const char * const pat[])
 			if (strncmp(sym, p, strlen(p) - 1) == 0)
 				return 1;
 		}
-		/* "foo$" */
-		else if (*endp == '$') {
-			if (strncmp(sym, p, strlen(p) - 1) == 0) {
-				if (number_prefix(sym + strlen(p) - 1))
-					return 1;
-			}
-		}
 		/* no wildcards */
 		else {
 			if (strcmp(p, sym) == 0)
@@ -880,20 +851,20 @@ static void check_section(const char *modname, struct elf_info *elf,
 
 
 #define ALL_INIT_DATA_SECTIONS \
-	".init.setup$", ".init.rodata$", ".meminit.rodata$", \
-	".init.data$", ".meminit.data$"
+	".init.setup", ".init.rodata", ".meminit.rodata", \
+	".init.data", ".meminit.data"
 #define ALL_EXIT_DATA_SECTIONS \
-	".exit.data$", ".memexit.data$"
+	".exit.data", ".memexit.data"
 
 #define ALL_INIT_TEXT_SECTIONS \
-	".init.text$", ".meminit.text$"
+	".init.text", ".meminit.text"
 #define ALL_EXIT_TEXT_SECTIONS \
-	".exit.text$", ".memexit.text$"
+	".exit.text", ".memexit.text"
 
 #define ALL_PCI_INIT_SECTIONS	\
-	".pci_fixup_early$", ".pci_fixup_header$", ".pci_fixup_final$", \
-	".pci_fixup_enable$", ".pci_fixup_resume$", \
-	".pci_fixup_resume_early$", ".pci_fixup_suspend$"
+	".pci_fixup_early", ".pci_fixup_header", ".pci_fixup_final", \
+	".pci_fixup_enable", ".pci_fixup_resume", \
+	".pci_fixup_resume_early", ".pci_fixup_suspend"
 
 #define ALL_XXXINIT_SECTIONS MEM_INIT_SECTIONS
 #define ALL_XXXEXIT_SECTIONS MEM_EXIT_SECTIONS
@@ -901,8 +872,8 @@ static void check_section(const char *modname, struct elf_info *elf,
 #define ALL_INIT_SECTIONS INIT_SECTIONS, ALL_XXXINIT_SECTIONS
 #define ALL_EXIT_SECTIONS EXIT_SECTIONS, ALL_XXXEXIT_SECTIONS
 
-#define DATA_SECTIONS ".data$", ".data.rel$"
-#define TEXT_SECTIONS ".text$", ".text.unlikely$"
+#define DATA_SECTIONS ".data", ".data.rel"
+#define TEXT_SECTIONS ".text", ".text.unlikely"
 
 #define INIT_SECTIONS      ".init.*"
 #define MEM_INIT_SECTIONS  ".meminit.*"
-- 
1.9.2

Re: [PATCH] scripts: modpost: Remove numeric suffix pattern matching

[Sam Ravnborg]

On Wed, Jul 09, 2014 at 11:41:48AM +0200, Rasmus Villemoes wrote:
> For several years, the pattern "foo$" has effectively been treated as
> equivalent to "foo" due to a bug in the (misnamed) helper
> number_prefix(). This hasn't been observed to cause any problems, so
> remove the broken $ functionality and change all foo$ patterns to foo.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> Sam, was this what you meant? Otherwise, I'd be more than happy to
> leave the issue to someone else.
Exactly - thanks.
Acked-by: Sam Ravnborg <sam@ravnborg.org>

Rusty - I assume you pick this up and channel it through the module tree.

	Sam

Re: [PATCH] scripts: modpost: Remove numeric suffix pattern matching

[Rusty Russell]

Sam Ravnborg <sam@ravnborg.org> writes:
> On Wed, Jul 09, 2014 at 11:41:48AM +0200, Rasmus Villemoes wrote:
>> For several years, the pattern "foo$" has effectively been treated as
>> equivalent to "foo" due to a bug in the (misnamed) helper
>> number_prefix(). This hasn't been observed to cause any problems, so
>> remove the broken $ functionality and change all foo$ patterns to foo.
>> 
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>> Sam, was this what you meant? Otherwise, I'd be more than happy to
>> leave the issue to someone else.
> Exactly - thanks.
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
>
> Rusty - I assume you pick this up and channel it through the module tree.
>
> 	Sam

Done, thanks!

Applied,
Rusty.