LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] modpost: move the namespace field in Module.symvers last
@ 2020-03-11 17:01 Jessica Yu
  2020-03-12  7:17 ` Lucas De Marchi
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Jessica Yu @ 2020-03-11 17:01 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Matthias Maennich, Lucas De Marchi, stable, linux-kernel, Jessica Yu

In order to preserve backwards compatability with kmod tools, we have to
move the namespace field in Module.symvers last, as the depmod -e -E
option looks at the first three fields in Module.symvers to check symbol
versions (and it's expected they stay in the original order of crc,
symbol, module).

In addition, update an ancient comment above read_dump() in modpost that
suggested that the export type field in Module.symvers was optional. I
suspect that there were historical reasons behind that comment that are
no longer accurate. We have been unconditionally printing the export
type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.

Fix up read_dump() to treat each field as non-optional. I suspect the
original read_dump() code treated the export field as optional in order
to support pre <= 2.6.18 Module.symvers (which did not have the export
type field). Note that although symbol namespaces are optional, the
field will not be omitted from Module.symvers if a symbol does not have
a namespace. In this case, the field will simply be empty and the next
delimiter or end of line will follow.

Cc: stable@vger.kernel.org
Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
Tested-by: Matthias Maennich <maennich@google.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Jessica Yu <jeyu@kernel.org>
---
v2:

  - Explain the changes to read_dump() and the comment (and provide
    historical context) in the commit message. (Lucas De Marchi)

 Documentation/kbuild/modules.rst |  4 ++--
 scripts/export_report.pl         |  2 +-
 scripts/mod/modpost.c            | 24 ++++++++++++------------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
index 69fa48ee93d6..e0b45a257f21 100644
--- a/Documentation/kbuild/modules.rst
+++ b/Documentation/kbuild/modules.rst
@@ -470,9 +470,9 @@ build.
 
 	The syntax of the Module.symvers file is::
 
-	<CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
+	<CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
 
-	0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
+	0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
 
 	The fields are separated by tabs and values may be empty (e.g.
 	if no namespace is defined for an exported symbol).
diff --git a/scripts/export_report.pl b/scripts/export_report.pl
index 548330e8c4e7..feb3d5542a62 100755
--- a/scripts/export_report.pl
+++ b/scripts/export_report.pl
@@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
 #
 while ( <$module_symvers> ) {
 	chomp;
-	my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
+	my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
 	$SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
 }
 close($module_symvers);
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index a3d8370f9544..e1963ef8c07c 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2421,7 +2421,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
 }
 
 /* parse Module.symvers file. line format:
- * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
+ * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace
  **/
 static void read_dump(const char *fname, unsigned int kernel)
 {
@@ -2434,7 +2434,7 @@ static void read_dump(const char *fname, unsigned int kernel)
 		return;
 
 	while ((line = get_next_line(&pos, file, size))) {
-		char *symname, *namespace, *modname, *d, *export, *end;
+		char *symname, *namespace, *modname, *d, *export;
 		unsigned int crc;
 		struct module *mod;
 		struct symbol *s;
@@ -2442,16 +2442,16 @@ static void read_dump(const char *fname, unsigned int kernel)
 		if (!(symname = strchr(line, '\t')))
 			goto fail;
 		*symname++ = '\0';
-		if (!(namespace = strchr(symname, '\t')))
-			goto fail;
-		*namespace++ = '\0';
-		if (!(modname = strchr(namespace, '\t')))
+		if (!(modname = strchr(symname, '\t')))
 			goto fail;
 		*modname++ = '\0';
-		if ((export = strchr(modname, '\t')) != NULL)
-			*export++ = '\0';
-		if (export && ((end = strchr(export, '\t')) != NULL))
-			*end = '\0';
+		if (!(export = strchr(modname, '\t')))
+			goto fail;
+		*export++ = '\0';
+		if (!(namespace = strchr(export, '\t')))
+			goto fail;
+		*namespace++ = '\0';
+
 		crc = strtoul(line, &d, 16);
 		if (*symname == '\0' || *modname == '\0' || *d != '\0')
 			goto fail;
@@ -2502,9 +2502,9 @@ static void write_dump(const char *fname)
 				namespace = symbol->namespace;
 				buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
 					   symbol->crc, symbol->name,
-					   namespace ? namespace : "",
 					   symbol->module->name,
-					   export_str(symbol->export));
+					   export_str(symbol->export),
+					   namespace ? namespace : "");
 			}
 			symbol = symbol->next;
 		}
-- 
2.16.4


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

* Re: [PATCH v2] modpost: move the namespace field in Module.symvers last
  2020-03-11 17:01 [PATCH v2] modpost: move the namespace field in Module.symvers last Jessica Yu
@ 2020-03-12  7:17 ` Lucas De Marchi
  2020-03-14  2:11 ` Masahiro Yamada
  2020-03-31  5:49 ` John Stultz
  2 siblings, 0 replies; 10+ messages in thread
