Re: [PATCH] modpost: move the namespace field in Module.symvers last

[Lucas De Marchi <lucas.demarchi@intel.com>]

Hi,

On Wed, Mar 04, 2020 at 06:22:23PM +0100, Jessica Yu wrote:
>+++ Jessica Yu [04/03/20 18:03 +0100]:
>>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).
>>
>>Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>>Cc: stable@vger.kernel.org
>>Signed-off-by: Jessica Yu <jeyu@kernel.org>
>
>First, I apologize for not having caught this mistake earlier. I still
>have questions about the Module.symvers format, please see below.
>
>>---
>>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 7edfdb2f4497..6ab235354f36 100644
>>--- a/scripts/mod/modpost.c
>>+++ b/scripts/mod/modpost.c
>>@@ -2427,7 +2427,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
>
>So, this comment was a source of confusion for me and Matthias I
>think. It suggests that the export field is optional, and that even
>following the export field there may also be "something" else,
>whatever that is.
>
>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. And let me explain the read_dump() changes...

I think this is a good information to be amended in the commit message,
so 10 years from now we can get find this info in git log.

>
>> **/
>>static void read_dump(const char *fname, unsigned int kernel)
>>{
>>@@ -2440,7 +2440,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;
>>@@ -2448,16 +2448,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';
>
>I believe the original read_dump() code treated the export field here
>as optional, to support pre <= 2.6.18 Module.symvers (which does not
>have the export type field). But I don't believe we have to support
>this case anymore, right? It's ages ago. So I cleaned up this area,
>made each field non-optional (but empty string "" for namespace is
>allowed), and updated the comment.

Same here. And agreed.

thanks

>
>>+		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;
>>@@ -2508,9 +2508,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
>>