Re: [PATCH 2/6] module: add support for symbol namespaces.
From: Martijn Coenen
Date: Fri Jul 20 2018 - 03:54:21 EST
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,
Martijn
>
> 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
>>
>