From: Lucas De Marchi @ 2020-03-12  7:17 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Masahiro Yamada, Matthias Maennich, stable, lkml

On Wed, Mar 11, 2020 at 10:02 AM Jessica Yu <jeyu@kernel.org> wrote:
>
> In order to preserve backwards compatability with kmod tools, we have to
> move the namespace field in Module.symvers last, as the depmod -e -E
> option looks at the first three fields in Module.symvers to check symbol
> versions (and it's expected they stay in the original order of crc,
> symbol, module).
>
> In addition, update an ancient comment above read_dump() in modpost that
> suggested that the export type field in Module.symvers was optional. I
> suspect that there were historical reasons behind that comment that are
> no longer accurate. We have been unconditionally printing the export
> type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.
>
> Fix up read_dump() to treat each field as non-optional. I suspect the
> original read_dump() code treated the export field as optional in order
> to support pre <= 2.6.18 Module.symvers (which did not have the export
> type field). Note that although symbol namespaces are optional, the
> field will not be omitted from Module.symvers if a symbol does not have
> a namespace. In this case, the field will simply be empty and the next
> delimiter or end of line will follow.
>
> Cc: stable@vger.kernel.org
> Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
> Tested-by: Matthias Maennich <maennich@google.com>
> Reviewed-by: Matthias Maennich <maennich@google.com>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Jessica Yu <jeyu@kernel.org>
> ---
> v2:
>
>   - Explain the changes to read_dump() and the comment (and provide
>     historical context) in the commit message. (Lucas De Marchi)

Great, thanks for fixing this.

Lucas De Marchi

>
>  Documentation/kbuild/modules.rst |  4 ++--
>  scripts/export_report.pl         |  2 +-
>  scripts/mod/modpost.c            | 24 ++++++++++++------------
>  3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
> index 69fa48ee93d6..e0b45a257f21 100644
> --- a/Documentation/kbuild/modules.rst
> +++ b/Documentation/kbuild/modules.rst
> @@ -470,9 +470,9 @@ build.
>
>         The syntax of the Module.symvers file is::
>
> -       <CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
> +       <CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>
> -       0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
> +       0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>
>         The fields are separated by tabs and values may be empty (e.g.
>         if no namespace is defined for an exported symbol).
> diff --git a/scripts/export_report.pl b/scripts/export_report.pl
> index 548330e8c4e7..feb3d5542a62 100755
> --- a/scripts/export_report.pl
> +++ b/scripts/export_report.pl
> @@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
>  #
>  while ( <$module_symvers> ) {
>         chomp;
> -       my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
> +       my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
>         $SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
>  }
>  close($module_symvers);
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index a3d8370f9544..e1963ef8c07c 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2421,7 +2421,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
>  }
>
>  /* parse Module.symvers file. line format:
> - * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
> + * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace
>   **/
>  static void read_dump(const char *fname, unsigned int kernel)
>  {
> @@ -2434,7 +2434,7 @@ static void read_dump(const char *fname, unsigned int kernel)
>                 return;
>
>         while ((line = get_next_line(&pos, file, size))) {
> -               char *symname, *namespace, *modname, *d, *export, *end;
> +               char *symname, *namespace, *modname, *d, *export;
>                 unsigned int crc;
>                 struct module *mod;
>                 struct symbol *s;
> @@ -2442,16 +2442,16 @@ static void read_dump(const char *fname, unsigned int kernel)
>                 if (!(symname = strchr(line, '\t')))
>                         goto fail;
>                 *symname++ = '\0';
> -               if (!(namespace = strchr(symname, '\t')))
> -                       goto fail;
> -               *namespace++ = '\0';
> -               if (!(modname = strchr(namespace, '\t')))
> +               if (!(modname = strchr(symname, '\t')))
>                         goto fail;
>                 *modname++ = '\0';
> -               if ((export = strchr(modname, '\t')) != NULL)
> -                       *export++ = '\0';
> -               if (export && ((end = strchr(export, '\t')) != NULL))
> -                       *end = '\0';
> +               if (!(export = strchr(modname, '\t')))
> +                       goto fail;
> +               *export++ = '\0';
> +               if (!(namespace = strchr(export, '\t')))
> +                       goto fail;
> +               *namespace++ = '\0';
> +
>                 crc = strtoul(line, &d, 16);
>                 if (*symname == '\0' || *modname == '\0' || *d != '\0')
>                         goto fail;
> @@ -2502,9 +2502,9 @@ static void write_dump(const char *fname)
>                                 namespace = symbol->namespace;
>                                 buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
>                                            symbol->crc, symbol->name,
> -                                          namespace ? namespace : "",
>                                            symbol->module->name,
> -                                          export_str(symbol->export));
> +                                          export_str(symbol->export),
> +                                          namespace ? namespace : "");
>                         }
>                         symbol = symbol->next;
>                 }
> --
> 2.16.4
>


-- 
Lucas De Marchi

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

