lkml.org 
[lkml]   [2020]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] modpost: move the namespace field in Module.symvers last
+++ 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...

> **/
> 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.

>+ 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
>

\
 
 \ /
  Last update: 2020-03-04 18:22    [W:0.172 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site