As of Linux 5.3-rc4, there are 31203 [1] exported symbols in the kernel.
That is a growth of almost 1000 symbols since 4.17 (30206 [2]). There
seems to be some consensus amongst kernel devs that the export surface
is too large, and hard to reason about.
Generally, these symbols fall in one of these categories:
1) Symbols actually meant for drivers
2) Symbols that are only exported because functionality is split over
multiple modules, yet they really shouldn't be used by modules outside
of their own subsystem
3) Symbols really only meant for in-tree use
When module developers try to upstream their code, it regularly turns
out that they are using exported symbols that they really shouldn't be
using. This problem is even bigger for drivers that are currently
out-of-tree, which may be using many symbols that they shouldn't be
using, and that break when those symbols are removed or modified.
This patch allows subsystem maintainers to partition their exported
symbols into separate namespaces, and module authors to import such
namespaces only when needed.
This allows subsystem maintainers to more easily limit availability of
these namespaced symbols to other parts of the kernel. It can also be
used to partition the set of exported symbols for documentation
purposes; for example, a set of symbols that is really only used for
debugging could be in a "SUBSYSTEM_DEBUG" namespace.
I continued the work mainly done by Martijn Coenen. In this v2 the
following changes have been introduced compared to v1 of this series:
- Rather than adding and evaluating separate sections __knsimport_NS,
use modinfo tags to declare the namespaces a module introduces.
Adjust modpost and the module loader accordingly.
- Also add support for reading multiple modinfo values for the same tag
to allow list-like access to modinfo tags.
- The macros in export.h have been cleaned up to avoid redundancy in the
macro parameters (ns, nspost, nspost2).
- The introduction of relative references in the ksymtab entries caused
a rework of the macros to accommodate that configuration as well.
- Alignment of kernel_symbol in the ksymtab needed to be fixed to allow
growing the kernel_symbol struct.
- Modpost does now also append the namespace suffix to the symbol
entries in Module.symvers.
- The configuration option MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS allows
relaxing the enforcement of properly declared namespace imports at module
loading time.
- Symbols can be collectively exported into a namespace by defining
DEFAULT_SYMBOL_NAMESPACE in the corresponding Makefile.
- The requirement for a very recent coccinelle spatch has been lifted by
simplifying the script.
- nsdeps does now ensures MODULE_IMPORT_NS statements are sorted when
patching the module source files.
- Some minor bugs have been addressed in nsdeps to allow it to work with
modules that have more than one source file.
- The RFC for the usb-storage symbols has been simplified by using
DEFAULT_SYMBOL_NAMESPACE=USB_STORAGE rather than explicitly exporting each
and every symbol into that new namespace.
This patch series was developed against v5.3-rc4.
[1] git grep "^EXPORT_SYMBOL\w*(" v5.3-rc4 | wc -l
[2] git grep "^EXPORT_SYMBOL\w*(" v4.17 | wc -l
Matthias Maennich (10):
module: support reading multiple values per modinfo tag
export: explicitly align struct kernel_symbol
module: add support for symbol namespaces.
modpost: add support for symbol namespaces
module: add config option MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS
export: allow definition default namespaces in Makefiles or sources
modpost: add support for generating namespace dependencies
scripts: Coccinelle script for namespace dependencies.
usb-storage: remove single-use define for debugging
RFC: usb-storage: export symbols in USB_STORAGE namespace
MAINTAINERS | 5 +
Makefile | 12 ++
arch/m68k/include/asm/export.h | 1 -
drivers/usb/storage/Makefile | 2 +
drivers/usb/storage/alauda.c | 1 +
drivers/usb/storage/cypress_atacb.c | 1 +
drivers/usb/storage/datafab.c | 1 +
drivers/usb/storage/debug.h | 2 -
drivers/usb/storage/ene_ub6250.c | 1 +
drivers/usb/storage/freecom.c | 1 +
drivers/usb/storage/isd200.c | 1 +
drivers/usb/storage/jumpshot.c | 1 +
drivers/usb/storage/karma.c | 1 +
drivers/usb/storage/onetouch.c | 1 +
drivers/usb/storage/realtek_cr.c | 1 +
drivers/usb/storage/scsiglue.c | 2 +-
drivers/usb/storage/sddr09.c | 1 +
drivers/usb/storage/sddr55.c | 1 +
drivers/usb/storage/shuttle_usbat.c | 1 +
drivers/usb/storage/uas.c | 1 +
include/asm-generic/export.h | 14 +-
include/linux/export.h | 92 +++++++++++--
include/linux/module.h | 2 +
init/Kconfig | 14 ++
kernel/module.c | 67 ++++++++-
scripts/Makefile.modpost | 4 +-
scripts/coccinelle/misc/add_namespace.cocci | 23 ++++
scripts/mod/modpost.c | 144 ++++++++++++++++++--
scripts/mod/modpost.h | 9 ++
scripts/nsdeps | 54 ++++++++
30 files changed, 421 insertions(+), 40 deletions(-)
create mode 100644 scripts/coccinelle/misc/add_namespace.cocci
create mode 100644 scripts/nsdeps
--
2.23.0.rc1.153.gdeed80330f-goog
Similar to modpost's get_next_modinfo(), introduce get_next_modinfo() in
kernel/module.c to acquire any further values associated with the same
modinfo tag name. That is useful for any tags that have multiple
occurrences (such as 'alias'), but is in particular introduced here as
part of the symbol namespaces patch series to read the (potentially)
multiple namespaces a module is importing.
Reviewed-by: Joel Fernandes (Google) <redacted>
Reviewed-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
kernel/module.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
This change allows growing struct kernel_symbol without wasting bytes to
alignment. It also concretized the alignment of ksymtab entries if
relative references are used for ksymtab entries.
struct kernel_symbol was already implicitly being aligned to the word
size, except on x86_64 and m68k, where it is aligned to 16 and 2 bytes,
respectively.
As far as I can tell there is no requirement for aligning struct
kernel_symbol to 16 bytes on x86_64, but gcc aligns structs to their
size, and the linker aligns the custom __ksymtab sections to the largest
data type contained within, so setting KSYM_ALIGN to 16 was necessary to
stay consistent with the code generated for non-ASM EXPORT_SYMBOL(). Now
that non-ASM EXPORT_SYMBOL() explicitly aligns to word size (8),
KSYM_ALIGN is no longer necessary.
In case of relative references, the alignment has been changed
accordingly to not waste space when adding new struct members.
As for m68k, struct kernel_symbol is aligned to 2 bytes even though the
structure itself is 8 bytes; using a 4-byte alignment shouldn't hurt.
I manually verified the output of the __ksymtab sections didn't change
on x86, x86_64, arm, arm64 and m68k. As expected, the section contents
didn't change, and the ELF section alignment only changed on x86_64 and
m68k. Feedback from other archs more than welcome.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
arch/m68k/include/asm/export.h | 1 -
include/asm-generic/export.h | 8 +++-----
include/linux/export.h | 3 ++-
3 files changed, 5 insertions(+), 7 deletions(-)
The EXPORT_SYMBOL_NS() and EXPORT_SYMBOL_NS_GPL() macros can be used to
export a symbol to a specific namespace. There are no _GPL_FUTURE and
_UNUSED variants because these are currently unused, and I'm not sure
they are necessary.
I didn't add EXPORT_SYMBOL_NS() for ASM exports; this patch sets the
namespace of ASM exports to NULL by default. In case of relative
references, it will be relocatable to NULL. If there's a need, this
should be pretty easy to add.
A module that wants to use a symbol exported to a namespace must add a
MODULE_IMPORT_NS() statement to their module code; otherwise, modpost
will complain when building the module, and the kernel module loader
will emit an error and fail when loading the module.
MODULE_IMPORT_NS() adds a modinfo tag 'import_ns' to the module. That
tag can be observed by the modinfo command, modpost and kernel/module.c
at the time of loading the module.
The ELF symbols are renamed to include the namespace with an asm label;
for example, symbol 'usb_stor_suspend' in namespace USB_STORAGE becomes
'usb_stor_suspend.USB_STORAGE'. This allows modpost to do namespace
checking, without having to go through all the effort of parsing ELF and
relocation records just to get to the struct kernel_symbols.
On x86_64 I saw no difference in binary size (compression), but at
runtime this will require a word of memory per export to hold the
namespace. An alternative could be to store namespaced symbols in their
own section and use a separate 'struct namespaced_kernel_symbol' for
that section, at the cost of making the module loader more complex.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
include/asm-generic/export.h | 6 +--
include/linux/export.h | 85 ++++++++++++++++++++++++++++++------
include/linux/module.h | 2 +
kernel/module.c | 43 ++++++++++++++++++
4 files changed, 120 insertions(+), 16 deletions(-)
@@ -20,6 +20,8 @@ extern struct module __this_module;#ifdef CONFIG_MODULES+#define NS_SEPARATOR "."+#if defined(__KERNEL__) && !defined(__GENKSYMS__)#ifdef CONFIG_MODVERSIONS/* Mark the CRC weak since genksyms apparently decides not to
@@ -280,6 +280,8 @@ struct notifier_block;#ifdef CONFIG_MODULES+#define MODULE_IMPORT_NS(ns) MODULE_INFO(import_ns, #ns)+externintmodules_disabled;/* for sysctl *//* Get/put a kernel symbol (calls must be symmetric) */void*__symbol_get(constchar*symbol);
@@ -1379,6 +1388,34 @@ static inline int same_magic(const char *amagic, const char *bmagic,}#endif /* CONFIG_MODVERSIONS */+staticchar*get_modinfo(conststructload_info*info,constchar*tag);+staticchar*get_next_modinfo(conststructload_info*info,constchar*tag,+char*prev);++staticintverify_namespace_is_imported(conststructload_info*info,+conststructkernel_symbol*sym,+structmodule*mod)+{+constchar*namespace;+char*imported_namespace;++namespace=kernel_symbol_namespace(sym);+if(namespace){+imported_namespace=get_modinfo(info,"import_ns");+while(imported_namespace){+if(strcmp(namespace,imported_namespace)==0)+return0;+imported_namespace=get_next_modinfo(+info,"import_ns",imported_namespace);+}+pr_err("%s: module uses symbol (%s) from namespace %s, but does not import it.\n",+mod->name,kernel_symbol_name(sym),namespace);+return-EINVAL;+}+return0;+}++/* Resolve a symbol for this module. I.e. if we find one, record usage. */staticconststructkernel_symbol*resolve_symbol(structmodule*mod,conststructload_info*info,
@@ -1413,6 +1450,12 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,gotogetname;}+err=verify_namespace_is_imported(info,sym,mod);+if(err){+sym=ERR_PTR(err);+gotogetname;+}+getname:/* We must make copy under the lock if we failed to get ref. */strncpy(ownername,module_name(owner),MODULE_NAME_LEN);
Add support for symbols that are exported into namespaces. For that,
extract any namespace suffix from the symbol name. In addition, emit a
warning whenever a module refers to an exported symbol without
explicitly importing the namespace that it is defined in. This patch
consistently adds the namespace suffix to symbol names exported into
Module.symvers.
Example warning emitted by modpost in case of the above violation:
WARNING: module ums-usbat uses symbol usb_stor_resume from namespace
USB_STORAGE, but does not import it.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Reviewed-by: Joel Fernandes (Google) <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
scripts/mod/modpost.c | 91 +++++++++++++++++++++++++++++++++++++------
scripts/mod/modpost.h | 7 ++++
2 files changed, 87 insertions(+), 11 deletions(-)
@@ -164,6 +164,7 @@ struct symbol {structmodule*module;unsignedintcrc;intcrc_valid;+constchar*namespace;unsignedintweak:1;unsignedintvmlinux:1;/* 1 if symbol is defined in vmlinux */unsignedintkernel:1;/* 1 if symbol is from kernel
@@ -233,6 +234,37 @@ static struct symbol *find_symbol(const char *name)returnNULL;}+staticboolcontains_namespace(structnamespace_list*list,+constchar*namespace)+{+structnamespace_list*ns_entry;++for(ns_entry=list;ns_entry!=NULL;ns_entry=ns_entry->next)+if(strcmp(ns_entry->namespace,namespace)==0)+returntrue;++returnfalse;+}++staticvoidadd_namespace(structnamespace_list**list,constchar*namespace)+{+structnamespace_list*ns_entry;++if(!contains_namespace(*list,namespace)){+ns_entry=NOFAIL(malloc(sizeof(structnamespace_list)++strlen(namespace)+1));+strcpy(ns_entry->namespace,namespace);+ns_entry->next=*list;+*list=ns_entry;+}+}++staticboolmodule_imports_namespace(structmodule*module,+constchar*namespace)+{+returncontains_namespace(module->imported_namespaces,namespace);+}+staticconststruct{constchar*str;enumexportexport;
@@ -319,16 +367,18 @@ static enum export export_from_sec(struct elf_info *elf, unsigned int sec)staticstructsymbol*sym_add_exported(constchar*name,structmodule*mod,enumexportexport){-structsymbol*s=find_symbol(name);+constchar*symbol_name=name;+constchar*namespace=sym_extract_namespace(&symbol_name);+structsymbol*s=find_symbol(symbol_name);if(!s){-s=new_symbol(name,mod,export);+s=new_symbol(symbol_name,mod,export);+s->namespace=namespace;}else{if(!s->preloaded){-warn("%s: '%s' exported twice. Previous export "-"was in %s%s\n",mod->name,name,-s->module->name,-is_vmlinux(s->module->name)?"":".ko");+warn("%s: '%s' exported twice. Previous export was in %s%s\n",+mod->name,symbol_name,s->module->name,+is_vmlinux(s->module->name)?"":".ko");}else{/* In case Module.symvers was out of date */s->module=mod;
@@ -2118,6 +2175,13 @@ static int check_exports(struct module *mod)basename++;elsebasename=mod->name;++if(exp->namespace&&+!module_imports_namespace(mod,exp->namespace)){+warn("module %s uses symbol %s from namespace %s, but does not import it.\n",+basename,exp->name,exp->namespace);+}+if(!mod->gpl_compatible)check_for_gpl_usage(exp->export,basename,exp->name);check_for_unused(exp->export,basename,exp->name);
If MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is enabled (default=n), the
requirement for modules to import all namespaces that are used by
the module is relaxed.
Enabling this option effectively allows (invalid) modules to be loaded
while only a warning is emitted.
Disabling this option keeps the enforcement at module loading time and
loading is denied if the module's imports are not satisfactory.
Reviewed-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
init/Kconfig | 14 ++++++++++++++
kernel/module.c | 11 +++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
@@ -1408,9 +1408,16 @@ static int verify_namespace_is_imported(const struct load_info *info,imported_namespace=get_next_modinfo(info,"import_ns",imported_namespace);}-pr_err("%s: module uses symbol (%s) from namespace %s, but does not import it.\n",-mod->name,kernel_symbol_name(sym),namespace);+#ifdef CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS+pr_warn(+#else+pr_err(+#endif+"%s: module uses symbol (%s) from namespace %s, but does not import it.\n",+mod->name,kernel_symbol_name(sym),namespace);+#ifndef CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTSreturn-EINVAL;+#endif}return0;}
To avoid excessive usage of EXPORT_SYMBOL_NS(sym, MY_NAMESPACE), where
MY_NAMESPACE will always be the namespace we are exporting to, allow
exporting all definitions of EXPORT_SYMBOL() and friends by defining
DEFAULT_SYMBOL_NAMESPACE.
For example, to export all symbols defined in usb-common into the
namespace USB_COMMON, add a line like this to drivers/usb/common/Makefile:
ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_COMMON
That is equivalent to changing all EXPORT_SYMBOL(sym) definitions to
EXPORT_SYMBOL_NS(sym, USB_COMMON). Subsequently all symbol namespaces
functionality will apply.
Another way of making use of this feature is to define the namespace
within source or header files similar to how TRACE_SYSTEM defines are
used:
#undef DEFAULT_SYMBOL_NAMESPACE
#define DEFAULT_SYMBOL_NAMESPACE USB_COMMON
Please note that, as opposed to TRACE_SYSTEM, DEFAULT_SYMBOL_NAMESPACE
has to be defined before including include/linux/export.h.
If DEFAULT_SYMBOL_NAMESPACE is defined, a symbol can still be exported
to another namespace by using EXPORT_SYMBOL_NS() and friends with
explicitly specifying the namespace.
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
include/linux/export.h | 6 ++++++
1 file changed, 6 insertions(+)
This patch adds an option to modpost to generate a <module>.ns_deps file
per module, containing the namespace dependencies for that module.
E.g. if the linked module my-module.ko would depend on the symbol
myfunc.MY_NS in the namespace MY_NS, the my-module.ns_deps file created
by modpost would contain the entry MY_NS to express the namespace
dependency of my-module imposed by using the symbol myfunc.
These files can subsequently be used by static analysis tools (like
coccinelle scripts) to address issues with missing namespace imports. A
later patch of this series will introduce such a script 'nsdeps' and a
corresponding make target to automatically add missing
MODULE_IMPORT_NS() definitions to the module's sources. For that it uses
the information provided in the generated .ns_deps files.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
scripts/mod/modpost.c | 61 +++++++++++++++++++++++++++++++++++++++----
scripts/mod/modpost.h | 2 ++
2 files changed, 58 insertions(+), 5 deletions(-)
@@ -2176,10 +2178,15 @@ static int check_exports(struct module *mod)elsebasename=mod->name;-if(exp->namespace&&-!module_imports_namespace(mod,exp->namespace)){-warn("module %s uses symbol %s from namespace %s, but does not import it.\n",-basename,exp->name,exp->namespace);+if(exp->namespace){+add_namespace(&mod->required_namespaces,+exp->namespace);++if(!write_namespace_deps&&+!module_imports_namespace(mod,exp->namespace)){+warn("module %s uses symbol %s from namespace %s, but does not import it.\n",+basename,exp->name,exp->namespace);+}}if(!mod->gpl_compatible)
A script that uses the '<module>.ns_deps' file generated by modpost to
automatically add the required symbol namespace dependencies to each
module.
Usage:
1) Move some symbols to a namespace with EXPORT_SYMBOL_NS() or define
DEFAULT_SYMBOL_NAMESPACE
2) Run 'make' (or 'make modules') and get warnings about modules not
importing that namespace.
3) Run 'make nsdeps' to automatically add required import statements
to said modules.
This makes it easer for subsystem maintainers to introduce and maintain
symbol namespaces into their codebase.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
MAINTAINERS | 5 ++
Makefile | 12 +++++
scripts/Makefile.modpost | 4 +-
scripts/coccinelle/misc/add_namespace.cocci | 23 +++++++++
scripts/nsdeps | 54 +++++++++++++++++++++
5 files changed, 97 insertions(+), 1 deletion(-)
create mode 100644 scripts/coccinelle/misc/add_namespace.cocci
create mode 100644 scripts/nsdeps
@@ -1500,6 +1500,9 @@ help:@echo' headerdep - Detect inclusion cycles in headers'@echo' coccicheck - Check with Coccinelle'@echo''+@echo'Tools:'+@echo' nsdeps - Generate missing symbol namespace dependencies'+@echo''@echo'Kernel selftest:'@echo' kselftest - Build and run kernel selftest (run as root)'@echo' Build, install, and boot kernel before'
@@ -1687,6 +1690,15 @@ quiet_cmd_tags = GEN $@tags TAGS cscope gtags:FORCE$(callcmd,tags)+# Script to generate missing namespace dependencies+# ---------------------------------------------------------------------------++PHONY+=nsdeps++nsdeps:+$(Q)$(MAKE)-f$(srctree)/scripts/Makefile.modpostnsdeps+$(Q)$(CONFIG_SHELL)$(srctree)/scripts/$@+# Scripts to check various things for consistency# ---------------------------------------------------------------------------
@@ -134,6 +135,7 @@ $(modules): %.ko :%.o %.mod.o FORCEtargets+=$(modules)+nsdeps:__modpost# Add FORCE to the prequisites of a target to force it to be always rebuilt.# ---------------------------------------------------------------------------
@@ -0,0 +1,23 @@+// SPDX-License-Identifier: GPL-2.0-only+//+/// Adds missing MODULE_IMPORT_NS statements to source files+///+/// This script is usually called from scripts/nsdeps with -D ns=<namespace> to+/// add a missing namespace tag to a module source file.+///++@has_ns_import@+declarer name MODULE_IMPORT_NS;+identifier virtual.ns;+@@+MODULE_IMPORT_NS(ns);++// Add missing imports, but only adjacent to a MODULE_LICENSE statement.+// That ensures we are adding it only to the main module source file.+@do_import depends on !has_ns_import@+declarer name MODULE_LICENSE;+expression license;+identifier virtual.ns;+@@+MODULE_LICENSE(license);++ MODULE_IMPORT_NS(ns);
USB_STORAGE was defined as "usb-storage: " and used in a single location
as argument to printk. In order to be able to use the name
'USB_STORAGE', drop the definition and use the string directly for the
printk call.
Signed-off-by: Matthias Maennich <maennich@google.com>
---
drivers/usb/storage/debug.h | 2 --
drivers/usb/storage/scsiglue.c | 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
Modules using these symbols are required to explicitly import the
namespace. This patch was generated with the following steps and serves
as a reference to use the symbol namespace feature:
1) Define DDEFAULT_SYMBOL_NAMESPACE in the corresponding Makefile
2) make (see warnings during modpost about missing imports)
3) make nsdeps
Instead of a DEFAULT_SYMBOL_NAMESPACE definition, the EXPORT_SYMBOL_NS
variants can be used to explicitly specify the namespace. The advantage
of the method used here is that newly added symbols are automatically
exported and existing ones are exported without touching their
respective EXPORT_SYMBOL macro expansion.
Signed-off-by: Matthias Maennich <maennich@google.com>
---
drivers/usb/storage/Makefile | 2 ++
drivers/usb/storage/alauda.c | 1 +
drivers/usb/storage/cypress_atacb.c | 1 +
drivers/usb/storage/datafab.c | 1 +
drivers/usb/storage/ene_ub6250.c | 1 +
drivers/usb/storage/freecom.c | 1 +
drivers/usb/storage/isd200.c | 1 +
drivers/usb/storage/jumpshot.c | 1 +
drivers/usb/storage/karma.c | 1 +
drivers/usb/storage/onetouch.c | 1 +
drivers/usb/storage/realtek_cr.c | 1 +
drivers/usb/storage/sddr09.c | 1 +
drivers/usb/storage/sddr55.c | 1 +
drivers/usb/storage/shuttle_usbat.c | 1 +
drivers/usb/storage/uas.c | 1 +
15 files changed, 16 insertions(+)
@@ -22,6 +22,7 @@MODULE_DESCRIPTION("SAT support for Cypress USB/ATA bridges with ATACB");MODULE_AUTHOR("Matthieu Castet <castet.matthieu@free.fr>");MODULE_LICENSE("GPL");+MODULE_IMPORT_NS(USB_STORAGE);/**Thetableofdevices
@@ -26,6 +26,7 @@MODULE_DESCRIPTION("Driver for ENE UB6250 reader");MODULE_LICENSE("GPL");+MODULE_IMPORT_NS(USB_STORAGE);MODULE_FIRMWARE(SD_INIT1_FIRMWARE);MODULE_FIRMWARE(SD_INIT2_FIRMWARE);MODULE_FIRMWARE(SD_RW_FIRMWARE);
@@ -35,6 +35,7 @@MODULE_DESCRIPTION("Driver for Realtek USB Card Reader");MODULE_AUTHOR("wwang <wei_wang@realsil.com.cn>");MODULE_LICENSE("GPL");+MODULE_IMPORT_NS(USB_STORAGE);staticintauto_delink_en=1;module_param(auto_delink_en,int,S_IRUGO|S_IWUSR);
@@ -1219,5 +1219,6 @@ static struct usb_driver uas_driver = {module_usb_driver(uas_driver);MODULE_LICENSE("GPL");+MODULE_IMPORT_NS(USB_STORAGE);MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>, Matthew Wilcox and Sarah Sharp");
From: Julia Lawall <hidden> Date: 2019-08-13 12:31:34
On Tue, 13 Aug 2019, Matthias Maennich wrote:
A script that uses the '<module>.ns_deps' file generated by modpost to
automatically add the required symbol namespace dependencies to each
module.
Usage:
1) Move some symbols to a namespace with EXPORT_SYMBOL_NS() or define
DEFAULT_SYMBOL_NAMESPACE
2) Run 'make' (or 'make modules') and get warnings about modules not
importing that namespace.
3) Run 'make nsdeps' to automatically add required import statements
to said modules.
This makes it easer for subsystem maintainers to introduce and maintain
symbol namespaces into their codebase.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
@@ -1500,6 +1500,9 @@ help:@echo' headerdep - Detect inclusion cycles in headers'@echo' coccicheck - Check with Coccinelle'@echo''+@echo'Tools:'+@echo' nsdeps - Generate missing symbol namespace dependencies'+@echo''@echo'Kernel selftest:'@echo' kselftest - Build and run kernel selftest (run as root)'@echo' Build, install, and boot kernel before'
@@ -1687,6 +1690,15 @@ quiet_cmd_tags = GEN $@tags TAGS cscope gtags:FORCE$(callcmd,tags)+# Script to generate missing namespace dependencies+# ---------------------------------------------------------------------------++PHONY+=nsdeps++nsdeps:+$(Q)$(MAKE)-f$(srctree)/scripts/Makefile.modpostnsdeps+$(Q)$(CONFIG_SHELL)$(srctree)/scripts/$@+# Scripts to check various things for consistency# ---------------------------------------------------------------------------
@@ -134,6 +135,7 @@ $(modules): %.ko :%.o %.mod.o FORCEtargets+=$(modules)+nsdeps:__modpost# Add FORCE to the prequisites of a target to force it to be always rebuilt.# ---------------------------------------------------------------------------
@@ -0,0 +1,23 @@+// SPDX-License-Identifier: GPL-2.0-only+//+/// Adds missing MODULE_IMPORT_NS statements to source files+///+/// This script is usually called from scripts/nsdeps with -D ns=<namespace> to+/// add a missing namespace tag to a module source file.+///++@has_ns_import@+declarer name MODULE_IMPORT_NS;+identifier virtual.ns;+@@+MODULE_IMPORT_NS(ns);++// Add missing imports, but only adjacent to a MODULE_LICENSE statement.+// That ensures we are adding it only to the main module source file.+@do_import depends on !has_ns_import@+declarer name MODULE_LICENSE;+expression license;+identifier virtual.ns;+@@+MODULE_LICENSE(license);++ MODULE_IMPORT_NS(ns);
On Tue, Aug 13, 2019 at 01:16:58PM +0100, Matthias Maennich wrote:
Similar to modpost's get_next_modinfo(), introduce get_next_modinfo() in
kernel/module.c to acquire any further values associated with the same
modinfo tag name. That is useful for any tags that have multiple
occurrences (such as 'alias'), but is in particular introduced here as
part of the symbol namespaces patch series to read the (potentially)
multiple namespaces a module is importing.
Reviewed-by: Joel Fernandes (Google) <redacted>
Reviewed-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
On Tue, Aug 13, 2019 at 01:16:59PM +0100, Matthias Maennich wrote:
This change allows growing struct kernel_symbol without wasting bytes to
alignment. It also concretized the alignment of ksymtab entries if
relative references are used for ksymtab entries.
struct kernel_symbol was already implicitly being aligned to the word
size, except on x86_64 and m68k, where it is aligned to 16 and 2 bytes,
respectively.
As far as I can tell there is no requirement for aligning struct
kernel_symbol to 16 bytes on x86_64, but gcc aligns structs to their
size, and the linker aligns the custom __ksymtab sections to the largest
data type contained within, so setting KSYM_ALIGN to 16 was necessary to
stay consistent with the code generated for non-ASM EXPORT_SYMBOL(). Now
that non-ASM EXPORT_SYMBOL() explicitly aligns to word size (8),
KSYM_ALIGN is no longer necessary.
In case of relative references, the alignment has been changed
accordingly to not waste space when adding new struct members.
As for m68k, struct kernel_symbol is aligned to 2 bytes even though the
structure itself is 8 bytes; using a 4-byte alignment shouldn't hurt.
I manually verified the output of the __ksymtab sections didn't change
on x86, x86_64, arm, arm64 and m68k. As expected, the section contents
didn't change, and the ELF section alignment only changed on x86_64 and
m68k. Feedback from other archs more than welcome.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
On Tue, Aug 13, 2019 at 01:17:06PM +0100, Matthias Maennich wrote:
USB_STORAGE was defined as "usb-storage: " and used in a single location
as argument to printk. In order to be able to use the name
'USB_STORAGE', drop the definition and use the string directly for the
printk call.
Signed-off-by: Matthias Maennich <maennich@google.com>
---
drivers/usb/storage/debug.h | 2 --
drivers/usb/storage/scsiglue.c | 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
I'll go take this today. The module really should just be using
dev_err() there. It needs to be cleaned up :(
thanks,
greg k-h
On Tue, Aug 13, 2019 at 01:17:05PM +0100, Matthias Maennich wrote:
A script that uses the '<module>.ns_deps' file generated by modpost to
automatically add the required symbol namespace dependencies to each
module.
Usage:
1) Move some symbols to a namespace with EXPORT_SYMBOL_NS() or define
DEFAULT_SYMBOL_NAMESPACE
2) Run 'make' (or 'make modules') and get warnings about modules not
importing that namespace.
3) Run 'make nsdeps' to automatically add required import statements
to said modules.
This makes it easer for subsystem maintainers to introduce and maintain
symbol namespaces into their codebase.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
I really can't express just how cool this patch is. I was amazed when I
first saw it in action a long time ago, and still am.
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Aug 13, 2019 at 01:17:07PM +0100, Matthias Maennich wrote:
quoted hunk
Modules using these symbols are required to explicitly import the
namespace. This patch was generated with the following steps and serves
as a reference to use the symbol namespace feature:
1) Define DDEFAULT_SYMBOL_NAMESPACE in the corresponding Makefile
2) make (see warnings during modpost about missing imports)
3) make nsdeps
Instead of a DEFAULT_SYMBOL_NAMESPACE definition, the EXPORT_SYMBOL_NS
variants can be used to explicitly specify the namespace. The advantage
of the method used here is that newly added symbols are automatically
exported and existing ones are exported without touching their
respective EXPORT_SYMBOL macro expansion.
Signed-off-by: Matthias Maennich <maennich@google.com>
---
drivers/usb/storage/Makefile | 2 ++
drivers/usb/storage/alauda.c | 1 +
drivers/usb/storage/cypress_atacb.c | 1 +
drivers/usb/storage/datafab.c | 1 +
drivers/usb/storage/ene_ub6250.c | 1 +
drivers/usb/storage/freecom.c | 1 +
drivers/usb/storage/isd200.c | 1 +
drivers/usb/storage/jumpshot.c | 1 +
drivers/usb/storage/karma.c | 1 +
drivers/usb/storage/onetouch.c | 1 +
drivers/usb/storage/realtek_cr.c | 1 +
drivers/usb/storage/sddr09.c | 1 +
drivers/usb/storage/sddr55.c | 1 +
drivers/usb/storage/shuttle_usbat.c | 1 +
drivers/usb/storage/uas.c | 1 +
15 files changed, 16 insertions(+)
Wait, we have to do this for every subsystem? I thought there was a
macro we could use in the code itself for this. What changed from
earlier versions, or was this always here?
thanks,
greg k-h
On Tue, Aug 13, 2019 at 01:17:07PM +0100, Matthias Maennich wrote:
Modules using these symbols are required to explicitly import the
namespace. This patch was generated with the following steps and serves
as a reference to use the symbol namespace feature:
1) Define DDEFAULT_SYMBOL_NAMESPACE in the corresponding Makefile
2) make (see warnings during modpost about missing imports)
3) make nsdeps
Instead of a DEFAULT_SYMBOL_NAMESPACE definition, the EXPORT_SYMBOL_NS
variants can be used to explicitly specify the namespace. The advantage
of the method used here is that newly added symbols are automatically
exported and existing ones are exported without touching their
respective EXPORT_SYMBOL macro expansion.
Ok, I can't read text, this answers my previous question.
But, as an example, shouldn't we also have some code here that uses the
EXPORT_SYMBOL_NS() macro to ensure that it actually works?
thanks,
greg k-h
On Tue, Aug 13, 2019 at 02:42:59PM +0200, Greg KH wrote:
On Tue, Aug 13, 2019 at 01:17:06PM +0100, Matthias Maennich wrote:
quoted
USB_STORAGE was defined as "usb-storage: " and used in a single location
as argument to printk. In order to be able to use the name
'USB_STORAGE', drop the definition and use the string directly for the
printk call.
Signed-off-by: Matthias Maennich <maennich@google.com>
---
drivers/usb/storage/debug.h | 2 --
drivers/usb/storage/scsiglue.c | 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
I'll go take this today. The module really should just be using
dev_err() there. It needs to be cleaned up :(
On Tue, Aug 13, 2019 at 02:47:08PM +0200, Greg KH wrote:
On Tue, Aug 13, 2019 at 01:17:07PM +0100, Matthias Maennich wrote:
quoted
Modules using these symbols are required to explicitly import the
namespace. This patch was generated with the following steps and serves
as a reference to use the symbol namespace feature:
1) Define DDEFAULT_SYMBOL_NAMESPACE in the corresponding Makefile
2) make (see warnings during modpost about missing imports)
3) make nsdeps
Instead of a DEFAULT_SYMBOL_NAMESPACE definition, the EXPORT_SYMBOL_NS
variants can be used to explicitly specify the namespace. The advantage
of the method used here is that newly added symbols are automatically
exported and existing ones are exported without touching their
respective EXPORT_SYMBOL macro expansion.
Ok, I can't read text, this answers my previous question.
But, as an example, shouldn't we also have some code here that uses the
EXPORT_SYMBOL_NS() macro to ensure that it actually works?
I will create another patch for a different subsystem where the use of
the macros is more appropriate. Then we have both use cases covered.
Cheers,
Matthias
On Tue, Aug 13, 2019 at 01:17:00PM +0100, Matthias Maennich wrote:
The EXPORT_SYMBOL_NS() and EXPORT_SYMBOL_NS_GPL() macros can be used to
export a symbol to a specific namespace. There are no _GPL_FUTURE and
_UNUSED variants because these are currently unused, and I'm not sure
they are necessary.
I didn't add EXPORT_SYMBOL_NS() for ASM exports; this patch sets the
namespace of ASM exports to NULL by default. In case of relative
references, it will be relocatable to NULL. If there's a need, this
should be pretty easy to add.
A module that wants to use a symbol exported to a namespace must add a
MODULE_IMPORT_NS() statement to their module code; otherwise, modpost
will complain when building the module, and the kernel module loader
will emit an error and fail when loading the module.
MODULE_IMPORT_NS() adds a modinfo tag 'import_ns' to the module. That
tag can be observed by the modinfo command, modpost and kernel/module.c
at the time of loading the module.
The ELF symbols are renamed to include the namespace with an asm label;
for example, symbol 'usb_stor_suspend' in namespace USB_STORAGE becomes
'usb_stor_suspend.USB_STORAGE'. This allows modpost to do namespace
checking, without having to go through all the effort of parsing ELF and
relocation records just to get to the struct kernel_symbols.
On x86_64 I saw no difference in binary size (compression), but at
runtime this will require a word of memory per export to hold the
namespace. An alternative could be to store namespaced symbols in their
own section and use a separate 'struct namespaced_kernel_symbol' for
that section, at the cost of making the module loader more complex.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
On Tue, Aug 13, 2019 at 01:17:01PM +0100, Matthias Maennich wrote:
Add support for symbols that are exported into namespaces. For that,
extract any namespace suffix from the symbol name. In addition, emit a
warning whenever a module refers to an exported symbol without
explicitly importing the namespace that it is defined in. This patch
consistently adds the namespace suffix to symbol names exported into
Module.symvers.
Example warning emitted by modpost in case of the above violation:
WARNING: module ums-usbat uses symbol usb_stor_resume from namespace
USB_STORAGE, but does not import it.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Reviewed-by: Joel Fernandes (Google) <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
scripts/mod/modpost.c | 91 +++++++++++++++++++++++++++++++++++++------
scripts/mod/modpost.h | 7 ++++
2 files changed, 87 insertions(+), 11 deletions(-)
On Tue, Aug 13, 2019 at 01:17:03PM +0100, Matthias Maennich wrote:
To avoid excessive usage of EXPORT_SYMBOL_NS(sym, MY_NAMESPACE), where
MY_NAMESPACE will always be the namespace we are exporting to, allow
exporting all definitions of EXPORT_SYMBOL() and friends by defining
DEFAULT_SYMBOL_NAMESPACE.
For example, to export all symbols defined in usb-common into the
namespace USB_COMMON, add a line like this to drivers/usb/common/Makefile:
ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_COMMON
I thought we were trying to get away from cflags :(
That is equivalent to changing all EXPORT_SYMBOL(sym) definitions to
EXPORT_SYMBOL_NS(sym, USB_COMMON). Subsequently all symbol namespaces
functionality will apply.
Another way of making use of this feature is to define the namespace
within source or header files similar to how TRACE_SYSTEM defines are
used:
#undef DEFAULT_SYMBOL_NAMESPACE
#define DEFAULT_SYMBOL_NAMESPACE USB_COMMON
Please note that, as opposed to TRACE_SYSTEM, DEFAULT_SYMBOL_NAMESPACE
has to be defined before including include/linux/export.h.
If DEFAULT_SYMBOL_NAMESPACE is defined, a symbol can still be exported
to another namespace by using EXPORT_SYMBOL_NS() and friends with
explicitly specifying the namespace.
Ok, good, hopefully the cflags stuff will not be the default for people.
thanks,
greg k-h
On Tue, Aug 13, 2019 at 01:17:03PM +0100, Matthias Maennich wrote:
quoted hunk
To avoid excessive usage of EXPORT_SYMBOL_NS(sym, MY_NAMESPACE), where
MY_NAMESPACE will always be the namespace we are exporting to, allow
exporting all definitions of EXPORT_SYMBOL() and friends by defining
DEFAULT_SYMBOL_NAMESPACE.
For example, to export all symbols defined in usb-common into the
namespace USB_COMMON, add a line like this to drivers/usb/common/Makefile:
ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=USB_COMMON
That is equivalent to changing all EXPORT_SYMBOL(sym) definitions to
EXPORT_SYMBOL_NS(sym, USB_COMMON). Subsequently all symbol namespaces
functionality will apply.
Another way of making use of this feature is to define the namespace
within source or header files similar to how TRACE_SYSTEM defines are
used:
#undef DEFAULT_SYMBOL_NAMESPACE
#define DEFAULT_SYMBOL_NAMESPACE USB_COMMON
Please note that, as opposed to TRACE_SYSTEM, DEFAULT_SYMBOL_NAMESPACE
has to be defined before including include/linux/export.h.
If DEFAULT_SYMBOL_NAMESPACE is defined, a symbol can still be exported
to another namespace by using EXPORT_SYMBOL_NS() and friends with
explicitly specifying the namespace.
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
include/linux/export.h | 6 ++++++
1 file changed, 6 insertions(+)
On Tue, Aug 13, 2019 at 01:17:02PM +0100, Matthias Maennich wrote:
quoted hunk
If MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is enabled (default=n), the
requirement for modules to import all namespaces that are used by
the module is relaxed.
Enabling this option effectively allows (invalid) modules to be loaded
while only a warning is emitted.
Disabling this option keeps the enforcement at module loading time and
loading is denied if the module's imports are not satisfactory.
Reviewed-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
init/Kconfig | 14 ++++++++++++++
kernel/module.c | 11 +++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
@@ -2119,6 +2119,20 @@ config MODULE_COMPRESS_XZendchoice+configMODULE_ALLOW_MISSING_NAMESPACE_IMPORTS+bool"Allow loading of modules with missing namespace imports"+defaultn
the default for config options is always N, no need to list it here.
quoted hunk
+ help
+ Symbols exported with EXPORT_SYMBOL_NS*() are considered exported in
+ a namespace. A module that makes use of a symbol exported with such a
+ namespace is required to import the namespace via MODULE_IMPORT_NS().
+ This option relaxes this requirement when loading a module. While
+ technically there is no reason to enforce correct namespace imports,
+ it creates consistency between symbols defining namespaces and users
+ importing namespaces they make use of.
+
+ If unsure, say N.
+
config TRIM_UNUSED_KSYMS
bool "Trim unused exported kernel symbols"
depends on MODULES && !UNUSED_SYMBOLS
@@ -1408,9 +1408,16 @@ static int verify_namespace_is_imported(const struct load_info *info,imported_namespace=get_next_modinfo(info,"import_ns",imported_namespace);}-pr_err("%s: module uses symbol (%s) from namespace %s, but does not import it.\n",-mod->name,kernel_symbol_name(sym),namespace);+#ifdef CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS+pr_warn(+#else+pr_err(+#endif+"%s: module uses symbol (%s) from namespace %s, but does not import it.\n",+mod->name,kernel_symbol_name(sym),namespace);+#ifndef CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTSreturn-EINVAL;+#endif
This #ifdef mess is a hack, but oh well :)
If you drop the above default line, feel free to add:
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Aug 13, 2019 at 01:17:04PM +0100, Matthias Maennich wrote:
This patch adds an option to modpost to generate a <module>.ns_deps file
per module, containing the namespace dependencies for that module.
E.g. if the linked module my-module.ko would depend on the symbol
myfunc.MY_NS in the namespace MY_NS, the my-module.ns_deps file created
by modpost would contain the entry MY_NS to express the namespace
dependency of my-module imposed by using the symbol myfunc.
These files can subsequently be used by static analysis tools (like
coccinelle scripts) to address issues with missing namespace imports. A
later patch of this series will introduce such a script 'nsdeps' and a
corresponding make target to automatically add missing
MODULE_IMPORT_NS() definitions to the module's sources. For that it uses
the information provided in the generated .ns_deps files.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
scripts/mod/modpost.c | 61 +++++++++++++++++++++++++++++++++++++++----
scripts/mod/modpost.h | 2 ++
2 files changed, 58 insertions(+), 5 deletions(-)
On Tue, Aug 13, 2019 at 5:19 AM 'Matthias Maennich' via kernel-team
[off-list ref] wrote:
quoted hunk
If MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is enabled (default=n), the
requirement for modules to import all namespaces that are used by
the module is relaxed.
Enabling this option effectively allows (invalid) modules to be loaded
while only a warning is emitted.
Disabling this option keeps the enforcement at module loading time and
loading is denied if the module's imports are not satisfactory.
Reviewed-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
init/Kconfig | 14 ++++++++++++++
kernel/module.c | 11 +++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
@@ -2119,6 +2119,20 @@ config MODULE_COMPRESS_XZendchoice+configMODULE_ALLOW_MISSING_NAMESPACE_IMPORTS+bool"Allow loading of modules with missing namespace imports"+defaultn+help+SymbolsexportedwithEXPORT_SYMBOL_NS*()areconsideredexportedin+anamespace.Amodulethatmakesuseofasymbolexportedwithsucha+namespaceisrequiredtoimportthenamespaceviaMODULE_IMPORT_NS().+Thisoptionrelaxesthisrequirementwhenloadingamodule.
While
+ technically there is no reason to enforce correct namespace imports,
+ it creates consistency between symbols defining namespaces and users
+ importing namespaces they make use of.
I'm confused by this sentence. It sounds like it's the opposite of
what the config is doing? Can you please reword it for clarify?
-Saravana
On Tue, Aug 13, 2019 at 01:17:05PM +0100, Matthias Maennich wrote:
A script that uses the '<module>.ns_deps' file generated by modpost to
automatically add the required symbol namespace dependencies to each
module.
Usage:
1) Move some symbols to a namespace with EXPORT_SYMBOL_NS() or define
DEFAULT_SYMBOL_NAMESPACE
2) Run 'make' (or 'make modules') and get warnings about modules not
importing that namespace.
3) Run 'make nsdeps' to automatically add required import statements
to said modules.
This makes it easer for subsystem maintainers to introduce and maintain
symbol namespaces into their codebase.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
On Wed, Aug 14, 2019 at 12:06:11PM +0530, Himanshu Jha wrote:
On Tue, Aug 13, 2019 at 01:17:05PM +0100, Matthias Maennich wrote:
quoted
A script that uses the '<module>.ns_deps' file generated by modpost to
automatically add the required symbol namespace dependencies to each
module.
Usage:
1) Move some symbols to a namespace with EXPORT_SYMBOL_NS() or define
DEFAULT_SYMBOL_NAMESPACE
2) Run 'make' (or 'make modules') and get warnings about modules not
importing that namespace.
3) Run 'make nsdeps' to automatically add required import statements
to said modules.
This makes it easer for subsystem maintainers to introduce and maintain
symbol namespaces into their codebase.
Co-developed-by: Martijn Coenen <redacted>
Signed-off-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
From: Markus Elfring <hidden> Date: 2019-08-14 12:02:27
+# This script requires at least spatch
+# version 1.0.4.
How do you think about to avoid the duplicate specification of this identification?
Regards,
Markus
On Tue, Aug 13, 2019 at 01:15:44PM -0700, Saravana Kannan wrote:
On Tue, Aug 13, 2019 at 5:19 AM 'Matthias Maennich' via kernel-team
[off-list ref] wrote:
quoted
If MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is enabled (default=n), the
requirement for modules to import all namespaces that are used by
the module is relaxed.
Enabling this option effectively allows (invalid) modules to be loaded
while only a warning is emitted.
Disabling this option keeps the enforcement at module loading time and
loading is denied if the module's imports are not satisfactory.
Reviewed-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
init/Kconfig | 14 ++++++++++++++
kernel/module.c | 11 +++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
@@ -2119,6 +2119,20 @@ config MODULE_COMPRESS_XZendchoice+configMODULE_ALLOW_MISSING_NAMESPACE_IMPORTS+bool"Allow loading of modules with missing namespace imports"+defaultn+help+SymbolsexportedwithEXPORT_SYMBOL_NS*()areconsideredexportedin+anamespace.Amodulethatmakesuseofasymbolexportedwithsucha+namespaceisrequiredtoimportthenamespaceviaMODULE_IMPORT_NS().+Thisoptionrelaxesthisrequirementwhenloadingamodule.
quoted
While
+ technically there is no reason to enforce correct namespace imports,
+ it creates consistency between symbols defining namespaces and users
+ importing namespaces they make use of.
I'm confused by this sentence. It sounds like it's the opposite of
what the config is doing? Can you please reword it for clarify?
How about:
Symbols exported with EXPORT_SYMBOL_NS*() are considered exported in
a namespace. A module that makes use of a symbol exported with such a
namespace is required to import the namespace via MODULE_IMPORT_NS().
There is no technical reason to enforce correct namespace imports,
but it creates consistency between symbols defining namespaces and
users importing namespaces they make use of. This option relaxes this
requirement and lifts the enforcement when loading a module.
--
Cheers,
Matthias
On Wed, Aug 14, 2019 at 5:54 AM 'Matthias Maennich' via kernel-team
[off-list ref] wrote:
On Tue, Aug 13, 2019 at 01:15:44PM -0700, Saravana Kannan wrote:
quoted
On Tue, Aug 13, 2019 at 5:19 AM 'Matthias Maennich' via kernel-team
[off-list ref] wrote:
quoted
If MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS is enabled (default=n), the
requirement for modules to import all namespaces that are used by
the module is relaxed.
Enabling this option effectively allows (invalid) modules to be loaded
while only a warning is emitted.
Disabling this option keeps the enforcement at module loading time and
loading is denied if the module's imports are not satisfactory.
Reviewed-by: Martijn Coenen <redacted>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
init/Kconfig | 14 ++++++++++++++
kernel/module.c | 11 +++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
@@ -2119,6 +2119,20 @@ config MODULE_COMPRESS_XZendchoice+configMODULE_ALLOW_MISSING_NAMESPACE_IMPORTS+bool"Allow loading of modules with missing namespace imports"+defaultn+help+SymbolsexportedwithEXPORT_SYMBOL_NS*()areconsideredexportedin+anamespace.Amodulethatmakesuseofasymbolexportedwithsucha+namespaceisrequiredtoimportthenamespaceviaMODULE_IMPORT_NS().+Thisoptionrelaxesthisrequirementwhenloadingamodule.
quoted
While
+ technically there is no reason to enforce correct namespace imports,
+ it creates consistency between symbols defining namespaces and users
+ importing namespaces they make use of.
I'm confused by this sentence. It sounds like it's the opposite of
what the config is doing? Can you please reword it for clarify?
How about:
Symbols exported with EXPORT_SYMBOL_NS*() are considered exported in
a namespace. A module that makes use of a symbol exported with such a
namespace is required to import the namespace via MODULE_IMPORT_NS().
There is no technical reason to enforce correct namespace imports,
but it creates consistency between symbols defining namespaces and
users importing namespaces they make use of. This option relaxes this
requirement and lifts the enforcement when loading a module.
That's a lot better. Especially moving the "This option relaxes..." to
the bottom. Thanks.
-Saravana
* Where will the variable “srctree” be set for the file “scripts/nsdeps”?
* Would you like to support a separate build directory for desired adjustments?
* How do you think about to check error handling around such commands?
+generate_deps() {
…
+ for source_file in $mod_source_files; do
+ sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp
…
I suggest to assign the name for the temporary file to a variable
which should be used by subsequent commands.
Regards,
Markus
* Where will the variable “srctree” be set for the file “scripts/nsdeps”?
$srctree is defined by kbuild in the toplevel Makefile.
* Would you like to support a separate build directory for desired adjustments?
No, as the purpose of this script is to directly patch the kernel
sources where applicable.
* How do you think about to check error handling around such commands?
spatch emits a descriptive message on error. I will add a 'set
-e' to the script so that it aborts on errors.
quoted
+generate_deps() {
…
quoted
+ for source_file in $mod_source_files; do
+ sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp
…
I suggest to assign the name for the temporary file to a variable
which should be used by subsequent commands.
I somehow don't agree that this is an improvement to the code as the
variable would likely be something like ${source_file_tmp}. Sticking to
${source_file}.tmp does express the intent of a temporary file next to
the original source file and the reader of the code does not need to
reason about the value of ${source_file_tmp}.
Cheers,
Matthias
From: Markus Elfring <hidden> Date: 2019-08-22 11:02:40
$srctree is defined by kbuild in the toplevel Makefile.
How is this variable passed to the file “scripts/nsdeps”?
quoted
* Would you like to support a separate build directory for desired adjustments?
No, as the purpose of this script is to directly patch the kernel
sources where applicable.
Will there occasionally be a need to provide a generated patch
(without in-place file modification)?
quoted
I suggest to assign the name for the temporary file to a variable
which should be used by subsequent commands.
I somehow don't agree that this is an improvement to the code as the
variable would likely be something like ${source_file_tmp}.
Would you dare to choose a shorter variable name?
${source_file}.tmp does express the intent of a temporary file next to
the original source file and the reader of the code does not need to
reason about the value of ${source_file_tmp}.
I would find a code variant with less suffix repetition nicer.
Regards,
Markus