* Re: [PATCH v2] modpost: move the namespace field in Module.symvers last
  2020-03-11 17:01 [PATCH v2] modpost: move the namespace field in Module.symvers last Jessica Yu
  2020-03-12  7:17 ` Lucas De Marchi
@ 2020-03-14  2:11 ` Masahiro Yamada
  2020-03-17  0:24   ` Masahiro Yamada
  2020-03-17 11:24   ` Jessica Yu
  2020-03-31  5:49 ` John Stultz
  2 siblings, 2 replies; 10+ messages in thread
From: Masahiro Yamada @ 2020-03-14  2:11 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Matthias Maennich, Lucas De Marchi, stable, Linux Kernel Mailing List

On Thu, Mar 12, 2020 at 2:02 AM Jessica Yu <jeyu@kernel.org> wrote:
>
> In order to preserve backwards compatability with kmod tools, we have to
> move the namespace field in Module.symvers last, as the depmod -e -E
> option looks at the first three fields in Module.symvers to check symbol
> versions (and it's expected they stay in the original order of crc,
> symbol, module).
>
> In addition, update an ancient comment above read_dump() in modpost that
> suggested that the export type field in Module.symvers was optional. I
> suspect that there were historical reasons behind that comment that are
> no longer accurate. We have been unconditionally printing the export
> type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.
>
> Fix up read_dump() to treat each field as non-optional. I suspect the
> original read_dump() code treated the export field as optional in order
> to support pre <= 2.6.18 Module.symvers (which did not have the export
> type field). Note that although symbol namespaces are optional, the
> field will not be omitted from Module.symvers if a symbol does not have
> a namespace. In this case, the field will simply be empty and the next
> delimiter or end of line will follow.
>
> Cc: stable@vger.kernel.org
> Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
> Tested-by: Matthias Maennich <maennich@google.com>
> Reviewed-by: Matthias Maennich <maennich@google.com>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Jessica Yu <jeyu@kernel.org>


While I am not opposed to this fix,
I did not even notice Module.symvers was official interface.

Kbuild invokes scripts/depmod.sh to finalize
the 'make modules_install', but I do not see the -E
option being used there.

I do not see Module.symvers installed in
/lib/modules/$(uname -r)/.





> ---
> v2:
>
>   - Explain the changes to read_dump() and the comment (and provide
>     historical context) in the commit message. (Lucas De Marchi)
>
>  Documentation/kbuild/modules.rst |  4 ++--
>  scripts/export_report.pl         |  2 +-
>  scripts/mod/modpost.c            | 24 ++++++++++++------------
>  3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
> index 69fa48ee93d6..e0b45a257f21 100644
> --- a/Documentation/kbuild/modules.rst
> +++ b/Documentation/kbuild/modules.rst
> @@ -470,9 +470,9 @@ build.
>
>         The syntax of the Module.symvers file is::
>
> -       <CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
> +       <CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>
> -       0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
> +       0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>
>         The fields are separated by tabs and values may be empty (e.g.
>         if no namespace is defined for an exported symbol).
> diff --git a/scripts/export_report.pl b/scripts/export_report.pl
> index 548330e8c4e7..feb3d5542a62 100755
> --- a/scripts/export_report.pl
> +++ b/scripts/export_report.pl
> @@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
>  #
>  while ( <$module_symvers> ) {
>         chomp;
> -       my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
> +       my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
>         $SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
>  }
>  close($module_symvers);
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index a3d8370f9544..e1963ef8c07c 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2421,7 +2421,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
>  }
>
>  /* parse Module.symvers file. line format:
> - * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
> + * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace
>   **/
>  static void read_dump(const char *fname, unsigned int kernel)
>  {
> @@ -2434,7 +2434,7 @@ static void read_dump(const char *fname, unsigned int kernel)
>                 return;
>
>         while ((line = get_next_line(&pos, file, size))) {
> -               char *symname, *namespace, *modname, *d, *export, *end;
> +               char *symname, *namespace, *modname, *d, *export;
>                 unsigned int crc;
>                 struct module *mod;
>                 struct symbol *s;
> @@ -2442,16 +2442,16 @@ static void read_dump(const char *fname, unsigned int kernel)
>                 if (!(symname = strchr(line, '\t')))
>                         goto fail;
>                 *symname++ = '\0';
> -               if (!(namespace = strchr(symname, '\t')))
> -                       goto fail;
> -               *namespace++ = '\0';
> -               if (!(modname = strchr(namespace, '\t')))
> +               if (!(modname = strchr(symname, '\t')))
>                         goto fail;
>                 *modname++ = '\0';
> -               if ((export = strchr(modname, '\t')) != NULL)
> -                       *export++ = '\0';
> -               if (export && ((end = strchr(export, '\t')) != NULL))
> -                       *end = '\0';
> +               if (!(export = strchr(modname, '\t')))
> +                       goto fail;
> +               *export++ = '\0';
> +               if (!(namespace = strchr(export, '\t')))
> +                       goto fail;
> +               *namespace++ = '\0';
> +
>                 crc = strtoul(line, &d, 16);
>                 if (*symname == '\0' || *modname == '\0' || *d != '\0')
>                         goto fail;
> @@ -2502,9 +2502,9 @@ static void write_dump(const char *fname)
>                                 namespace = symbol->namespace;
>                                 buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
>                                            symbol->crc, symbol->name,
> -                                          namespace ? namespace : "",
>                                            symbol->module->name,
> -                                          export_str(symbol->export));
> +                                          export_str(symbol->export),
> +                                          namespace ? namespace : "");
>                         }
>                         symbol = symbol->next;
>                 }
> --
> 2.16.4
>


-- 
Best Regards
Masahiro Yamada

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

* Re: [PATCH v2] modpost: move the namespace field in Module.symvers last
  2020-03-14  2:11 ` Masahiro Yamada
