I was hit by some problems caused by the module namespace feature
that was merged recently. At least, the breakage of
external module builds is a fatal one. I just took a look at the code
closer, and I noticed some more issues (some are nit-picking).
V2:
- I dropped "module: avoid code duplication in include/linux/export.h"
because Matthias offered to refactor the code by himself.
- V1 missed the problem when a symbol is preloaded before
sym_add_exported() is called. I fixed it too.
Masahiro Yamada (6):
module: swap the order of symbol.namespace
modpost: fix broken sym->namespace for external module builds
module: rename __kstrtab_ns_* to __kstrtabns_* to avoid symbol
conflict
kbuild: fix build error of 'make nsdeps' in clean tree
nsdeps: fix hashbang of scripts/nsdeps
nsdeps: make generated patches independent of locale
Makefile | 2 +-
include/linux/export.h | 10 +++++-----
scripts/mod/modpost.c | 29 +++++++++++++++--------------
scripts/nsdeps | 4 ++--
4 files changed, 23 insertions(+), 22 deletions(-)
--
2.17.1
Currently, EXPORT_SYMBOL_NS(_GPL) constructs the kernel symbol as
follows:
__ksymtab_SYMBOL.NAMESPACE
The sym_extract_namespace() in modpost allocates memory for the part
SYMBOL.NAMESPACE when '.' is contained. One problem is that the pointer
returned by strdup() is lost because the symbol name will be copied to
malloc'ed memory by alloc_symbol(). No one will keep track of the
pointer of strdup'ed memory.
sym->namespace still points to the NAMESPACE part. So, you can free it
with complicated code like this:
free(sym->namespace - strlen(sym->name) - 1);
It complicates memory free.
To fix it elegantly, I swapped the order of the symbol and the
namespace as follows:
__ksymtab_NAMESPACE.SYMBOL
then, simplified sym_extract_namespace() so that it allocates memory
only for the NAMESPACE part.
I prefer this order because it is intuitive and also matches to major
languages. For example, NAMESPACE::NAME in C++, MODULE.NAME in Python.
Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Matthias Maennich <[email protected]>
---
Changes in v2: None
include/linux/export.h | 4 ++--
scripts/mod/modpost.c | 16 +++++++---------
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/include/linux/export.h b/include/linux/export.h
index 95f55b7f83a0..0695d4e847d9 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -52,7 +52,7 @@ extern struct module __this_module;
__ADDRESSABLE(sym) \
asm(" .section \"___ksymtab" sec "+" #sym "\", \"a\" \n" \
" .balign 4 \n" \
- "__ksymtab_" #sym NS_SEPARATOR #ns ": \n" \
+ "__ksymtab_" #ns NS_SEPARATOR #sym ": \n" \
" .long " #sym "- . \n" \
" .long __kstrtab_" #sym "- . \n" \
" .long __kstrtab_ns_" #sym "- . \n" \
@@ -76,7 +76,7 @@ struct kernel_symbol {
#else
#define __KSYMTAB_ENTRY_NS(sym, sec, ns) \
static const struct kernel_symbol __ksymtab_##sym##__##ns \
- asm("__ksymtab_" #sym NS_SEPARATOR #ns) \
+ asm("__ksymtab_" #ns NS_SEPARATOR #sym) \
__attribute__((section("___ksymtab" sec "+" #sym), used)) \
__aligned(sizeof(void *)) \
= { (unsigned long)&sym, __kstrtab_##sym, __kstrtab_ns_##sym }
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 442d5e2ad688..2c644086c412 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -350,18 +350,16 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
static const char *sym_extract_namespace(const char **symname)
{
- size_t n;
- char *dupsymname;
+ char *namespace = NULL;
+ char *ns_separator;
- n = strcspn(*symname, ".");
- if (n < strlen(*symname) - 1) {
- dupsymname = NOFAIL(strdup(*symname));
- dupsymname[n] = '\0';
- *symname = dupsymname;
- return dupsymname + n + 1;
+ ns_separator = strchr(*symname, '.');
+ if (ns_separator) {
+ namespace = NOFAIL(strndup(*symname, ns_separator - *symname));
+ *symname = ns_separator + 1;
}
- return NULL;
+ return namespace;
}
/**
--
2.17.1
Running 'make nsdeps' in a clean source tree fails as follows:
$ make -s clean; make -s defconfig; make nsdeps
[ snip ]
awk: fatal: cannot open file `init/modules.order' for reading (No such file or directory)
make: *** [Makefile;1307: modules.order] Error 2
make: *** Deleting file 'modules.order'
make: *** Waiting for unfinished jobs....
The cause of the error is 'make nsdeps' does not build modules at all.
Set KBUILD_MODULES to fix it.
Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Matthias Maennich <[email protected]>
---
Changes in v2: None
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 6f54f2f95743..d97f38ad5d9f 100644
--- a/Makefile
+++ b/Makefile
@@ -616,7 +616,7 @@ endif
# in addition to whatever we do anyway.
# Just "make" or "make all" shall build modules as well
-ifneq ($(filter all _all modules,$(MAKECMDGOALS)),)
+ifneq ($(filter all _all modules nsdeps,$(MAKECMDGOALS)),)
KBUILD_MODULES := 1
endif
--
2.17.1
The module namespace produces __strtab_ns_<sym> symbols to store
namespace strings, but it does not guarantee the name uniqueness.
This is a potential problem because we have exported symbols starting
with "ns_".
For example, kernel/capability.c exports the following symbols:
EXPORT_SYMBOL(ns_capable);
EXPORT_SYMBOL(capable);
Assume a situation where those are converted as follows:
EXPORT_SYMBOL_NS(ns_capable, some_namespace);
EXPORT_SYMBOL_NS(capable, some_namespace);
The former expands to "__kstrtab_ns_capable" and "__kstrtab_ns_ns_capable",
and the latter to "__kstrtab_capable" and "__kstrtab_ns_capable".
Then, we have the duplicated "__kstrtab_ns_capable".
To ensure the uniqueness, rename "__kstrtab_ns_*" to "__kstrtabns_*".
Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Matthias Maennich <[email protected]>
---
Changes in v2: None
include/linux/export.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/export.h b/include/linux/export.h
index 0695d4e847d9..621158ecd2e2 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -55,7 +55,7 @@ extern struct module __this_module;
"__ksymtab_" #ns NS_SEPARATOR #sym ": \n" \
" .long " #sym "- . \n" \
" .long __kstrtab_" #sym "- . \n" \
- " .long __kstrtab_ns_" #sym "- . \n" \
+ " .long __kstrtabns_" #sym "- . \n" \
" .previous \n")
#define __KSYMTAB_ENTRY(sym, sec) \
@@ -79,7 +79,7 @@ struct kernel_symbol {
asm("__ksymtab_" #ns NS_SEPARATOR #sym) \
__attribute__((section("___ksymtab" sec "+" #sym), used)) \
__aligned(sizeof(void *)) \
- = { (unsigned long)&sym, __kstrtab_##sym, __kstrtab_ns_##sym }
+ = { (unsigned long)&sym, __kstrtab_##sym, __kstrtabns_##sym }
#define __KSYMTAB_ENTRY(sym, sec) \
static const struct kernel_symbol __ksymtab_##sym \
@@ -112,7 +112,7 @@ struct kernel_symbol {
/* For every exported symbol, place a struct in the __ksymtab section */
#define ___EXPORT_SYMBOL_NS(sym, sec, ns) \
___export_symbol_common(sym, sec); \
- static const char __kstrtab_ns_##sym[] \
+ static const char __kstrtabns_##sym[] \
__attribute__((section("__ksymtab_strings"), used, aligned(1))) \
= #ns; \
__KSYMTAB_ENTRY_NS(sym, sec, ns)
--
2.17.1
Currently, external module builds produce tons of false-positives:
WARNING: module <mod> uses symbol <sym> from namespace <ns>, but does not import it.
Here, the <ns> part shows a random string.
When you build external modules, the symbol info of vmlinux and
in-kernel modules are read from $(objtree)/Module.symvers, but
read_dump() is buggy in multiple ways:
[1] When the modpost is run for vmlinux and in-kernel modules,
sym_extract_namespace() allocates memory for the namespace. On the
other hand, read_dump() does not, then sym->namespace will point to
somewhere in the line buffer of get_next_line(). The data in the
buffer will be replaced soon, and sym->namespace will end up with
pointing to unrelated data. As a result, check_exports() will show
random strings in the warning messages.
[2] When there is no namespace, sym_extract_namespace() returns NULL.
On the other hand, read_dump() sets namespace to an empty string "".
(but, it will be later replaced with unrelated data due to bug [1].)
The check_exports() shows a warning unless exp->namespace is NULL,
so every symbol read from read_dump() emits the warning, which is
mostly false positive.
To address [1], sym_add_exported() calls strdup() for s->namespace.
The namespace from sym_extract_namespace() must be freed to avoid
memory leak.
For [2], I changed the if-conditional in check_exports().
This commit also fixes sym_add_exported() to set s->namespace correctly
when the symbol is preloaded.
Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Matthias Maennich <[email protected]>
---
Changes in v2:
- Change the approach to deal with ->preloaded
scripts/mod/modpost.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 2c644086c412..936d3ad23c83 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -166,7 +166,7 @@ struct symbol {
struct module *module;
unsigned int crc;
int crc_valid;
- const char *namespace;
+ char *namespace;
unsigned int weak:1;
unsigned int vmlinux:1; /* 1 if symbol is defined in vmlinux */
unsigned int kernel:1; /* 1 if symbol is from kernel
@@ -348,7 +348,7 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
return export_unknown;
}
-static const char *sym_extract_namespace(const char **symname)
+static char *sym_extract_namespace(const char **symname)
{
char *namespace = NULL;
char *ns_separator;
@@ -373,7 +373,6 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
if (!s) {
s = new_symbol(name, mod, export);
- s->namespace = namespace;
} else {
if (!s->preloaded) {
warn("%s: '%s' exported twice. Previous export was in %s%s\n",
@@ -384,6 +383,8 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
s->module = mod;
}
}
+ free(s->namespace);
+ s->namespace = namespace ? strdup(namespace) : NULL;
s->preloaded = 0;
s->vmlinux = is_vmlinux(mod->name);
s->kernel = 0;
@@ -670,7 +671,8 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
unsigned int crc;
enum export export;
bool is_crc = false;
- const char *name, *namespace;
+ const char *name;
+ char *namespace;
if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
strstarts(symname, "__ksymtab"))
@@ -745,6 +747,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
name = symname + strlen("__ksymtab_");
namespace = sym_extract_namespace(&name);
sym_add_exported(name, namespace, mod, export);
+ free(namespace);
}
if (strcmp(symname, "init_module") == 0)
mod->has_init = 1;
@@ -2193,7 +2196,7 @@ static int check_exports(struct module *mod)
else
basename = mod->name;
- if (exp->namespace) {
+ if (exp->namespace && exp->namespace[0]) {
add_namespace(&mod->required_namespaces,
exp->namespace);
--
2.17.1
This script does not use bash-extension. I am guessing this hashbang
was copied from scripts/coccicheck, which really uses bash-extension.
/bin/sh is enough for this script.
Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Matthias Maennich <[email protected]>
---
Changes in v2: None
scripts/nsdeps | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/nsdeps b/scripts/nsdeps
index ac2b6031dd13..964b7fb8c546 100644
--- a/scripts/nsdeps
+++ b/scripts/nsdeps
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
# SPDX-License-Identifier: GPL-2.0
# Linux kernel symbol namespace import generator
#
--
2.17.1
On Thu, Oct 03, 2019 at 04:58:22PM +0900, Masahiro Yamada wrote:
> Currently, external module builds produce tons of false-positives:
>
> WARNING: module <mod> uses symbol <sym> from namespace <ns>, but does not import it.
>
> Here, the <ns> part shows a random string.
>
> When you build external modules, the symbol info of vmlinux and
> in-kernel modules are read from $(objtree)/Module.symvers, but
> read_dump() is buggy in multiple ways:
>
> [1] When the modpost is run for vmlinux and in-kernel modules,
> sym_extract_namespace() allocates memory for the namespace. On the
> other hand, read_dump() does not, then sym->namespace will point to
> somewhere in the line buffer of get_next_line(). The data in the
> buffer will be replaced soon, and sym->namespace will end up with
> pointing to unrelated data. As a result, check_exports() will show
> random strings in the warning messages.
>
> [2] When there is no namespace, sym_extract_namespace() returns NULL.
> On the other hand, read_dump() sets namespace to an empty string "".
> (but, it will be later replaced with unrelated data due to bug [1].)
> The check_exports() shows a warning unless exp->namespace is NULL,
> so every symbol read from read_dump() emits the warning, which is
> mostly false positive.
>
> To address [1], sym_add_exported() calls strdup() for s->namespace.
> The namespace from sym_extract_namespace() must be freed to avoid
> memory leak.
>
> For [2], I changed the if-conditional in check_exports().
>
> This commit also fixes sym_add_exported() to set s->namespace correctly
> when the symbol is preloaded.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> Reviewed-by: Matthias Maennich <[email protected]>
> ---
>
> Changes in v2:
> - Change the approach to deal with ->preloaded
>
> scripts/mod/modpost.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 2c644086c412..936d3ad23c83 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -166,7 +166,7 @@ struct symbol {
> struct module *module;
> unsigned int crc;
> int crc_valid;
> - const char *namespace;
> + char *namespace;
> unsigned int weak:1;
> unsigned int vmlinux:1; /* 1 if symbol is defined in vmlinux */
> unsigned int kernel:1; /* 1 if symbol is from kernel
> @@ -348,7 +348,7 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
> return export_unknown;
> }
>
> -static const char *sym_extract_namespace(const char **symname)
> +static char *sym_extract_namespace(const char **symname)
> {
> char *namespace = NULL;
> char *ns_separator;
> @@ -373,7 +373,6 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
>
> if (!s) {
> s = new_symbol(name, mod, export);
> - s->namespace = namespace;
> } else {
> if (!s->preloaded) {
> warn("%s: '%s' exported twice. Previous export was in %s%s\n",
> @@ -384,6 +383,8 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
> s->module = mod;
> }
> }
> + free(s->namespace);
> + s->namespace = namespace ? strdup(namespace) : NULL;
> s->preloaded = 0;
> s->vmlinux = is_vmlinux(mod->name);
> s->kernel = 0;
> @@ -670,7 +671,8 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> unsigned int crc;
> enum export export;
> bool is_crc = false;
> - const char *name, *namespace;
> + const char *name;
> + char *namespace;
>
> if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
> strstarts(symname, "__ksymtab"))
> @@ -745,6 +747,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> name = symname + strlen("__ksymtab_");
> namespace = sym_extract_namespace(&name);
> sym_add_exported(name, namespace, mod, export);
> + free(namespace);
> }
> if (strcmp(symname, "init_module") == 0)
> mod->has_init = 1;
> @@ -2193,7 +2196,7 @@ static int check_exports(struct module *mod)
> else
> basename = mod->name;
>
> - if (exp->namespace) {
> + if (exp->namespace && exp->namespace[0]) {
> add_namespace(&mod->required_namespaces,
> exp->namespace);
>
This looks good to me and is better than what I had originally proposed.
I confirmed that I can still build an external module without any
warnings. (But I did have to convince myself that it was OK to store
empty namespace strings in the symbol structure and that check_exports()
would cover it sufficiently)
If you would like, feel free to add my
Reviewed-by: Shaun Ruffell <[email protected]>
or
Tested-by: Shaun Ruffell <[email protected]>
Hi Shaun,
On Thu, Oct 3, 2019 at 10:29 PM Shaun Ruffell <[email protected]> wrote:
>
> On Thu, Oct 03, 2019 at 04:58:22PM +0900, Masahiro Yamada wrote:
> > Currently, external module builds produce tons of false-positives:
> >
> > WARNING: module <mod> uses symbol <sym> from namespace <ns>, but does not import it.
> >
> > Here, the <ns> part shows a random string.
> >
> > When you build external modules, the symbol info of vmlinux and
> > in-kernel modules are read from $(objtree)/Module.symvers, but
> > read_dump() is buggy in multiple ways:
> >
> > [1] When the modpost is run for vmlinux and in-kernel modules,
> > sym_extract_namespace() allocates memory for the namespace. On the
> > other hand, read_dump() does not, then sym->namespace will point to
> > somewhere in the line buffer of get_next_line(). The data in the
> > buffer will be replaced soon, and sym->namespace will end up with
> > pointing to unrelated data. As a result, check_exports() will show
> > random strings in the warning messages.
> >
> > [2] When there is no namespace, sym_extract_namespace() returns NULL.
> > On the other hand, read_dump() sets namespace to an empty string "".
> > (but, it will be later replaced with unrelated data due to bug [1].)
> > The check_exports() shows a warning unless exp->namespace is NULL,
> > so every symbol read from read_dump() emits the warning, which is
> > mostly false positive.
> >
> > To address [1], sym_add_exported() calls strdup() for s->namespace.
> > The namespace from sym_extract_namespace() must be freed to avoid
> > memory leak.
> >
> > For [2], I changed the if-conditional in check_exports().
> >
> > This commit also fixes sym_add_exported() to set s->namespace correctly
> > when the symbol is preloaded.
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > Reviewed-by: Matthias Maennich <[email protected]>
> > ---
> >
> > Changes in v2:
> > - Change the approach to deal with ->preloaded
> >
> > scripts/mod/modpost.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 2c644086c412..936d3ad23c83 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -166,7 +166,7 @@ struct symbol {
> > struct module *module;
> > unsigned int crc;
> > int crc_valid;
> > - const char *namespace;
> > + char *namespace;
> > unsigned int weak:1;
> > unsigned int vmlinux:1; /* 1 if symbol is defined in vmlinux */
> > unsigned int kernel:1; /* 1 if symbol is from kernel
> > @@ -348,7 +348,7 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
> > return export_unknown;
> > }
> >
> > -static const char *sym_extract_namespace(const char **symname)
> > +static char *sym_extract_namespace(const char **symname)
> > {
> > char *namespace = NULL;
> > char *ns_separator;
> > @@ -373,7 +373,6 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
> >
> > if (!s) {
> > s = new_symbol(name, mod, export);
> > - s->namespace = namespace;
> > } else {
> > if (!s->preloaded) {
> > warn("%s: '%s' exported twice. Previous export was in %s%s\n",
> > @@ -384,6 +383,8 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
> > s->module = mod;
> > }
> > }
> > + free(s->namespace);
> > + s->namespace = namespace ? strdup(namespace) : NULL;
> > s->preloaded = 0;
> > s->vmlinux = is_vmlinux(mod->name);
> > s->kernel = 0;
> > @@ -670,7 +671,8 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> > unsigned int crc;
> > enum export export;
> > bool is_crc = false;
> > - const char *name, *namespace;
> > + const char *name;
> > + char *namespace;
> >
> > if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
> > strstarts(symname, "__ksymtab"))
> > @@ -745,6 +747,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
> > name = symname + strlen("__ksymtab_");
> > namespace = sym_extract_namespace(&name);
> > sym_add_exported(name, namespace, mod, export);
> > + free(namespace);
> > }
> > if (strcmp(symname, "init_module") == 0)
> > mod->has_init = 1;
> > @@ -2193,7 +2196,7 @@ static int check_exports(struct module *mod)
> > else
> > basename = mod->name;
> >
> > - if (exp->namespace) {
> > + if (exp->namespace && exp->namespace[0]) {
> > add_namespace(&mod->required_namespaces,
> > exp->namespace);
> >
>
> This looks good to me and is better than what I had originally proposed.
> I confirmed that I can still build an external module without any
> warnings. (But I did have to convince myself that it was OK to store
> empty namespace strings in the symbol structure and that check_exports()
> would cover it sufficiently)
You have a point.
The change to check_exports() looks strange.
It is actually related to my previous patch submission.
See this patch:
https://lore.kernel.org/patchwork/patch/1131970/
Currently, the NULL pointer means no namespace.
I noticed passing an empty string as the namespace
simplified <linux/export.h> a lot.
So, I changed it in a way that
an empty string also means no namespace.
Anyway, Matthias took over the refactoring work.
So, I am not sure if this change is still helpful...
If it is no longer useful, I am happy to send v3
with the following:
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 442d5e2ad688..bdd3956e89c9 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2453,6 +2453,8 @@ static void read_dump(const char *fname,
unsigned int kernel)
mod = new_module(modname);
mod->skip = 1;
}
+ if (namespace[0] == '\0')
+ namespace = NULL;
s = sym_add_exported(symname, namespace, mod,
export_no(export));
s->kernel = kernel;
--
Best Regards
Masahiro Yamada
+++ Masahiro Yamada [03/10/19 23:42 +0900]:
>Hi Shaun,
>
>On Thu, Oct 3, 2019 at 10:29 PM Shaun Ruffell <[email protected]> wrote:
>>
>> On Thu, Oct 03, 2019 at 04:58:22PM +0900, Masahiro Yamada wrote:
>> > Currently, external module builds produce tons of false-positives:
>> >
>> > WARNING: module <mod> uses symbol <sym> from namespace <ns>, but does not import it.
>> >
>> > Here, the <ns> part shows a random string.
>> >
>> > When you build external modules, the symbol info of vmlinux and
>> > in-kernel modules are read from $(objtree)/Module.symvers, but
>> > read_dump() is buggy in multiple ways:
>> >
>> > [1] When the modpost is run for vmlinux and in-kernel modules,
>> > sym_extract_namespace() allocates memory for the namespace. On the
>> > other hand, read_dump() does not, then sym->namespace will point to
>> > somewhere in the line buffer of get_next_line(). The data in the
>> > buffer will be replaced soon, and sym->namespace will end up with
>> > pointing to unrelated data. As a result, check_exports() will show
>> > random strings in the warning messages.
>> >
>> > [2] When there is no namespace, sym_extract_namespace() returns NULL.
>> > On the other hand, read_dump() sets namespace to an empty string "".
>> > (but, it will be later replaced with unrelated data due to bug [1].)
>> > The check_exports() shows a warning unless exp->namespace is NULL,
>> > so every symbol read from read_dump() emits the warning, which is
>> > mostly false positive.
>> >
>> > To address [1], sym_add_exported() calls strdup() for s->namespace.
>> > The namespace from sym_extract_namespace() must be freed to avoid
>> > memory leak.
>> >
>> > For [2], I changed the if-conditional in check_exports().
>> >
>> > This commit also fixes sym_add_exported() to set s->namespace correctly
>> > when the symbol is preloaded.
>> >
>> > Signed-off-by: Masahiro Yamada <[email protected]>
>> > Reviewed-by: Matthias Maennich <[email protected]>
>> > ---
>> >
>> > Changes in v2:
>> > - Change the approach to deal with ->preloaded
>> >
>> > scripts/mod/modpost.c | 13 ++++++++-----
>> > 1 file changed, 8 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>> > index 2c644086c412..936d3ad23c83 100644
>> > --- a/scripts/mod/modpost.c
>> > +++ b/scripts/mod/modpost.c
>> > @@ -166,7 +166,7 @@ struct symbol {
>> > struct module *module;
>> > unsigned int crc;
>> > int crc_valid;
>> > - const char *namespace;
>> > + char *namespace;
>> > unsigned int weak:1;
>> > unsigned int vmlinux:1; /* 1 if symbol is defined in vmlinux */
>> > unsigned int kernel:1; /* 1 if symbol is from kernel
>> > @@ -348,7 +348,7 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
>> > return export_unknown;
>> > }
>> >
>> > -static const char *sym_extract_namespace(const char **symname)
>> > +static char *sym_extract_namespace(const char **symname)
>> > {
>> > char *namespace = NULL;
>> > char *ns_separator;
>> > @@ -373,7 +373,6 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
>> >
>> > if (!s) {
>> > s = new_symbol(name, mod, export);
>> > - s->namespace = namespace;
>> > } else {
>> > if (!s->preloaded) {
>> > warn("%s: '%s' exported twice. Previous export was in %s%s\n",
>> > @@ -384,6 +383,8 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace,
>> > s->module = mod;
>> > }
>> > }
>> > + free(s->namespace);
>> > + s->namespace = namespace ? strdup(namespace) : NULL;
>> > s->preloaded = 0;
>> > s->vmlinux = is_vmlinux(mod->name);
>> > s->kernel = 0;
>> > @@ -670,7 +671,8 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
>> > unsigned int crc;
>> > enum export export;
>> > bool is_crc = false;
>> > - const char *name, *namespace;
>> > + const char *name;
>> > + char *namespace;
>> >
>> > if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
>> > strstarts(symname, "__ksymtab"))
>> > @@ -745,6 +747,7 @@ static void handle_modversions(struct module *mod, struct elf_info *info,
>> > name = symname + strlen("__ksymtab_");
>> > namespace = sym_extract_namespace(&name);
>> > sym_add_exported(name, namespace, mod, export);
>> > + free(namespace);
>> > }
>> > if (strcmp(symname, "init_module") == 0)
>> > mod->has_init = 1;
>> > @@ -2193,7 +2196,7 @@ static int check_exports(struct module *mod)
>> > else
>> > basename = mod->name;
>> >
>> > - if (exp->namespace) {
>> > + if (exp->namespace && exp->namespace[0]) {
>> > add_namespace(&mod->required_namespaces,
>> > exp->namespace);
>> >
>>
>> This looks good to me and is better than what I had originally proposed.
>> I confirmed that I can still build an external module without any
>> warnings. (But I did have to convince myself that it was OK to store
>> empty namespace strings in the symbol structure and that check_exports()
>> would cover it sufficiently)
>
>You have a point.
>
>The change to check_exports() looks strange.
>It is actually related to my previous patch submission.
>
>See this patch:
>https://lore.kernel.org/patchwork/patch/1131970/
>
>
>Currently, the NULL pointer means no namespace.
>
>I noticed passing an empty string as the namespace
>simplified <linux/export.h> a lot.
>
>So, I changed it in a way that
>an empty string also means no namespace.
>
>
>Anyway, Matthias took over the refactoring work.
>So, I am not sure if this change is still helpful...
Hm, I agree that the inconsistency (empty string vs. NULL) is a bit confusing.
I do not mind too much either way - but if we allow both NULL and the
empty string to represent "no namespace", perhaps we should at least
document this in a comment next to the namespace field in struct
symbol and also next to the check in check_exports() so that it's
clear that both can mean no namespace.
>If it is no longer useful, I am happy to send v3
>with the following:
>
>
>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>index 442d5e2ad688..bdd3956e89c9 100644
>--- a/scripts/mod/modpost.c
>+++ b/scripts/mod/modpost.c
>@@ -2453,6 +2453,8 @@ static void read_dump(const char *fname,
>unsigned int kernel)
> mod = new_module(modname);
> mod->skip = 1;
> }
>+ if (namespace[0] == '\0')
>+ namespace = NULL;
> s = sym_add_exported(symname, namespace, mod,
> export_no(export));
> s->kernel = kernel;
>
+++ Masahiro Yamada [03/10/19 16:58 +0900]:
>
>I was hit by some problems caused by the module namespace feature
>that was merged recently. At least, the breakage of
>external module builds is a fatal one. I just took a look at the code
>closer, and I noticed some more issues (some are nit-picking).
>
>V2:
> - I dropped "module: avoid code duplication in include/linux/export.h"
> because Matthias offered to refactor the code by himself.
>
> - V1 missed the problem when a symbol is preloaded before
> sym_add_exported() is called. I fixed it too.
Hi Masahiro!
Thanks for the v2. I've queued this up in the module tree with the
intention of getting the fixes in for -rc3.
Matthias is working on some modpost fixes that would get rid of the
__ksymtab_<symbol>.<ns>/__ksymtab_<ns>.<symbol> naming scheme
altogether in favor of just getting the namespace string from
__kstrtabns and __ksymtab_strings -- this may render patch 1
unnecessary. But since we want to fix this asap, we can just keep it
and apply Matthias's fix on top later.
Thanks!
Jessica