| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200306002250.4hxhtibvoiobosnr@ldmartin-desk1>
Date: Thu, 5 Mar 2020 16:22:50 -0800
From: Lucas De Marchi <lucas.demarchi@...el.com>
To: Jessica Yu <jeyu@...nel.org>
Cc: Masahiro Yamada <yamada.masahiro@...ionext.com>,
Matthias Maennich <maennich@...gle.com>,
Lucas De Marchi <lucas.de.marchi@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] modpost: move the namespace field in Module.symvers last
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@...r.kernel.org
>>Signed-off-by: Jessica Yu <jeyu@...nel.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
>>
Powered by blists - more mailing lists