@ 2020-03-17  0:24   ` Masahiro Yamada
  2020-03-17 11:24   ` Jessica Yu
  1 sibling, 0 replies; 10+ messages in thread
From: Masahiro Yamada @ 2020-03-17  0:24 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Matthias Maennich, Lucas De Marchi, stable, Linux Kernel Mailing List

On Sat, Mar 14, 2020 at 11:11 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Thu, Mar 12, 2020 at 2:02 AM Jessica Yu <jeyu@kernel.org> wrote:
> >
> > In order to preserve backwards compatability with kmod tools, we have to
> > move the namespace field in Module.symvers last, as the depmod -e -E
> > option looks at the first three fields in Module.symvers to check symbol
> > versions (and it's expected they stay in the original order of crc,
> > symbol, module).
> >
> > In addition, update an ancient comment above read_dump() in modpost that
> > suggested that the export type field in Module.symvers was optional. I
> > suspect that there were historical reasons behind that comment that are
> > no longer accurate. We have been unconditionally printing the export
> > type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.
> >
> > Fix up read_dump() to treat each field as non-optional. I suspect the
> > original read_dump() code treated the export field as optional in order
> > to support pre <= 2.6.18 Module.symvers (which did not have the export
> > type field). Note that although symbol namespaces are optional, the
> > field will not be omitted from Module.symvers if a symbol does not have
> > a namespace. In this case, the field will simply be empty and the next
> > delimiter or end of line will follow.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
> > Tested-by: Matthias Maennich <maennich@google.com>
> > Reviewed-by: Matthias Maennich <maennich@google.com>
> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Jessica Yu <jeyu@kernel.org>
>
>
> While I am not opposed to this fix,
> I did not even notice Module.symvers was official interface.
>
> Kbuild invokes scripts/depmod.sh to finalize
> the 'make modules_install', but I do not see the -E
> option being used there.
>
> I do not see Module.symvers installed in
> /lib/modules/$(uname -r)/.
>
>
>



Applied to linux-kbuild/fixes.



-- 
Best Regards
Masahiro Yamada

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

* Re: [PATCH v2] modpost: move the namespace field in Module.symvers last
  2020-03-14  2:11 ` Masahiro Yamada
  2020-03-17  0:24   ` Masahiro Yamada
