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

[Lucas De Marchi]

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@xxxxxxxxxxxxxxx
> > Signed-off-by: Jessica Yu <jeyu@xxxxxxxxxx>
>
> 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