+++ Martijn Coenen [20/07/18 09:54 +0200]:
Hi Jessica,
On Thu, Jul 19, 2018 at 6:32 PM, Jessica Yu <jeyu@xxxxxxxxxx> wrote:
First, thanks a lot for the patchset. This is definitely something that
will help distros as well to declutter the exported symbol space and
provide a more cleanly defined kernel interface.
Thanks, that's good to hear!
Hm, I'm wondering if we could avoid all this extra module-ns-dependency
checking work if we just put the import stuff as a modinfo tag, like
have MODULE_IMPORT_NS() insert an "import:" tag like we already do for
module parameters, author, license, aliases, etc. Then we'd have that
information pretty much from the start of load_module() and we don't
need to wait for relocations to be applied to __knsimport. Then you
could do the namespace checking right at resolve_symbol() instead of
going through the extra step of building a dependency list to be
verified later.
I believe I did consider modinfo when I started with this a while
back, but don't recall right now why I didn't decide to use it;
perhaps it was the lack of support for multiple identical tags. Since
both modpost and the module loader have access to them, I don't see
why it wouldn't work, and indeed I suspect it would make the module
loader a bit simpler. I'll rework this and see what it looks like.
Thanks. Also, it looks like we're currently just warning (in both
modpost and on module load) if a module uses symbols from a namespace
it doesn't import. Are you also planning to eventually introduce
namespace enforcement? It'd be trivial to fail the module build in
modpost as well as reject the module on load if it uses an exported
symbol belonging to a namespace it doesn't import. Although, I'd go
with the warnings for a development cycle or two, to gently introduce
the change in API and let other subsystems as well as out-of-tree
module developers catch up.
Jessica
Also, this would get rid of the extra __knsimport section, the extra
ns_dependencies field in struct module, and all those helper functions that
manage it. In addition, having the modinfo tag may potentially help with
debugging as we have the namespace imports clearly listed if we don't have
the source code for a module. We'd probably need to modify get_modinfo() to
handle multiple import: tags though. Luis [1] had written some code a while
ago to handle multiple (of the same) modinfo tags.
Thoughts on this?
Thanks,
Jessica
[1] https://lkml.kernel.org/r/20171130023605.29568-3-mcgrof@xxxxxxxxxx
getname:
/* We must make copy under the lock if we failed to get ref. */
strncpy(ownername, module_name(owner), MODULE_NAME_LEN);
@@ -3061,6 +3118,11 @@ static int find_module_sections(struct module *mod,
struct load_info *info)
sizeof(*mod->gpl_syms),
&mod->num_gpl_syms);
mod->gpl_crcs = section_addr(info, "__kcrctab_gpl");
+
+ mod->ns_imports = section_objs(info, "__knsimport",
+ sizeof(*mod->ns_imports),
+ &mod->num_ns_imports);
+
mod->gpl_future_syms = section_objs(info,
"__ksymtab_gpl_future",
sizeof(*mod->gpl_future_syms),
@@ -3381,6 +3443,19 @@ static int post_relocation(struct module *mod,
const struct load_info *info)
return module_finalize(info->hdr, info->sechdrs, mod);
}
+static void verify_namespace_dependencies(struct module *mod)
+{
+ struct module_ns_dep *ns_dep;
+
+ list_for_each_entry(ns_dep, &mod->ns_dependencies, ns_dep) {
+ if (!module_imports_ns(mod, ns_dep->namespace)) {
+ pr_warn("%s: module uses symbols from namespace
%s,"
+ " but does not import it.\n",
+ mod->name, ns_dep->namespace);
+ }
+ }
+}
+
/* Is this module of this name done loading? No locks held. */
static bool finished_loading(const char *name)
{
@@ -3682,6 +3757,8 @@ static int load_module(struct load_info *info, const
char __user *uargs,
if (err)
goto free_module;
+ INIT_LIST_HEAD(&mod->ns_dependencies);
+
#ifdef CONFIG_MODULE_SIG
mod->sig_ok = info->sig_ok;
if (!mod->sig_ok) {
@@ -3730,6 +3807,8 @@ static int load_module(struct load_info *info, const
char __user *uargs,
if (err < 0)
goto free_modinfo;
+ verify_namespace_dependencies(mod);
+
flush_module_icache(mod);
/* Now copy in args */
--
2.18.0.203.gfac676dfb9-goog