@ 2020-03-17 11:24   ` Jessica Yu
  1 sibling, 0 replies; 10+ messages in thread
From: Jessica Yu @ 2020-03-17 11:24 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Matthias Maennich, Lucas De Marchi, stable, Linux Kernel Mailing List

+++ Masahiro Yamada [14/03/20 11:11 +0900]:
>On Thu, Mar 12, 2020 at 2:02 AM Jessica Yu <jeyu@kernel.org> wrote:
>>
>> In order to preserve backwards compatability with kmod tools, we have to
>> move the namespace field in Module.symvers last, as the depmod -e -E
>> option looks at the first three fields in Module.symvers to check symbol
>> versions (and it's expected they stay in the original order of crc,
>> symbol, module).
>>
>> In addition, update an ancient comment above read_dump() in modpost that
>> suggested that the export type field in Module.symvers was optional. I
>> suspect that there were historical reasons behind that comment that are
>> no longer accurate. We have been unconditionally printing the export
>> type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.
>>
>> Fix up read_dump() to treat each field as non-optional. I suspect the
>> original read_dump() code treated the export field as optional in order
>> to support pre <= 2.6.18 Module.symvers (which did not have the export
>> type field). Note that although symbol namespaces are optional, the
>> field will not be omitted from Module.symvers if a symbol does not have
>> a namespace. In this case, the field will simply be empty and the next
>> delimiter or end of line will follow.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>> Tested-by: Matthias Maennich <maennich@google.com>
>> Reviewed-by: Matthias Maennich <maennich@google.com>
>> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> Signed-off-by: Jessica Yu <jeyu@kernel.org>
>
>
>While I am not opposed to this fix,
>I did not even notice Module.symvers was official interface.
>
>Kbuild invokes scripts/depmod.sh to finalize
>the 'make modules_install', but I do not see the -E
>option being used there.
>
>I do not see Module.symvers installed in
>/lib/modules/$(uname -r)/.

While that's true, distros typically package Module.symvers in their
linux-headers/kernel-devel (or equivalent) packages to support
external module builds. The -E option is usually used to do symbol
versioning/kabi checks with the Module.symvers file. Considering the
options, I think it will cause the least headaches down the line to
make this fix upstream and maintain backwards compatibility with kmod
rather than to patch kmod and remind distros to repackage because old
depmod versions don't work with the new Module.symvers anymore..

Thank you!

Jessica

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

* Re: [PATCH v2] modpost: move the namespace field in Module.symvers last
  2020-03-11 17:01 [PATCH v2] modpost: move the namespace field in Module.symvers last Jessica Yu
  2020-03-12  7:17 ` Lucas De Marchi
  2020-03-14  2:11 ` Masahiro Yamada
@ 2020-03-31  5:49 ` John Stultz
  2020-03-31  6:25   ` John Stultz
  2 siblings, 1 reply; 10+ messages in thread
From: John Stultz @ 2020-03-31  5:49 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Masahiro Yamada, Matthias Maennich, Lucas De Marchi, stable,
	Linux Kernel Mailing List

On Wed, Mar 11, 2020 at 10:03 AM Jessica Yu <jeyu@kernel.org> wrote:
>
> In order to preserve backwards compatability with kmod tools, we have to
> move the namespace field in Module.symvers last, as the depmod -e -E
> option looks at the first three fields in Module.symvers to check symbol
> versions (and it's expected they stay in the original order of crc,
> symbol, module).
>
> In addition, update an ancient comment above read_dump() in modpost that
> suggested that the export type field in Module.symvers was optional. I
> suspect that there were historical reasons behind that comment that are
> no longer accurate. We have been unconditionally printing the export
> type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.
>
> Fix up read_dump() to treat each field as non-optional. I suspect the
> original read_dump() code treated the export field as optional in order
> to support pre <= 2.6.18 Module.symvers (which did not have the export
> type field). Note that although symbol namespaces are optional, the
> field will not be omitted from Module.symvers if a symbol does not have
> a namespace. In this case, the field will simply be empty and the next
> delimiter or end of line will follow.
>
> Cc: stable@vger.kernel.org
> Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
> Tested-by: Matthias Maennich <maennich@google.com>
> Reviewed-by: Matthias Maennich <maennich@google.com>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Jessica Yu <jeyu@kernel.org>
> ---
> v2:
>
>   - Explain the changes to read_dump() and the comment (and provide
>     historical context) in the commit message. (Lucas De Marchi)
>
>  Documentation/kbuild/modules.rst |  4 ++--
>  scripts/export_report.pl         |  2 +-
>  scripts/mod/modpost.c            | 24 ++++++++++++------------
>  3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
> index 69fa48ee93d6..e0b45a257f21 100644
> --- a/Documentation/kbuild/modules.rst
> +++ b/Documentation/kbuild/modules.rst
> @@ -470,9 +470,9 @@ build.
>
>         The syntax of the Module.symvers file is::
>
> -       <CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
> +       <CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>
> -       0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
> +       0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>
>         The fields are separated by tabs and values may be empty (e.g.
>         if no namespace is defined for an exported symbol).

Despite the documentation here claiming the namespace field can be
empty, I'm seeing some trouble with this patch when building external
modules:
  FATAL: parse error in symbol dump file

I've confirmed reverting it make things work again, but its not clear
to me quite yet why.

The only difference I can find is that the Module.symvers in the
external module project doesn't seem to have a tab at the end of each
line (where as Module.symvers for the kernel - which doesn't seem to
have any namespace names - does).

Is there something I need to tweak on the external Kbuild to get
Module.symvers to be generated properly (with the empty tab at the
end) for this new change?
Or does the parser need to be a bit more flexible?

thanks
-john

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

* Re: [PATCH v2] modpost: move the namespace field in Module.symvers last
  2020-03-31  5:49 ` John Stultz
@ 2020-03-31  6:25   ` John Stultz
  2020-03-31  9:58     ` Jessica Yu
  0 siblings, 1 reply; 10+ messages in thread
From: John Stultz @ 2020-03-31  6:25 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Masahiro Yamada, Matthias Maennich, Lucas De Marchi, stable,
	Linux Kernel Mailing List

