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 and improvements.
I hope these patches are mostly OK.
The 4th patch might have room for argument since it is a trade-off
of "cleaner implermentation" vs "code size".
Masahiro Yamada (7):
modpost: fix broken sym->namespace for external module builds
module: swap the order of symbol.namespace
module: rename __kstrtab_ns_* to __kstrtabns_* to avoid symbol
conflict
module: avoid code duplication in include/linux/export.h
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 | 104 ++++++++++++++---------------------------
kernel/module.c | 2 +-
scripts/mod/modpost.c | 20 ++++----
scripts/nsdeps | 4 +-
5 files changed, 47 insertions(+), 85 deletions(-)
--
2.17.1
include/linux/export.h has lots of code duplication between
EXPORT_SYMBOL and EXPORT_SYMBOL_NS.
To improve the maintainability and readability, unify the
implementation.
When the symbol has no namespace, pass the empty string "" to
the 'ns' parameter.
The drawback of this change is, it grows the code size.
When the symbol has no namespace, sym->namespace was previously
NULL, but it is now am empty string "". So, it increases 1 byte
for every no namespace EXPORT_SYMBOL.
A typical kernel configuration has 10K exported symbols, so it
increases 10KB in rough estimation.
I did not come up with a good idea to refactor it without increasing
the code size.
I am not sure how big a deal it is, but at least include/linux/export.h
looks nicer.
Signed-off-by: Masahiro Yamada <[email protected]>
---
include/linux/export.h | 100 +++++++++++++----------------------------
kernel/module.c | 2 +-
2 files changed, 33 insertions(+), 69 deletions(-)
diff --git a/include/linux/export.h b/include/linux/export.h
index 621158ecd2e2..55245a405a2f 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -48,45 +48,28 @@ extern struct module __this_module;
* absolute relocations that require runtime processing on relocatable
* kernels.
*/
-#define __KSYMTAB_ENTRY_NS(sym, sec, ns) \
+#define __KSYMTAB_ENTRY(sym, sec, ns) \
__ADDRESSABLE(sym) \
asm(" .section \"___ksymtab" sec "+" #sym "\", \"a\" \n" \
" .balign 4 \n" \
- "__ksymtab_" #ns NS_SEPARATOR #sym ": \n" \
+ "__ksymtab_" ns NS_SEPARATOR #sym ": \n" \
" .long " #sym "- . \n" \
" .long __kstrtab_" #sym "- . \n" \
" .long __kstrtabns_" #sym "- . \n" \
" .previous \n")
-#define __KSYMTAB_ENTRY(sym, sec) \
- __ADDRESSABLE(sym) \
- asm(" .section \"___ksymtab" sec "+" #sym "\", \"a\" \n" \
- " .balign 4 \n" \
- "__ksymtab_" #sym ": \n" \
- " .long " #sym "- . \n" \
- " .long __kstrtab_" #sym "- . \n" \
- " .long 0 \n" \
- " .previous \n")
-
struct kernel_symbol {
int value_offset;
int name_offset;
int namespace_offset;
};
#else
-#define __KSYMTAB_ENTRY_NS(sym, sec, ns) \
- static const struct kernel_symbol __ksymtab_##sym##__##ns \
- asm("__ksymtab_" #ns NS_SEPARATOR #sym) \
- __attribute__((section("___ksymtab" sec "+" #sym), used)) \
- __aligned(sizeof(void *)) \
- = { (unsigned long)&sym, __kstrtab_##sym, __kstrtabns_##sym }
-
-#define __KSYMTAB_ENTRY(sym, sec) \
+#define __KSYMTAB_ENTRY(sym, sec, ns) \
static const struct kernel_symbol __ksymtab_##sym \
- asm("__ksymtab_" #sym) \
+ asm("__ksymtab_" ns NS_SEPARATOR #sym) \
__attribute__((section("___ksymtab" sec "+" #sym), used)) \
__aligned(sizeof(void *)) \
- = { (unsigned long)&sym, __kstrtab_##sym, NULL }
+ = { (unsigned long)&sym, __kstrtab_##sym, __kstrtabns_##sym }
struct kernel_symbol {
unsigned long value;
@@ -97,29 +80,21 @@ struct kernel_symbol {
#ifdef __GENKSYMS__
-#define ___EXPORT_SYMBOL(sym,sec) __GENKSYMS_EXPORT_SYMBOL(sym)
-#define ___EXPORT_SYMBOL_NS(sym,sec,ns) __GENKSYMS_EXPORT_SYMBOL(sym)
+#define ___EXPORT_SYMBOL(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
#else
-#define ___export_symbol_common(sym, sec) \
+/* For every exported symbol, place a struct in the __ksymtab section */
+#define ___EXPORT_SYMBOL(sym, sec, ns) \
extern typeof(sym) sym; \
__CRC_SYMBOL(sym, sec); \
static const char __kstrtab_##sym[] \
__attribute__((section("__ksymtab_strings"), used, aligned(1))) \
- = #sym \
-
-/* For every exported symbol, place a struct in the __ksymtab section */
-#define ___EXPORT_SYMBOL_NS(sym, sec, ns) \
- ___export_symbol_common(sym, sec); \
+ = #sym; \
static const char __kstrtabns_##sym[] \
__attribute__((section("__ksymtab_strings"), used, aligned(1))) \
- = #ns; \
- __KSYMTAB_ENTRY_NS(sym, sec, ns)
-
-#define ___EXPORT_SYMBOL(sym, sec) \
- ___export_symbol_common(sym, sec); \
- __KSYMTAB_ENTRY(sym, sec)
+ = ns; \
+ __KSYMTAB_ENTRY(sym, sec, ns)
#endif
@@ -130,8 +105,7 @@ struct kernel_symbol {
* be reused in other execution contexts such as the UEFI stub or the
* decompressor.
*/
-#define __EXPORT_SYMBOL_NS(sym, sec, ns)
-#define __EXPORT_SYMBOL(sym, sec)
+#define __EXPORT_SYMBOL(sym, sec, ns)
#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
@@ -147,48 +121,38 @@ struct kernel_symbol {
#define __ksym_marker(sym) \
static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
-#define __EXPORT_SYMBOL(sym, sec) \
+#define __EXPORT_SYMBOL(sym, sec, ns) \
__ksym_marker(sym); \
- __cond_export_sym(sym, sec, __is_defined(__KSYM_##sym))
-#define __cond_export_sym(sym, sec, conf) \
- ___cond_export_sym(sym, sec, conf)
-#define ___cond_export_sym(sym, sec, enabled) \
- __cond_export_sym_##enabled(sym, sec)
-#define __cond_export_sym_1(sym, sec) ___EXPORT_SYMBOL(sym, sec)
-#define __cond_export_sym_0(sym, sec) /* nothing */
-
-#define __EXPORT_SYMBOL_NS(sym, sec, ns) \
- __ksym_marker(sym); \
- __cond_export_ns_sym(sym, sec, ns, __is_defined(__KSYM_##sym))
-#define __cond_export_ns_sym(sym, sec, ns, conf) \
- ___cond_export_ns_sym(sym, sec, ns, conf)
-#define ___cond_export_ns_sym(sym, sec, ns, enabled) \
- __cond_export_ns_sym_##enabled(sym, sec, ns)
-#define __cond_export_ns_sym_1(sym, sec, ns) ___EXPORT_SYMBOL_NS(sym, sec, ns)
-#define __cond_export_ns_sym_0(sym, sec, ns) /* nothing */
+ __cond_export_sym(sym, sec, ns, __is_defined(__KSYM_##sym))
+#define __cond_export_sym(sym, sec, ns, conf) \
+ ___cond_export_sym(sym, sec, ns, conf)
+#define ___cond_export_sym(sym, sec, ns, enabled) \
+ __cond_export_sym_##enabled(sym, sec, ns)
+#define __cond_export_sym_1(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
+#define __cond_export_sym_0(sym, sec, ns) /* nothing */
#else
-#define __EXPORT_SYMBOL_NS(sym,sec,ns) ___EXPORT_SYMBOL_NS(sym,sec,ns)
-#define __EXPORT_SYMBOL(sym,sec) ___EXPORT_SYMBOL(sym,sec)
+#define __EXPORT_SYMBOL(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
#endif /* CONFIG_MODULES */
#ifdef DEFAULT_SYMBOL_NAMESPACE
-#undef __EXPORT_SYMBOL
-#define __EXPORT_SYMBOL(sym, sec) \
- __EXPORT_SYMBOL_NS(sym, sec, DEFAULT_SYMBOL_NAMESPACE)
+#include <linux/stringify.h>
+#define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, __stringify(DEFAULT_SYMBOL_NAMESPACE))
+#else
+#define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, "")
#endif
-#define EXPORT_SYMBOL(sym) __EXPORT_SYMBOL(sym, "")
-#define EXPORT_SYMBOL_GPL(sym) __EXPORT_SYMBOL(sym, "_gpl")
-#define EXPORT_SYMBOL_GPL_FUTURE(sym) __EXPORT_SYMBOL(sym, "_gpl_future")
-#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL_NS(sym, "", ns)
-#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL_NS(sym, "_gpl", ns)
+#define EXPORT_SYMBOL(sym) _EXPORT_SYMBOL(sym, "")
+#define EXPORT_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym, "_gpl")
+#define EXPORT_SYMBOL_GPL_FUTURE(sym) _EXPORT_SYMBOL(sym, "_gpl_future")
+#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL(sym, "", #ns)
+#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL(sym, "_gpl", #ns)
#ifdef CONFIG_UNUSED_SYMBOLS
-#define EXPORT_UNUSED_SYMBOL(sym) __EXPORT_SYMBOL(sym, "_unused")
-#define EXPORT_UNUSED_SYMBOL_GPL(sym) __EXPORT_SYMBOL(sym, "_unused_gpl")
+#define EXPORT_UNUSED_SYMBOL(sym) _EXPORT_SYMBOL(sym, "_unused")
+#define EXPORT_UNUSED_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym, "_unused_gpl")
#else
#define EXPORT_UNUSED_SYMBOL(sym)
#define EXPORT_UNUSED_SYMBOL_GPL(sym)
diff --git a/kernel/module.c b/kernel/module.c
index 32873bcce738..73f69ff86db5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1399,7 +1399,7 @@ static int verify_namespace_is_imported(const struct load_info *info,
char *imported_namespace;
namespace = kernel_symbol_namespace(sym);
- if (namespace) {
+ if (namespace && namespace[0]) {
imported_namespace = get_modinfo(info, "import_ns");
while (imported_namespace) {
if (strcmp(namespace, imported_namespace) == 0)
--
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, if you like,
you can free it with complicated code like this:
free(sym->namespace - strlen(sym->name) - 1);
I would not say it is fatal since we seldom bother to manually free
memory in host programs. But, I can fix it in an elegant way.
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]>
---
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 5c628a7d80f7..d171b0cffb05 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
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.
Especially when namespaces contain underscores, the result is
different depending on the locale.
For example, I got this:
$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort
usbcommon
usb_common
$ { echo usbcommon; echo usb_common; } | LANG=C sort
usb_common
usbcommon
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.
Signed-off-by: Masahiro Yamada <[email protected]>
---
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
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]>
---
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
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() correctly 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], I added NOFAIL(strdup(...)) to allocate memory.
For [2], I changed the if-conditional in check_exports().
Signed-off-by: Masahiro Yamada <[email protected]>
---
scripts/mod/modpost.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 3961941e8e7a..5c628a7d80f7 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2195,7 +2195,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);
@@ -2453,7 +2453,7 @@ static void read_dump(const char *fname, unsigned int kernel)
mod = new_module(modname);
mod->skip = 1;
}
- s = sym_add_exported(symname, namespace, mod,
+ s = sym_add_exported(symname, NOFAIL(strdup(namespace)), mod,
export_no(export));
s->kernel = kernel;
s->preloaded = 1;
--
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 staring
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 duplication for "__kstrtab_ns_capable".
To ensure the uniqueness, rename "__kstrtab_ns_*" to "__kstrtabns_*".
Signed-off-by: Masahiro Yamada <[email protected]>
---
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
On Fri, Sep 27, 2019 at 6:37 PM Masahiro Yamada
<[email protected]> 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() correctly 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], I added NOFAIL(strdup(...)) to allocate memory.
> For [2], I changed the if-conditional in check_exports().
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
--
Best Regards
Masahiro Yamada
On Fri, Sep 27, 2019 at 6:37 PM Masahiro Yamada
<[email protected]> wrote:
>
> include/linux/export.h has lots of code duplication between
> EXPORT_SYMBOL and EXPORT_SYMBOL_NS.
>
> To improve the maintainability and readability, unify the
> implementation.
>
> When the symbol has no namespace, pass the empty string "" to
> the 'ns' parameter.
>
> The drawback of this change is, it grows the code size.
> When the symbol has no namespace, sym->namespace was previously
> NULL, but it is now am empty string "". So, it increases 1 byte
Just a nit.
I meant "an empty string" instead of "am empty string".
> for every no namespace EXPORT_SYMBOL.
>
> A typical kernel configuration has 10K exported symbols, so it
> increases 10KB in rough estimation.
>
> I did not come up with a good idea to refactor it without increasing
> the code size.
>
> I am not sure how big a deal it is, but at least include/linux/export.h
> looks nicer.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> include/linux/export.h | 100 +++++++++++++----------------------------
> kernel/module.c | 2 +-
> 2 files changed, 33 insertions(+), 69 deletions(-)
>
> diff --git a/include/linux/export.h b/include/linux/export.h
> index 621158ecd2e2..55245a405a2f 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -48,45 +48,28 @@ extern struct module __this_module;
> * absolute relocations that require runtime processing on relocatable
> * kernels.
> */
> -#define __KSYMTAB_ENTRY_NS(sym, sec, ns) \
> +#define __KSYMTAB_ENTRY(sym, sec, ns) \
> __ADDRESSABLE(sym) \
> asm(" .section \"___ksymtab" sec "+" #sym "\", \"a\" \n" \
> " .balign 4 \n" \
> - "__ksymtab_" #ns NS_SEPARATOR #sym ": \n" \
> + "__ksymtab_" ns NS_SEPARATOR #sym ": \n" \
> " .long " #sym "- . \n" \
> " .long __kstrtab_" #sym "- . \n" \
> " .long __kstrtabns_" #sym "- . \n" \
> " .previous \n")
>
> -#define __KSYMTAB_ENTRY(sym, sec) \
> - __ADDRESSABLE(sym) \
> - asm(" .section \"___ksymtab" sec "+" #sym "\", \"a\" \n" \
> - " .balign 4 \n" \
> - "__ksymtab_" #sym ": \n" \
> - " .long " #sym "- . \n" \
> - " .long __kstrtab_" #sym "- . \n" \
> - " .long 0 \n" \
> - " .previous \n")
> -
> struct kernel_symbol {
> int value_offset;
> int name_offset;
> int namespace_offset;
> };
> #else
> -#define __KSYMTAB_ENTRY_NS(sym, sec, ns) \
> - static const struct kernel_symbol __ksymtab_##sym##__##ns \
> - asm("__ksymtab_" #ns NS_SEPARATOR #sym) \
> - __attribute__((section("___ksymtab" sec "+" #sym), used)) \
> - __aligned(sizeof(void *)) \
> - = { (unsigned long)&sym, __kstrtab_##sym, __kstrtabns_##sym }
> -
> -#define __KSYMTAB_ENTRY(sym, sec) \
> +#define __KSYMTAB_ENTRY(sym, sec, ns) \
> static const struct kernel_symbol __ksymtab_##sym \
> - asm("__ksymtab_" #sym) \
> + asm("__ksymtab_" ns NS_SEPARATOR #sym) \
> __attribute__((section("___ksymtab" sec "+" #sym), used)) \
> __aligned(sizeof(void *)) \
> - = { (unsigned long)&sym, __kstrtab_##sym, NULL }
> + = { (unsigned long)&sym, __kstrtab_##sym, __kstrtabns_##sym }
>
> struct kernel_symbol {
> unsigned long value;
> @@ -97,29 +80,21 @@ struct kernel_symbol {
>
> #ifdef __GENKSYMS__
>
> -#define ___EXPORT_SYMBOL(sym,sec) __GENKSYMS_EXPORT_SYMBOL(sym)
> -#define ___EXPORT_SYMBOL_NS(sym,sec,ns) __GENKSYMS_EXPORT_SYMBOL(sym)
> +#define ___EXPORT_SYMBOL(sym, sec, ns) __GENKSYMS_EXPORT_SYMBOL(sym)
>
> #else
>
> -#define ___export_symbol_common(sym, sec) \
> +/* For every exported symbol, place a struct in the __ksymtab section */
> +#define ___EXPORT_SYMBOL(sym, sec, ns) \
> extern typeof(sym) sym; \
> __CRC_SYMBOL(sym, sec); \
> static const char __kstrtab_##sym[] \
> __attribute__((section("__ksymtab_strings"), used, aligned(1))) \
> - = #sym \
> -
> -/* For every exported symbol, place a struct in the __ksymtab section */
> -#define ___EXPORT_SYMBOL_NS(sym, sec, ns) \
> - ___export_symbol_common(sym, sec); \
> + = #sym; \
> static const char __kstrtabns_##sym[] \
> __attribute__((section("__ksymtab_strings"), used, aligned(1))) \
> - = #ns; \
> - __KSYMTAB_ENTRY_NS(sym, sec, ns)
> -
> -#define ___EXPORT_SYMBOL(sym, sec) \
> - ___export_symbol_common(sym, sec); \
> - __KSYMTAB_ENTRY(sym, sec)
> + = ns; \
> + __KSYMTAB_ENTRY(sym, sec, ns)
>
> #endif
>
> @@ -130,8 +105,7 @@ struct kernel_symbol {
> * be reused in other execution contexts such as the UEFI stub or the
> * decompressor.
> */
> -#define __EXPORT_SYMBOL_NS(sym, sec, ns)
> -#define __EXPORT_SYMBOL(sym, sec)
> +#define __EXPORT_SYMBOL(sym, sec, ns)
>
> #elif defined(CONFIG_TRIM_UNUSED_KSYMS)
>
> @@ -147,48 +121,38 @@ struct kernel_symbol {
> #define __ksym_marker(sym) \
> static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
>
> -#define __EXPORT_SYMBOL(sym, sec) \
> +#define __EXPORT_SYMBOL(sym, sec, ns) \
> __ksym_marker(sym); \
> - __cond_export_sym(sym, sec, __is_defined(__KSYM_##sym))
> -#define __cond_export_sym(sym, sec, conf) \
> - ___cond_export_sym(sym, sec, conf)
> -#define ___cond_export_sym(sym, sec, enabled) \
> - __cond_export_sym_##enabled(sym, sec)
> -#define __cond_export_sym_1(sym, sec) ___EXPORT_SYMBOL(sym, sec)
> -#define __cond_export_sym_0(sym, sec) /* nothing */
> -
> -#define __EXPORT_SYMBOL_NS(sym, sec, ns) \
> - __ksym_marker(sym); \
> - __cond_export_ns_sym(sym, sec, ns, __is_defined(__KSYM_##sym))
> -#define __cond_export_ns_sym(sym, sec, ns, conf) \
> - ___cond_export_ns_sym(sym, sec, ns, conf)
> -#define ___cond_export_ns_sym(sym, sec, ns, enabled) \
> - __cond_export_ns_sym_##enabled(sym, sec, ns)
> -#define __cond_export_ns_sym_1(sym, sec, ns) ___EXPORT_SYMBOL_NS(sym, sec, ns)
> -#define __cond_export_ns_sym_0(sym, sec, ns) /* nothing */
> + __cond_export_sym(sym, sec, ns, __is_defined(__KSYM_##sym))
> +#define __cond_export_sym(sym, sec, ns, conf) \
> + ___cond_export_sym(sym, sec, ns, conf)
> +#define ___cond_export_sym(sym, sec, ns, enabled) \
> + __cond_export_sym_##enabled(sym, sec, ns)
> +#define __cond_export_sym_1(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
> +#define __cond_export_sym_0(sym, sec, ns) /* nothing */
>
> #else
>
> -#define __EXPORT_SYMBOL_NS(sym,sec,ns) ___EXPORT_SYMBOL_NS(sym,sec,ns)
> -#define __EXPORT_SYMBOL(sym,sec) ___EXPORT_SYMBOL(sym,sec)
> +#define __EXPORT_SYMBOL(sym, sec, ns) ___EXPORT_SYMBOL(sym, sec, ns)
>
> #endif /* CONFIG_MODULES */
>
> #ifdef DEFAULT_SYMBOL_NAMESPACE
> -#undef __EXPORT_SYMBOL
> -#define __EXPORT_SYMBOL(sym, sec) \
> - __EXPORT_SYMBOL_NS(sym, sec, DEFAULT_SYMBOL_NAMESPACE)
> +#include <linux/stringify.h>
> +#define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, __stringify(DEFAULT_SYMBOL_NAMESPACE))
> +#else
> +#define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, "")
> #endif
>
> -#define EXPORT_SYMBOL(sym) __EXPORT_SYMBOL(sym, "")
> -#define EXPORT_SYMBOL_GPL(sym) __EXPORT_SYMBOL(sym, "_gpl")
> -#define EXPORT_SYMBOL_GPL_FUTURE(sym) __EXPORT_SYMBOL(sym, "_gpl_future")
> -#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL_NS(sym, "", ns)
> -#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL_NS(sym, "_gpl", ns)
> +#define EXPORT_SYMBOL(sym) _EXPORT_SYMBOL(sym, "")
> +#define EXPORT_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym, "_gpl")
> +#define EXPORT_SYMBOL_GPL_FUTURE(sym) _EXPORT_SYMBOL(sym, "_gpl_future")
> +#define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL(sym, "", #ns)
> +#define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL(sym, "_gpl", #ns)
>
> #ifdef CONFIG_UNUSED_SYMBOLS
> -#define EXPORT_UNUSED_SYMBOL(sym) __EXPORT_SYMBOL(sym, "_unused")
> -#define EXPORT_UNUSED_SYMBOL_GPL(sym) __EXPORT_SYMBOL(sym, "_unused_gpl")
> +#define EXPORT_UNUSED_SYMBOL(sym) _EXPORT_SYMBOL(sym, "_unused")
> +#define EXPORT_UNUSED_SYMBOL_GPL(sym) _EXPORT_SYMBOL(sym, "_unused_gpl")
> #else
> #define EXPORT_UNUSED_SYMBOL(sym)
> #define EXPORT_UNUSED_SYMBOL_GPL(sym)
> diff --git a/kernel/module.c b/kernel/module.c
> index 32873bcce738..73f69ff86db5 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1399,7 +1399,7 @@ static int verify_namespace_is_imported(const struct load_info *info,
> char *imported_namespace;
>
> namespace = kernel_symbol_namespace(sym);
> - if (namespace) {
> + if (namespace && namespace[0]) {
> imported_namespace = get_modinfo(info, "import_ns");
> while (imported_namespace) {
> if (strcmp(namespace, imported_namespace) == 0)
> --
> 2.17.1
>
--
Best Regards
Masahiro Yamada
On 27/09/2019 11.36, Masahiro Yamada wrote:
> include/linux/export.h has lots of code duplication between
> EXPORT_SYMBOL and EXPORT_SYMBOL_NS.
>
> To improve the maintainability and readability, unify the
> implementation.
>
> When the symbol has no namespace, pass the empty string "" to
> the 'ns' parameter.
>
> The drawback of this change is, it grows the code size.
> When the symbol has no namespace, sym->namespace was previously
> NULL, but it is now am empty string "". So, it increases 1 byte
> for every no namespace EXPORT_SYMBOL.
>
> A typical kernel configuration has 10K exported symbols, so it
> increases 10KB in rough estimation.
>
> I did not come up with a good idea to refactor it without increasing
> the code size.
Can't we put the "aMS" flags on the __ksymtab_strings section? That
would make the empty strings free, and would also deduplicate the
USB_STORAGE string. And while almost per definition we don't have exact
duplicates among the names of exported symbols, we might have both a foo
and __foo, so that could save even more.
I don't know if we have it already, but we'd need each arch to tell us
what symbol to use for @ in @progbits (e.g. % for arm). It seems most
are fine with @, so maybe a generic version could be
#ifndef ARCH_SECTION_TYPE_CHAR
#define ARCH_SECTION_TYPE_CHAR "@"
#endif
and then it would be
section("__ksymtab_strings,\"aMS\","ARCH_SECTION_TYPE_CHAR"progbits,1")
But I don't know if any tooling relies on the strings not being
deduplicated.
Rasmus
On Fri, Sep 27, 2019 at 06:35:57PM +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() correctly 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], I added NOFAIL(strdup(...)) to allocate memory.
>For [2], I changed the if-conditional in check_exports().
Thanks for addressing this. Greg had reported this earlier this week and
Shaun was proposing a fix earlier today. Shaun's fix also ensures that
memory is released when updating the namespace. But judging from the
code around 'symbolhash' it seems that leaking this is accepted for
modpost. Not sure about that. Having said that, please feel free to add
Reviewed-by: Matthias Maennich <[email protected]>
Cheers,
Matthias
>Signed-off-by: Masahiro Yamada <[email protected]>
>---
>
> scripts/mod/modpost.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>index 3961941e8e7a..5c628a7d80f7 100644
>--- a/scripts/mod/modpost.c
>+++ b/scripts/mod/modpost.c
>@@ -2195,7 +2195,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);
>
>@@ -2453,7 +2453,7 @@ static void read_dump(const char *fname, unsigned int kernel)
> mod = new_module(modname);
> mod->skip = 1;
> }
>- s = sym_add_exported(symname, namespace, mod,
>+ s = sym_add_exported(symname, NOFAIL(strdup(namespace)), mod,
> export_no(export));
> s->kernel = kernel;
> s->preloaded = 1;
>--
>2.17.1
>
On Fri, Sep 27, 2019 at 06:35:58PM +0900, Masahiro Yamada wrote:
>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, if you like,
>you can free it with complicated code like this:
>
> free(sym->namespace - strlen(sym->name) - 1);
>
>I would not say it is fatal since we seldom bother to manually free
>memory in host programs. But, I can fix it in an elegant way.
>
>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.
I agree with this rationale and like the implementation. I believe the
idea of appending the namespace came from being afraid of other tools
facing the problem of parsing the namespace out of the middle of the
entry. Thanks for this improvement.
Reviewed-by: Matthias Maennich <[email protected]>
Cheers,
Matthias
>Signed-off-by: Masahiro Yamada <[email protected]>
>---
>
> 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 5c628a7d80f7..d171b0cffb05 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
>
On Fri, Sep 27, 2019 at 06:35:59PM +0900, Masahiro Yamada wrote:
>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 staring
>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 duplication for "__kstrtab_ns_capable".
>
>To ensure the uniqueness, rename "__kstrtab_ns_*" to "__kstrtabns_*".
Again, thanks for catching this!
Reviewed-by: Matthias Maennich <[email protected]>
Cheers,
Matthias
>Signed-off-by: Masahiro Yamada <[email protected]>
>---
>
> 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
>
On Fri, Sep 27, 2019 at 01:07:33PM +0200, Rasmus Villemoes wrote:
>On 27/09/2019 11.36, Masahiro Yamada wrote:
>> include/linux/export.h has lots of code duplication between
>> EXPORT_SYMBOL and EXPORT_SYMBOL_NS.
>>
>> To improve the maintainability and readability, unify the
>> implementation.
>>
>> When the symbol has no namespace, pass the empty string "" to
>> the 'ns' parameter.
>>
>> The drawback of this change is, it grows the code size.
>> When the symbol has no namespace, sym->namespace was previously
>> NULL, but it is now am empty string "". So, it increases 1 byte
>> for every no namespace EXPORT_SYMBOL.
>>
>> A typical kernel configuration has 10K exported symbols, so it
>> increases 10KB in rough estimation.
>>
>> I did not come up with a good idea to refactor it without increasing
>> the code size.
>
>Can't we put the "aMS" flags on the __ksymtab_strings section? That
>would make the empty strings free, and would also deduplicate the
>USB_STORAGE string. And while almost per definition we don't have exact
>duplicates among the names of exported symbols, we might have both a foo
>and __foo, so that could save even more.
I was not aware of this possibility and I was a bit bothered that I was
not able to deduplicate the namespace strings in the PREL case. So, at
least I tried to avoid having the redundant empty strings in it. If this
approach solves the deduplication problem _and_ reduces the complexity
of the code, I am very much for it. I will only be able to look into
this next week.
>I don't know if we have it already, but we'd need each arch to tell us
>what symbol to use for @ in @progbits (e.g. % for arm). It seems most
>are fine with @, so maybe a generic version could be
>
>#ifndef ARCH_SECTION_TYPE_CHAR
>#define ARCH_SECTION_TYPE_CHAR "@"
>#endif
>
>and then it would be
>section("__ksymtab_strings,\"aMS\","ARCH_SECTION_TYPE_CHAR"progbits,1")
>
>But I don't know if any tooling relies on the strings not being
>deduplicated.
Matthias
Cheers,
>Rasmus
On Fri, Sep 27, 2019 at 06:36:02PM +0900, Masahiro Yamada wrote:
>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.
Reviewed-by: Matthias Maennich <[email protected]>
Cheers,
Matthias
>Signed-off-by: Masahiro Yamada <[email protected]>
>---
>
> 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 Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote:
>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.
>
>Especially when namespaces contain underscores, the result is
>different depending on the locale.
>
>For example, I got this:
>
>$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort
>usbcommon
>usb_common
>$ { echo usbcommon; echo usb_common; } | LANG=C sort
>usb_common
>usbcommon
>
>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.
>
>Signed-off-by: Masahiro Yamada <[email protected]>
>---
>
> 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
I would prefer to have this set throughout the whole runtime of the
script. Otherwise we likely see a followup patch. So, either as an
export at the beginning of this file or as part of the command that
calls this script.
With this
Reviewed-by: Matthias Maennich <[email protected]>
Cheers,
Matthias
> 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
>
On Fri, Sep 27, 2019 at 06:35:56PM +0900, Masahiro Yamada wrote:
>
>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 and improvements.
>
>I hope these patches are mostly OK.
>The 4th patch might have room for argument since it is a trade-off
>of "cleaner implermentation" vs "code size".
>
Thanks Masahiro for taking the time to improve the implementation of the
symbol namespaces. These are all good points that you addressed!
For [04/07], I can work on this if you do not mind.
Cheers,
Matthias
>
>Masahiro Yamada (7):
> modpost: fix broken sym->namespace for external module builds
> module: swap the order of symbol.namespace
> module: rename __kstrtab_ns_* to __kstrtabns_* to avoid symbol
> conflict
> module: avoid code duplication in include/linux/export.h
> 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 | 104 ++++++++++++++---------------------------
> kernel/module.c | 2 +-
> scripts/mod/modpost.c | 20 ++++----
> scripts/nsdeps | 4 +-
> 5 files changed, 47 insertions(+), 85 deletions(-)
>
>--
>2.17.1
>
On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <[email protected]> wrote:
>
> On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote:
> >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.
> >
> >Especially when namespaces contain underscores, the result is
> >different depending on the locale.
> >
> >For example, I got this:
> >
> >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort
> >usbcommon
> >usb_common
> >$ { echo usbcommon; echo usb_common; } | LANG=C sort
> >usb_common
> >usbcommon
> >
> >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.
> >
> >Signed-off-by: Masahiro Yamada <[email protected]>
> >---
> >
> > 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
>
> I would prefer to have this set throughout the whole runtime of the
> script. Otherwise we likely see a followup patch. So, either as an
> export at the beginning of this file or as part of the command that
> calls this script.
I prefer to keep it close to the locale-dependent code.
If I move it to somewhere else, I need to add a comment like
# make "sort" command deterministic
export LANG=C
Otherwise, people would have no idea why it is needed.
> With this
>
> Reviewed-by: Matthias Maennich <[email protected]>
>
> Cheers,
> Matthias
>
> > 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
> >
--
Best Regards
Masahiro Yamada
Hi Matthias,
On Fri, Sep 27, 2019 at 10:41 PM Matthias Maennich <[email protected]> wrote:
>
> On Fri, Sep 27, 2019 at 06:35:56PM +0900, Masahiro Yamada wrote:
> >
> >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 and improvements.
> >
> >I hope these patches are mostly OK.
> >The 4th patch might have room for argument since it is a trade-off
> >of "cleaner implermentation" vs "code size".
> >
> Thanks Masahiro for taking the time to improve the implementation of the
> symbol namespaces. These are all good points that you addressed!
>
> For [04/07], I can work on this if you do not mind.
Please feel free to.
Thanks for your review.
--
Best Regards
Masahiro Yamada
On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote:
> On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <[email protected]> wrote:
> >
> > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote:
> > >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.
> > >
> > >Especially when namespaces contain underscores, the result is
> > >different depending on the locale.
> > >
> > >For example, I got this:
> > >
> > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort
> > >usbcommon
> > >usb_common
> > >$ { echo usbcommon; echo usb_common; } | LANG=C sort
> > >usb_common
> > >usbcommon
> > >
> > >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.
> > >
> > >Signed-off-by: Masahiro Yamada <[email protected]>
> > >---
> > >
> > > 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
> >
> > I would prefer to have this set throughout the whole runtime of the
> > script. Otherwise we likely see a followup patch. So, either as an
> > export at the beginning of this file or as part of the command that
> > calls this script.
>
>
> I prefer to keep it close to the locale-dependent code.
>
>
>
> If I move it to somewhere else, I need to add a comment like
>
> # make "sort" command deterministic
> export LANG=C
>
> Otherwise, people would have no idea why it is needed.
A comment is fine, it documents why it is here and it keeps anyone from
having to remember to add it to anything else that changes in here.
thanks,
greg k-h
On Sat, Sep 28, 2019 at 3:14 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote:
> > On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <[email protected]> wrote:
> > >
> > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote:
> > > >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.
> > > >
> > > >Especially when namespaces contain underscores, the result is
> > > >different depending on the locale.
> > > >
> > > >For example, I got this:
> > > >
> > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort
> > > >usbcommon
> > > >usb_common
> > > >$ { echo usbcommon; echo usb_common; } | LANG=C sort
> > > >usb_common
> > > >usbcommon
> > > >
> > > >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.
> > > >
> > > >Signed-off-by: Masahiro Yamada <[email protected]>
> > > >---
> > > >
> > > > 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
> > >
> > > I would prefer to have this set throughout the whole runtime of the
> > > script. Otherwise we likely see a followup patch. So, either as an
> > > export at the beginning of this file or as part of the command that
> > > calls this script.
> >
> >
> > I prefer to keep it close to the locale-dependent code.
> >
> >
> >
> > If I move it to somewhere else, I need to add a comment like
> >
> > # make "sort" command deterministic
> > export LANG=C
> >
> > Otherwise, people would have no idea why it is needed.
>
> A comment is fine, it documents why it is here and it keeps anyone from
> having to remember to add it to anything else that changes in here.
>
> thanks,
>
> greg k-h
Huh, people who live in a country with English as mother tongue
cannot understand the i18n because English is the
only language in the world?
For example, in my locale (ja_JP.UTF-8)
I can see messages in Japanese as follows:
$ sh scripts/nsdeps
cat: /modules.order: そのようなファイルやディレクトリはありません
If I added "LANG=C" to the whole of this script,
it would forcibly change all the messages into English.
$ sh scripts/nsdeps
cat: /modules.order: No such file or directory
The impact on the locale should be really limited.
So, "LANG=C" must be placed immediately before the "find" command.
Nobody is asking to change log messages into English,
and offering what was not asked is just *pushy*.
--
Best Regards
Masahiro Yamada
On Sun, Sep 29, 2019 at 10:18 AM Masahiro Yamada
<[email protected]> wrote:
>
> On Sat, Sep 28, 2019 at 3:14 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote:
> > > On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <[email protected]> wrote:
> > > >
> > > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote:
> > > > >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.
> > > > >
> > > > >Especially when namespaces contain underscores, the result is
> > > > >different depending on the locale.
> > > > >
> > > > >For example, I got this:
> > > > >
> > > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort
> > > > >usbcommon
> > > > >usb_common
> > > > >$ { echo usbcommon; echo usb_common; } | LANG=C sort
> > > > >usb_common
> > > > >usbcommon
> > > > >
> > > > >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.
> > > > >
> > > > >Signed-off-by: Masahiro Yamada <[email protected]>
> > > > >---
> > > > >
> > > > > 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
> > > >
> > > > I would prefer to have this set throughout the whole runtime of the
> > > > script. Otherwise we likely see a followup patch. So, either as an
> > > > export at the beginning of this file or as part of the command that
> > > > calls this script.
> > >
> > >
> > > I prefer to keep it close to the locale-dependent code.
> > >
> > >
> > >
> > > If I move it to somewhere else, I need to add a comment like
> > >
> > > # make "sort" command deterministic
> > > export LANG=C
> > >
> > > Otherwise, people would have no idea why it is needed.
> >
> > A comment is fine, it documents why it is here and it keeps anyone from
> > having to remember to add it to anything else that changes in here.
> >
> > thanks,
> >
> > greg k-h
>
>
> Huh, people who live in a country with English as mother tongue
> cannot understand the i18n because English is the
> only language in the world?
I take back this comment.
I actually do not know where you live, or what native language you speak.
It was used to exaggerate things, but It is not important to
the point of discussions. Sorry.
--
Best Regards
Masahiro Yamada
On Sun, Sep 29, 2019 at 10:18:50AM +0900, Masahiro Yamada wrote:
> On Sat, Sep 28, 2019 at 3:14 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote:
> > > On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <[email protected]> wrote:
> > > >
> > > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote:
> > > > >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.
> > > > >
> > > > >Especially when namespaces contain underscores, the result is
> > > > >different depending on the locale.
> > > > >
> > > > >For example, I got this:
> > > > >
> > > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort
> > > > >usbcommon
> > > > >usb_common
> > > > >$ { echo usbcommon; echo usb_common; } | LANG=C sort
> > > > >usb_common
> > > > >usbcommon
> > > > >
> > > > >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.
> > > > >
> > > > >Signed-off-by: Masahiro Yamada <[email protected]>
> > > > >---
> > > > >
> > > > > 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
> > > >
> > > > I would prefer to have this set throughout the whole runtime of the
> > > > script. Otherwise we likely see a followup patch. So, either as an
> > > > export at the beginning of this file or as part of the command that
> > > > calls this script.
> > >
> > >
> > > I prefer to keep it close to the locale-dependent code.
> > >
> > >
> > >
> > > If I move it to somewhere else, I need to add a comment like
> > >
> > > # make "sort" command deterministic
> > > export LANG=C
> > >
> > > Otherwise, people would have no idea why it is needed.
> >
> > A comment is fine, it documents why it is here and it keeps anyone from
> > having to remember to add it to anything else that changes in here.
> >
> > thanks,
> >
> > greg k-h
>
>
> Huh, people who live in a country with English as mother tongue
> cannot understand the i18n because English is the
> only language in the world?
Heh, I live in a country where English is not the "mother tongue" and I
totally missed this, sorry about that :(
> For example, in my locale (ja_JP.UTF-8)
>
> I can see messages in Japanese as follows:
>
> $ sh scripts/nsdeps
> cat: /modules.order: そのようなファイルやディレクトリはありません
Ugh, I forgot this would change the error messages from other programs.
You are right, your proposal isn't ok, I missed this given that I am
used to programming in English.
sorry,
greg k-h
On Sun, Sep 29, 2019 at 10:30:27AM +0900, Masahiro Yamada wrote:
>On Sun, Sep 29, 2019 at 10:18 AM Masahiro Yamada
><[email protected]> wrote:
>>
>> On Sat, Sep 28, 2019 at 3:14 AM Greg Kroah-Hartman
>> <[email protected]> wrote:
>> >
>> > On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote:
>> > > On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <[email protected]> wrote:
>> > > >
>> > > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote:
>> > > > >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.
>> > > > >
>> > > > >Especially when namespaces contain underscores, the result is
>> > > > >different depending on the locale.
>> > > > >
>> > > > >For example, I got this:
>> > > > >
>> > > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort
>> > > > >usbcommon
>> > > > >usb_common
>> > > > >$ { echo usbcommon; echo usb_common; } | LANG=C sort
>> > > > >usb_common
>> > > > >usbcommon
>> > > > >
>> > > > >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.
>> > > > >
>> > > > >Signed-off-by: Masahiro Yamada <[email protected]>
>> > > > >---
>> > > > >
>> > > > > 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
>> > > >
>> > > > I would prefer to have this set throughout the whole runtime of the
>> > > > script. Otherwise we likely see a followup patch. So, either as an
>> > > > export at the beginning of this file or as part of the command that
>> > > > calls this script.
>> > >
>> > >
>> > > I prefer to keep it close to the locale-dependent code.
>> > >
>> > >
>> > >
>> > > If I move it to somewhere else, I need to add a comment like
>> > >
>> > > # make "sort" command deterministic
>> > > export LANG=C
>> > >
>> > > Otherwise, people would have no idea why it is needed.
>> >
>> > A comment is fine, it documents why it is here and it keeps anyone from
>> > having to remember to add it to anything else that changes in here.
>> >
>> > thanks,
>> >
>> > greg k-h
>>
>>
>> Huh, people who live in a country with English as mother tongue
>> cannot understand the i18n because English is the
>> only language in the world?
>
>I take back this comment.
>I actually do not know where you live, or what native language you speak.
>It was used to exaggerate things, but It is not important to
>the point of discussions. Sorry.
Thanks for pointing this out and reminding us! I am not a native English
speaker, but often are surrounded by English, especially when
programming. Error messages in my native language feel often rather
funny than helpful, but I guess this not the case for most translations.
Based on that I am ok with your original version of the patch.
Cheers,
Matthias
+++ Matthias Maennich [27/09/19 14:41 +0100]:
>On Fri, Sep 27, 2019 at 06:35:56PM +0900, Masahiro Yamada wrote:
>>
>>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 and improvements.
>>
>>I hope these patches are mostly OK.
>>The 4th patch might have room for argument since it is a trade-off
>>of "cleaner implermentation" vs "code size".
>>
>Thanks Masahiro for taking the time to improve the implementation of the
>symbol namespaces. These are all good points that you addressed!
Agreed, thanks Masahiro for fixing up all the rough edges! Your series
of fixes look good to me, I will queue this up on modules-next this
week with the exception of patch 4 - Matthias, you are planning to
submit a patch that would supercede patch 04/07, right?
Thanks!
Jessica
On Wed, Oct 02, 2019 at 08:57:02PM +0200, Jessica Yu wrote:
>+++ Matthias Maennich [27/09/19 14:41 +0100]:
>>On Fri, Sep 27, 2019 at 06:35:56PM +0900, Masahiro Yamada wrote:
>>>
>>>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 and improvements.
>>>
>>>I hope these patches are mostly OK.
>>>The 4th patch might have room for argument since it is a trade-off
>>>of "cleaner implermentation" vs "code size".
>>>
>>Thanks Masahiro for taking the time to improve the implementation of the
>>symbol namespaces. These are all good points that you addressed!
>
>Agreed, thanks Masahiro for fixing up all the rough edges! Your series
>of fixes look good to me, I will queue this up on modules-next this
>week with the exception of patch 4 - Matthias, you are planning to
>submit a patch that would supercede patch 04/07, right?
I will most likely create a patch on top of 04/07 and will submit
everything as a separate series.
Cheers,
Matthias
H Jessica,
On Thu, Oct 3, 2019 at 3:57 AM Jessica Yu <[email protected]> wrote:
>
> +++ Matthias Maennich [27/09/19 14:41 +0100]:
> >On Fri, Sep 27, 2019 at 06:35:56PM +0900, Masahiro Yamada wrote:
> >>
> >>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 and improvements.
> >>
> >>I hope these patches are mostly OK.
> >>The 4th patch might have room for argument since it is a trade-off
> >>of "cleaner implermentation" vs "code size".
> >>
> >Thanks Masahiro for taking the time to improve the implementation of the
> >symbol namespaces. These are all good points that you addressed!
>
> Agreed, thanks Masahiro for fixing up all the rough edges! Your series
> of fixes look good to me, I will queue this up on modules-next this
> week
Since these are bug fixes,
please send them before v5.4.
Thanks.
> with the exception of patch 4 - Matthias, you are planning to
> submit a patch that would supercede patch 04/07, right?
>
> Thanks!
>
> Jessica
--
Best Regards
Masahiro Yamada
Hi Jessica,
On Thu, Oct 3, 2019 at 3:57 AM Jessica Yu <[email protected]> wrote:
>
> +++ Matthias Maennich [27/09/19 14:41 +0100]:
> >On Fri, Sep 27, 2019 at 06:35:56PM +0900, Masahiro Yamada wrote:
> >>
> >>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 and improvements.
> >>
> >>I hope these patches are mostly OK.
> >>The 4th patch might have room for argument since it is a trade-off
> >>of "cleaner implermentation" vs "code size".
> >>
> >Thanks Masahiro for taking the time to improve the implementation of the
> >symbol namespaces. These are all good points that you addressed!
>
> Agreed, thanks Masahiro for fixing up all the rough edges! Your series
> of fixes look good to me, I will queue this up on modules-next this
> week with the exception of patch 4 - Matthias, you are planning to
> submit a patch that would supercede patch 04/07, right?
>
> Thanks!
I missed to fix one issue in v1.
sym_add_exported() misses to set s->namespace
if the struct symbol is already created by
read_dump() or sym_update_crc().
So, I just sent v2.
--
Best Regards
Masahiro Yamada
+++ Rasmus Villemoes [27/09/19 13:07 +0200]:
>On 27/09/2019 11.36, Masahiro Yamada wrote:
>> include/linux/export.h has lots of code duplication between
>> EXPORT_SYMBOL and EXPORT_SYMBOL_NS.
>>
>> To improve the maintainability and readability, unify the
>> implementation.
>>
>> When the symbol has no namespace, pass the empty string "" to
>> the 'ns' parameter.
>>
>> The drawback of this change is, it grows the code size.
>> When the symbol has no namespace, sym->namespace was previously
>> NULL, but it is now am empty string "". So, it increases 1 byte
>> for every no namespace EXPORT_SYMBOL.
>>
>> A typical kernel configuration has 10K exported symbols, so it
>> increases 10KB in rough estimation.
>>
>> I did not come up with a good idea to refactor it without increasing
>> the code size.
>
>Can't we put the "aMS" flags on the __ksymtab_strings section? That
>would make the empty strings free, and would also deduplicate the
>USB_STORAGE string. And while almost per definition we don't have exact
>duplicates among the names of exported symbols, we might have both a foo
>and __foo, so that could save even more.
>
>I don't know if we have it already, but we'd need each arch to tell us
>what symbol to use for @ in @progbits (e.g. % for arm). It seems most
>are fine with @, so maybe a generic version could be
>
>#ifndef ARCH_SECTION_TYPE_CHAR
>#define ARCH_SECTION_TYPE_CHAR "@"
>#endif
>
>and then it would be
>section("__ksymtab_strings,\"aMS\","ARCH_SECTION_TYPE_CHAR"progbits,1")
FWIW, I've just tinkered with this, and unfortunately the strings
don't get deduplicated for kernel modules :-(
Apparently ld does not do the deduplication for SHF_MERGE|SHF_STRINGS
sections for relocatable files (ld -r), which kernel modules are. See:
https://sourceware.org/ml/binutils/2009-07/msg00291.html
But, the strings do get deduplicated for vmlinux. Not sure if we can
find a workaround for modules or if the benefit is significant enough
if it only for vmlinux.
Thanks,
Jessica
On 29/10/2019 20.19, Jessica Yu wrote:
> +++ Rasmus Villemoes [27/09/19 13:07 +0200]:
>> On 27/09/2019 11.36, Masahiro Yamada wrote:
>>>
>>> A typical kernel configuration has 10K exported symbols, so it
>>> increases 10KB in rough estimation.
>>>
>>> I did not come up with a good idea to refactor it without increasing
>>> the code size.
>>
>> Can't we put the "aMS" flags on the __ksymtab_strings section? That
>> would make the empty strings free, and would also deduplicate the
>> USB_STORAGE string. And while almost per definition we don't have exact
>> duplicates among the names of exported symbols, we might have both a foo
>> and __foo, so that could save even more.
>>
>> I don't know if we have it already, but we'd need each arch to tell us
>> what symbol to use for @ in @progbits (e.g. % for arm). It seems most
>> are fine with @, so maybe a generic version could be
>>
>> #ifndef ARCH_SECTION_TYPE_CHAR
>> #define ARCH_SECTION_TYPE_CHAR "@"
>> #endif
>>
>> and then it would be
>> section("__ksymtab_strings,\"aMS\","ARCH_SECTION_TYPE_CHAR"progbits,1")
>
> FWIW, I've just tinkered with this, and unfortunately the strings
> don't get deduplicated for kernel modules :-(
>
> Apparently ld does not do the deduplication for SHF_MERGE|SHF_STRINGS
> sections for relocatable files (ld -r), which kernel modules are. See:
>
> ?? https://sourceware.org/ml/binutils/2009-07/msg00291.html
I know <https://patches-gcc.linaro.org/patch/5858/> :)
> But, the strings do get deduplicated for vmlinux. Not sure if we can
> find a workaround for modules or if the benefit is significant enough
> if it only for vmlinux.
I think it's definitely worth if, even if it "only" benefits vmlinux for
now. And I still hope to revisit the --force-section-merge some day, but
it's very far down my priority list.
Rasmus
+++ Rasmus Villemoes [29/10/19 22:11 +0100]:
>On 29/10/2019 20.19, Jessica Yu wrote:
>> +++ Rasmus Villemoes [27/09/19 13:07 +0200]:
>>> On 27/09/2019 11.36, Masahiro Yamada wrote:
>>>>
>>>> A typical kernel configuration has 10K exported symbols, so it
>>>> increases 10KB in rough estimation.
>>>>
>>>> I did not come up with a good idea to refactor it without increasing
>>>> the code size.
>>>
>>> Can't we put the "aMS" flags on the __ksymtab_strings section? That
>>> would make the empty strings free, and would also deduplicate the
>>> USB_STORAGE string. And while almost per definition we don't have exact
>>> duplicates among the names of exported symbols, we might have both a foo
>>> and __foo, so that could save even more.
>>>
>>> I don't know if we have it already, but we'd need each arch to tell us
>>> what symbol to use for @ in @progbits (e.g. % for arm). It seems most
>>> are fine with @, so maybe a generic version could be
>>>
>>> #ifndef ARCH_SECTION_TYPE_CHAR
>>> #define ARCH_SECTION_TYPE_CHAR "@"
>>> #endif
>>>
>>> and then it would be
>>> section("__ksymtab_strings,\"aMS\","ARCH_SECTION_TYPE_CHAR"progbits,1")
>>
>> FWIW, I've just tinkered with this, and unfortunately the strings
>> don't get deduplicated for kernel modules :-(
>>
>> Apparently ld does not do the deduplication for SHF_MERGE|SHF_STRINGS
>> sections for relocatable files (ld -r), which kernel modules are. See:
>>
>> ?? https://sourceware.org/ml/binutils/2009-07/msg00291.html
>
>I know <https://patches-gcc.linaro.org/patch/5858/> :)
That is exactly what we need! :)
>> But, the strings do get deduplicated for vmlinux. Not sure if we can
>> find a workaround for modules or if the benefit is significant enough
>> if it only for vmlinux.
>
>I think it's definitely worth if, even if it "only" benefits vmlinux for
>now. And I still hope to revisit the --force-section-merge some day, but
>it's very far down my priority list.
Yeah, I think it's worth having too.
If you don't have any extra cycles at the moment, and it's far down
your priority list, do you mind if I take a look and maybe try to push
that patch of yours upstream again? I don't know how successful I'd
be, but now since it's especially relevant for namespaces, it's
definitely worth looking at again.
Thanks!
Jessica
On 31/10/2019 11.13, Jessica Yu wrote:
> +++ Rasmus Villemoes [29/10/19 22:11 +0100]:
>> On 29/10/2019 20.19, Jessica Yu wrote:
>>> Apparently ld does not do the deduplication for SHF_MERGE|SHF_STRINGS
>>> sections for relocatable files (ld -r), which kernel modules are. See:
>>>
>>> ?? https://sourceware.org/ml/binutils/2009-07/msg00291.html
>>
>> I know <https://patches-gcc.linaro.org/patch/5858/> :)
>
> That is exactly what we need! :)
>
>>> But, the strings do get deduplicated for vmlinux. Not sure if we can
>>> find a workaround for modules or if the benefit is significant enough
>>> if it only for vmlinux.
>>
>> I think it's definitely worth if, even if it "only" benefits vmlinux for
>> now. And I still hope to revisit the --force-section-merge some day, but
>> it's very far down my priority list.
>
> Yeah, I think it's worth having too.
>
> If you don't have any extra cycles at the moment, and it's far down
> your priority list, do you mind if I take a look and maybe try to push
> that patch of yours upstream again?
Knock yourself out :) IIRC, it did actually work for the powerpc I was
targeting, but I don't remember if that was just "readelf/objdump
inspection of the ELF files looks reasonable" or if I actually tried
loading the modules. I've pushed the patch to
https://github.com/Villemoes/binutils-gdb/commit/107b9302858fc5fc1a1690f4a36e1f80808ab421
so you don't have to copy-paste from a browser.
I don't know how successful I'd
> be, but now since it's especially relevant for namespaces, it's
> definitely worth looking at again.
Yeah, but even ignoring namespaces, it would be nice to have format
strings etc. deduplicated. Please keep me cc'ed on any progress you make.
Thanks,
Rasmus
+++ Rasmus Villemoes [31/10/19 12:03 +0100]:
>On 31/10/2019 11.13, Jessica Yu wrote:
>> +++ Rasmus Villemoes [29/10/19 22:11 +0100]:
>>> On 29/10/2019 20.19, Jessica Yu wrote:
>
>>>> Apparently ld does not do the deduplication for SHF_MERGE|SHF_STRINGS
>>>> sections for relocatable files (ld -r), which kernel modules are. See:
>>>>
>>>> ?? https://sourceware.org/ml/binutils/2009-07/msg00291.html
>>>
>>> I know <https://patches-gcc.linaro.org/patch/5858/> :)
>>
>> That is exactly what we need! :)
>>
>>>> But, the strings do get deduplicated for vmlinux. Not sure if we can
>>>> find a workaround for modules or if the benefit is significant enough
>>>> if it only for vmlinux.
>>>
>>> I think it's definitely worth if, even if it "only" benefits vmlinux for
>>> now. And I still hope to revisit the --force-section-merge some day, but
>>> it's very far down my priority list.
>>
>> Yeah, I think it's worth having too.
>>
>> If you don't have any extra cycles at the moment, and it's far down
>> your priority list, do you mind if I take a look and maybe try to push
>> that patch of yours upstream again?
>
>Knock yourself out :) IIRC, it did actually work for the powerpc I was
>targeting, but I don't remember if that was just "readelf/objdump
>inspection of the ELF files looks reasonable" or if I actually tried
>loading the modules. I've pushed the patch to
>https://github.com/Villemoes/binutils-gdb/commit/107b9302858fc5fc1a1690f4a36e1f80808ab421
>so you don't have to copy-paste from a browser.
Thanks a bunch!
>I don't know how successful I'd
>> be, but now since it's especially relevant for namespaces, it's
>> definitely worth looking at again.
>
>Yeah, but even ignoring namespaces, it would be nice to have format
>strings etc. deduplicated. Please keep me cc'ed on any progress you make.
Thanks, I'll keep you posted if I manage to get somewhere with it :)