[PATCH v2 0/6] module: various bug-fixes and clean-ups for module namespace

[PATCH v2 0/6] module: various bug-fixes and clean-ups for module namespace

[Masahiro Yamada]

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

[PATCH v2 1/6] module: swap the order of symbol.namespace

[Masahiro Yamada]

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 <yamada.masahiro@socionext.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
---

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

[PATCH v2 2/6] modpost: fix broken sym->namespace for external module builds

[Masahiro Yamada]

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 <yamada.masahiro@socionext.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
---

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

Re: [PATCH v2 2/6] modpost: fix broken sym->namespace for external module builds

[Shaun Ruffell]

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 <yamada.masahiro@socionext.com>
> Reviewed-by: Matthias Maennich <maennich@google.com>
> ---
> 
> 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 <sruffell@sruffell.net>
  or
Tested-by: Shaun Ruffell <sruffell@sruffell.net>

Re: [PATCH v2 2/6] modpost: fix broken sym->namespace for external module builds

[Masahiro Yamada]

Hi Shaun,

On Thu, Oct 3, 2019 at 10:29 PM Shaun Ruffell <sruffell@sruffell.net> 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 <yamada.masahiro@socionext.com>
> > Reviewed-by: Matthias Maennich <maennich@google.com>
> > ---
> >
> > 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

Re: [PATCH v2 2/6] modpost: fix broken sym->namespace for external module builds

[Jessica Yu]

+++ Masahiro Yamada [03/10/19 23:42 +0900]:
>Hi Shaun,
>
>On Thu, Oct 3, 2019 at 10:29 PM Shaun Ruffell <sruffell@sruffell.net> 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 <yamada.masahiro@socionext.com>
>> > Reviewed-by: Matthias Maennich <maennich@google.com>
>> > ---
>> >
>> > 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;
>

[PATCH v2 3/6] module: rename __kstrtab_ns_* to __kstrtabns_* to avoid symbol conflict

[Masahiro Yamada]

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 <yamada.masahiro@socionext.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
---

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

[PATCH v2 4/6] kbuild: fix build error of 'make nsdeps' in clean tree

[Masahiro Yamada]

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 <yamada.masahiro@socionext.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
---

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

[PATCH v2 5/6] nsdeps: fix hashbang of scripts/nsdeps

[Masahiro Yamada]

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 <yamada.masahiro@socionext.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
---

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

[PATCH v2 6/6] nsdeps: make generated patches independent of locale

[Masahiro Yamada]

scripts/nsdeps automatically generates a patch to add MODULE_IMPORT_NS
tags, and what is nicer, it sorts the lines alphabetically with the
'sort' command. However, the output from the 'sort' command depends on
locale.

For example, I got this:

$ { echo usbstorage; echo usb_storage; } | LANG=en_US.UTF-8 sort
usbstorage
usb_storage
$ { echo usbstorage; echo usb_storage; } | LANG=C sort
usb_storage
usbstorage

So, this means people might potentially send different patches.

This kind of issue was reported in the past, for example,
commit f55f2328bb28 ("kbuild: make sorting initramfs contents
independent of locale").

Adding 'LANG=C' is a conventional way of fixing when a deterministic
result is desirable.

I added 'LANG=C' very close to the 'sort' command since changing
locale affects the language of error messages etc. We should respect
users' choice as much as possible.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Reviewed-by: Matthias Maennich <maennich@google.com>
---

Changes in v2:
  - Add more commit log about the locale impact

 scripts/nsdeps | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/nsdeps b/scripts/nsdeps
index 964b7fb8c546..3754dac13b31 100644
--- a/scripts/nsdeps
+++ b/scripts/nsdeps
@@ -41,7 +41,7 @@ generate_deps() {
 		for source_file in $mod_source_files; do
 			sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp
 			offset=$(wc -l ${source_file}.tmp | awk '{print $1;}')
-			cat $source_file | grep MODULE_IMPORT_NS | sort -u >> ${source_file}.tmp
+			cat $source_file | grep MODULE_IMPORT_NS | LANG=C sort -u >> ${source_file}.tmp
 			tail -n +$((offset +1)) ${source_file} | grep -v MODULE_IMPORT_NS >> ${source_file}.tmp
 			if ! diff -q ${source_file} ${source_file}.tmp; then
 				mv ${source_file}.tmp ${source_file}
-- 
2.17.1

Re: [PATCH v2 0/6] module: various bug-fixes and clean-ups for module namespace

[Jessica Yu]

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