On Mon, Mar 30, 2020 at 10:49 PM John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Mar 11, 2020 at 10:03 AM Jessica Yu <jeyu@kernel.org> wrote:
> >
> > In order to preserve backwards compatability with kmod tools, we have to
> > move the namespace field in Module.symvers last, as the depmod -e -E
> > option looks at the first three fields in Module.symvers to check symbol
> > versions (and it's expected they stay in the original order of crc,
> > symbol, module).
> >
> > In addition, update an ancient comment above read_dump() in modpost that
> > suggested that the export type field in Module.symvers was optional. I
> > suspect that there were historical reasons behind that comment that are
> > no longer accurate. We have been unconditionally printing the export
> > type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.
> >
> > Fix up read_dump() to treat each field as non-optional. I suspect the
> > original read_dump() code treated the export field as optional in order
> > to support pre <= 2.6.18 Module.symvers (which did not have the export
> > type field). Note that although symbol namespaces are optional, the
> > field will not be omitted from Module.symvers if a symbol does not have
> > a namespace. In this case, the field will simply be empty and the next
> > delimiter or end of line will follow.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
> > Tested-by: Matthias Maennich <maennich@google.com>
> > Reviewed-by: Matthias Maennich <maennich@google.com>
> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Jessica Yu <jeyu@kernel.org>
> > ---
> > v2:
> >
> >   - Explain the changes to read_dump() and the comment (and provide
> >     historical context) in the commit message. (Lucas De Marchi)
> >
> >  Documentation/kbuild/modules.rst |  4 ++--
> >  scripts/export_report.pl         |  2 +-
> >  scripts/mod/modpost.c            | 24 ++++++++++++------------
> >  3 files changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
> > index 69fa48ee93d6..e0b45a257f21 100644
> > --- a/Documentation/kbuild/modules.rst
> > +++ b/Documentation/kbuild/modules.rst
> > @@ -470,9 +470,9 @@ build.
> >
> >         The syntax of the Module.symvers file is::
> >
> > -       <CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
> > +       <CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
> >
> > -       0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
> > +       0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
> >
> >         The fields are separated by tabs and values may be empty (e.g.
> >         if no namespace is defined for an exported symbol).
>
> Despite the documentation here claiming the namespace field can be
> empty, I'm seeing some trouble with this patch when building external
> modules:
>   FATAL: parse error in symbol dump file
>
> I've confirmed reverting it make things work again, but its not clear
> to me quite yet why.
>
> The only difference I can find is that the Module.symvers in the
> external module project doesn't seem to have a tab at the end of each
> line (where as Module.symvers for the kernel - which doesn't seem to
> have any namespace names - does).
>
> Is there something I need to tweak on the external Kbuild to get
> Module.symvers to be generated properly (with the empty tab at the
> end) for this new change?
> Or does the parser need to be a bit more flexible?
>

One extra clue on this: I noticed the external module Makefile had
KBUILD_EXTRA_SYMBOLS="$(EXTRA_SYMBOLS)"  in the $(MAKE) string, where
EXTRA_SYMBOLS pointed to some files that no longer exist.  I removed
the KBUILD_EXTRA_SYMBOLS= argument, and magically, the generated
Module.symvers now had tabs at the end of each line.

I wonder if there some path in the KBUILD_EXTRA_SYMBOLS= handling that
isn't generating the output in the same way?

thanks
-john

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

* Re: [PATCH v2] modpost: move the namespace field in Module.symvers last
  2020-03-31  6:25   ` John Stultz
@ 2020-03-31  9:58     ` Jessica Yu
  2020-03-31 22:09       ` John Stultz
  0 siblings, 1 reply; 10+ messages in thread
From: Jessica Yu @ 2020-03-31  9:58 UTC (permalink / raw)
  To: John Stultz
  Cc: Masahiro Yamada, Matthias Maennich, Lucas De Marchi, stable,
	Linux Kernel Mailing List

+++ John Stultz [30/03/20 23:25 -0700]:
>On Mon, Mar 30, 2020 at 10:49 PM John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, Mar 11, 2020 at 10:03 AM Jessica Yu <jeyu@kernel.org> wrote:
>> >
>> > In order to preserve backwards compatability with kmod tools, we have to
>> > move the namespace field in Module.symvers last, as the depmod -e -E
>> > option looks at the first three fields in Module.symvers to check symbol
>> > versions (and it's expected they stay in the original order of crc,
>> > symbol, module).
>> >
>> > In addition, update an ancient comment above read_dump() in modpost that
>> > suggested that the export type field in Module.symvers was optional. I
>> > suspect that there were historical reasons behind that comment that are
>> > no longer accurate. We have been unconditionally printing the export
>> > type since 2.6.18 (commit bd5cbcedf44), which is over a decade ago now.
>> >
>> > Fix up read_dump() to treat each field as non-optional. I suspect the
>> > original read_dump() code treated the export field as optional in order
>> > to support pre <= 2.6.18 Module.symvers (which did not have the export
>> > type field). Note that although symbol namespaces are optional, the
>> > field will not be omitted from Module.symvers if a symbol does not have
>> > a namespace. In this case, the field will simply be empty and the next
>> > delimiter or end of line will follow.
>> >
>> > Cc: stable@vger.kernel.org
>> > Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>> > Tested-by: Matthias Maennich <maennich@google.com>
>> > Reviewed-by: Matthias Maennich <maennich@google.com>
>> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> > Signed-off-by: Jessica Yu <jeyu@kernel.org>
>> > ---
>> > v2:
>> >
>> >   - Explain the changes to read_dump() and the comment (and provide
>> >     historical context) in the commit message. (Lucas De Marchi)
>> >
>> >  Documentation/kbuild/modules.rst |  4 ++--
>> >  scripts/export_report.pl         |  2 +-
>> >  scripts/mod/modpost.c            | 24 ++++++++++++------------
>> >  3 files changed, 15 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
>> > index 69fa48ee93d6..e0b45a257f21 100644
>> > --- a/Documentation/kbuild/modules.rst
>> > +++ b/Documentation/kbuild/modules.rst
>> > @@ -470,9 +470,9 @@ build.
>> >
>> >         The syntax of the Module.symvers file is::
>> >
>> > -       <CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
>> > +       <CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>> >
>> > -       0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
>> > +       0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>> >
>> >         The fields are separated by tabs and values may be empty (e.g.
>> >         if no namespace is defined for an exported symbol).
>>
>> Despite the documentation here claiming the namespace field can be
>> empty, I'm seeing some trouble with this patch when building external
>> modules:
>>   FATAL: parse error in symbol dump file
>>
>> I've confirmed reverting it make things work again, but its not clear
>> to me quite yet why.
>>
>> The only difference I can find is that the Module.symvers in the
>> external module project doesn't seem to have a tab at the end of each
>> line (where as Module.symvers for the kernel - which doesn't seem to
>> have any namespace names - does).
>>
>> Is there something I need to tweak on the external Kbuild to get
>> Module.symvers to be generated properly (with the empty tab at the
>> end) for this new change?
>> Or does the parser need to be a bit more flexible?
>>
>
>One extra clue on this: I noticed the external module Makefile had
>KBUILD_EXTRA_SYMBOLS="$(EXTRA_SYMBOLS)"  in the $(MAKE) string, where
>EXTRA_SYMBOLS pointed to some files that no longer exist.  I removed
>the KBUILD_EXTRA_SYMBOLS= argument, and magically, the generated
>Module.symvers now had tabs at the end of each line.
>
>I wonder if there some path in the KBUILD_EXTRA_SYMBOLS= handling that
>isn't generating the output in the same way?

I'm afraid we're going to need some specifics on reproducing this
issue. Could you provide a reproducer or steps on how to reproduce? I
have not been able to trigger this problem even with
KBUILD_EXTRA_SYMBOLS pointing to an invalid path (I also tested with
valid paths).

I tested with a skeleton external module that exports two functions,
one with a namespace and one without. I built this module against the
latest v5.6 kernel. The generated Module.symvers was correct - the
namespaced function had the namespace at the end and the other
function without a namespace had a tab at the end.

I also tested with two external modules, one with a symbol dependency
on the other, so KBUILD_EXTRA_SYMBOLS usage is required here. The
generated Module.symvers was also correct here.

The only advice I can offer at this time is that all external modules
must be built against the new kernel to generate a correctly formated
Module.symvers file. If you have any KBUILD_EXTRA_SYMBOLS pointing to
an outdated Module.symvers file for example, you will see the "FATAL:
parse error in symbol dump file" error.

Hope this helps,

Jessica


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

* Re: [PATCH v2] modpost: move the namespace field in Module.symvers last
  2020-03-31  9:58     ` Jessica Yu
@ 2020-03-31 22:09       ` John Stultz
  2020-04-01  8:00         ` Jessica Yu
  0 siblings, 1 reply; 10+ messages in thread
From: John Stultz @ 2020-03-31 22:09 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Masahiro Yamada, Matthias Maennich, Lucas De Marchi, stable,
	Linux Kernel Mailing List

On Tue, Mar 31, 2020 at 2:58 AM Jessica Yu <jeyu@kernel.org> wrote:
> +++ John Stultz [30/03/20 23:25 -0700]:
> >On Mon, Mar 30, 2020 at 10:49 PM John Stultz <john.stultz@linaro.org> wrote:
> >> The only difference I can find is that the Module.symvers in the
> >> external module project doesn't seem to have a tab at the end of each
> >> line (where as Module.symvers for the kernel - which doesn't seem to
> >> have any namespace names - does).
> >>
> >> Is there something I need to tweak on the external Kbuild to get
> >> Module.symvers to be generated properly (with the empty tab at the
> >> end) for this new change?
> >> Or does the parser need to be a bit more flexible?
> >>
> >
> >One extra clue on this: I noticed the external module Makefile had
> >KBUILD_EXTRA_SYMBOLS="$(EXTRA_SYMBOLS)"  in the $(MAKE) string, where
> >EXTRA_SYMBOLS pointed to some files that no longer exist.  I removed
> >the KBUILD_EXTRA_SYMBOLS= argument, and magically, the generated
> >Module.symvers now had tabs at the end of each line.
> >
> >I wonder if there some path in the KBUILD_EXTRA_SYMBOLS= handling that
> >isn't generating the output in the same way?
>
> I'm afraid we're going to need some specifics on reproducing this
> issue. Could you provide a reproducer or steps on how to reproduce? I
> have not been able to trigger this problem even with
> KBUILD_EXTRA_SYMBOLS pointing to an invalid path (I also tested with
> valid paths).
>
> I tested with a skeleton external module that exports two functions,
> one with a namespace and one without. I built this module against the
> latest v5.6 kernel. The generated Module.symvers was correct - the
> namespaced function had the namespace at the end and the other
> function without a namespace had a tab at the end.
>
> I also tested with two external modules, one with a symbol dependency
> on the other, so KBUILD_EXTRA_SYMBOLS usage is required here. The
> generated Module.symvers was also correct here.
>
> The only advice I can offer at this time is that all external modules
> must be built against the new kernel to generate a correctly formated
> Module.symvers file. If you have any KBUILD_EXTRA_SYMBOLS pointing to
> an outdated Module.symvers file for example, you will see the "FATAL:
> parse error in symbol dump file" error.

Well, my apologies.  :(  In the light of day, this isn't reproducing
anymore. I'm a bit at a loss as to why I was tripping over it so
regularly before, but I suspect something in the build is leaving a
stale Modules.symvers around from before this patch landed.

Terribly sorry for the noise.

thanks
-john

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

* Re: [PATCH v2] modpost: move the namespace field in Module.symvers last
  2020-03-31 22:09       ` John Stultz
@ 2020-04-01  8:00         ` Jessica Yu
  0 siblings, 0 replies; 10+ messages in thread
From: Jessica Yu @ 2020-04-01  8:00 UTC (permalink / raw)
  To: John Stultz
  Cc: Masahiro Yamada, Matthias Maennich, Lucas De Marchi, stable,
	Linux Kernel Mailing List

+++ John Stultz [31/03/20 15:09 -0700]:
>On Tue, Mar 31, 2020 at 2:58 AM Jessica Yu <jeyu@kernel.org> wrote:
>> +++ John Stultz [30/03/20 23:25 -0700]:
>> >On Mon, Mar 30, 2020 at 10:49 PM John Stultz <john.stultz@linaro.org> wrote:
>> >> The only difference I can find is that the Module.symvers in the
>> >> external module project doesn't seem to have a tab at the end of each
>> >> line (where as Module.symvers for the kernel - which doesn't seem to
>> >> have any namespace names - does).
>> >>
>> >> Is there something I need to tweak on the external Kbuild to get
>> >> Module.symvers to be generated properly (with the empty tab at the
>> >> end) for this new change?
>> >> Or does the parser need to be a bit more flexible?
>> >>
>> >
>> >One extra clue on this: I noticed the external module Makefile had
>> >KBUILD_EXTRA_SYMBOLS="$(EXTRA_SYMBOLS)"  in the $(MAKE) string, where
>> >EXTRA_SYMBOLS pointed to some files that no longer exist.  I removed
>> >the KBUILD_EXTRA_SYMBOLS= argument, and magically, the generated
>> >Module.symvers now had tabs at the end of each line.
>> >
>> >I wonder if there some path in the KBUILD_EXTRA_SYMBOLS= handling that
>> >isn't generating the output in the same way?
>>
>> I'm afraid we're going to need some specifics on reproducing this
>> issue. Could you provide a reproducer or steps on how to reproduce? I
>> have not been able to trigger this problem even with
>> KBUILD_EXTRA_SYMBOLS pointing to an invalid path (I also tested with
>> valid paths).
>>
>> I tested with a skeleton external module that exports two functions,
>> one with a namespace and one without. I built this module against the
>> latest v5.6 kernel. The generated Module.symvers was correct - the
>> namespaced function had the namespace at the end and the other
>> function without a namespace had a tab at the end.
>>
>> I also tested with two external modules, one with a symbol dependency
>> on the other, so KBUILD_EXTRA_SYMBOLS usage is required here. The
>> generated Module.symvers was also correct here.
>>
>> The only advice I can offer at this time is that all external modules
>> must be built against the new kernel to generate a correctly formated
>> Module.symvers file. If you have any KBUILD_EXTRA_SYMBOLS pointing to
>> an outdated Module.symvers file for example, you will see the "FATAL:
>> parse error in symbol dump file" error.
>
>Well, my apologies.  :(  In the light of day, this isn't reproducing
>anymore. I'm a bit at a loss as to why I was tripping over it so
>regularly before, but I suspect something in the build is leaving a
>stale Modules.symvers around from before this patch landed.
>
>Terribly sorry for the noise.

No problem, thank for reporting back!

Jessica

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

end of thread, other threads:[~2020-04-01  8:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 17:01 [PATCH v2] modpost: move the namespace field in Module.symvers last Jessica Yu
2020-03-12  7:17 ` Lucas De Marchi
2020-03-14  2:11 ` Masahiro Yamada
2020-03-17  0:24   ` Masahiro Yamada
2020-03-17 11:24   ` Jessica Yu
2020-03-31  5:49 ` John Stultz
2020-03-31  6:25   ` John Stultz
2020-03-31  9:58     ` Jessica Yu
2020-03-31 22:09       ` John Stultz
2020-04-01  8:00         ` Jessica Yu

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).