From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7B85BC67790 for ; Wed, 25 Jul 2018 16:48:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 10AB620843 for ; Wed, 25 Jul 2018 16:48:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="P5CFusir" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 10AB620843 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729241AbeGYSBO (ORCPT ); Wed, 25 Jul 2018 14:01:14 -0400 Received: from mail-wr1-f66.google.com ([209.85.221.66]:43267 "EHLO mail-wr1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728741AbeGYSBO (ORCPT ); Wed, 25 Jul 2018 14:01:14 -0400 Received: by mail-wr1-f66.google.com with SMTP id b15-v6so8034505wrv.10; Wed, 25 Jul 2018 09:48:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fv3uIYajyzS4idh5oidtO1VSbmP2mjgcY3ULspz4SEQ=; b=P5CFusirJFQhqXANSuzpUdXDgu/j/SEJfMR1sB0+y3/L2UR62bh4kd8l/TaZtmL8Cx AkMX84AilTX9x0+gGR5+jMvgGjtVVkfRDRF8apyAFf9mZ2YH9pIL4+LiKSeOzW9WhsgP 2F+7SbpENPtAOfXxHhl8QsFe+236gTbaPW2xBn2b+TVtskhBu2TFYhiolejGNW+PovVn aScXn/ADgj1mPqFzmSFFbMiV+vjFp/XlTTUeKfFMkXemNMjD62wVqbpoPZb4wp+c7F05 KCxmdXJClGMDKMAxhG0tVVOKt5KgypuRbWpaGAp7UBJKRPTAg/TjxmAWd8X4D34enMEQ NVlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=fv3uIYajyzS4idh5oidtO1VSbmP2mjgcY3ULspz4SEQ=; b=IRW0QQh29trzr8FPc09fk3o+iqWTp3Hmey3ystJv2pRl6/iP76VBUHSjupOrGkfjb/ RpNP5aEUpegmvQsp1mVyAuX4mVw1kNBzJ24ExM4Q11nD5ls7ui5Ri3xWTGLwHj79qy6E 8v9SpAiwJewFJTy9ZLRz1HZb+K24pdnwP1jJVIK+FDMmqr1zaHAjWscxN7oNy9mZqvfy 8N25/Szr7Y3MUwY3XFyWBG/kEJcT+nQUBvq8lNn5CNrKkqkPjMw4bx8Hzhvm/cqKgqzt F6Lnq0lOEMc4o1TFXO0sokoMvqSy7jJsqkoNi2x8oaZEV5LvapVdBdgM/rQj+nGBnOtT qUXw== X-Gm-Message-State: AOUpUlE3nxZ3yVROzW7v/poNZ02q4JYYlLHRH8TH9R6Khf2FnTY8Desq 64Ru++sMNi8bE+SDFvoT5G1u5uI7WXuCnW78fvw= X-Google-Smtp-Source: AAOMgpdWoVTpP8Um5VaWKSgBXFqsjJGE4CsI25wzGS5SnQ1RVsJYPMoY42M1kvwxWSuoiItrKhY+WYfHkKLsbuAaIwU= X-Received: by 2002:adf:afd3:: with SMTP id y19-v6mr16433580wrd.176.1532537320879; Wed, 25 Jul 2018 09:48:40 -0700 (PDT) MIME-Version: 1.0 References: <20180716122125.175792-1-maco@android.com> <20180716122125.175792-3-maco@android.com> <20180725155507.umb5oatduquxwlmt@linux-8ccs> In-Reply-To: <20180725155507.umb5oatduquxwlmt@linux-8ccs> From: Lucas De Marchi Date: Wed, 25 Jul 2018 09:48:29 -0700 Message-ID: Subject: Re: [PATCH 2/6] module: add support for symbol namespaces. To: Jessica Yu Cc: maco@android.com, lkml , yamada.masahiro@socionext.com, michal.lkml@markovi.net, geert@linux-m68k.org, Thomas Gleixner , mingo@redhat.com, "H. Peter Anvin" , x86@kernel.org, Alan Stern , Greg KH , oneukum@suse.com, Arnd Bergmann , sboyd@codeaurora.org, pombredanne@nexb.com, kstewart@linuxfoundation.org, sam@ravnborg.org, linux-kbuild@vger.kernel.org, linux-m68k@lists.linux-m68k.org, linux-usb@vger.kernel.org, usb-storage@lists.one-eyed-alien.net, linux-scsi@vger.kernel.org, linux-arch@vger.kernel.org, maco@google.com, sspatil@google.com, malchev@google.com, joelaf@google.com, linux-modules Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 25, 2018 at 8:55 AM Jessica Yu wrote: > > +++ Martijn Coenen [24/07/18 09:56 +0200]: > >I did find an issue with my approach: > > > >On Mon, Jul 16, 2018 at 2:21 PM, Martijn Coenen wrote: > >> 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 > >> reloction records just to get to the struct kernel_symbols. > > > >depmod doesn't like this: if namespaced symbols are built-in to the > >kernel, they will appear as 'symbol.NS' in the symbol table, whereas > >modules using those symbols just depend on 'symbol'. This will cause > >depmod to warn about unknown symbols. I didn't find this previously > >because all the exports/imports I tested were done from modules > >themselves. > > > >One way to deal with it is to change depmod, but it looks like it > >hasn't been changed in ages, and it would also introduce a dependency this might be because you are looking to the wrong project (module-init-tools) rather than kmod that replaced it some years ago? > >on userspaces to update it to avoid these warnings. So, we'd have to > >encode the symbol namespace information in another way for modpost to > >use. I could just write a separate post-processing tool (much like > >genksyms), or perhaps encode the information in a discardable section. > >Any other suggestions welcome. > > This seems to be because depmod uses System.map as a source for kernel > symbols and scans for only the ones prefixed with "__ksymtab" to find > the exported ones - and those happen to use the '.' to mark the > namespace it belongs to. It strips that prefix and uses the remainder > as the actual symbol name. To fix that it'd have to strip the > namespace suffix in the symbol name as well. > > Just scanning the depmod source code, it seems really trivial to just > have depmod accommodate the new symbol name format. Adding Lucas (kmod > maintainer) to CC. Yep, that would be easy and allow depmod to be backward compatible since we would do nothing if the symbol doesn't have the suffix. As for dependency on a new version, this seems trivial enough to be backported to previous releases used on distros so even if they are not rolling they would get a compatible depmod. CC'ing linux-modules@vger.kernel.org to keep track of this there thanks Lucas De Marchi > > Thanks, > > Jessica > > > > >> > >> 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. > >> > >> Signed-off-by: Martijn Coenen > >> --- > >> include/asm-generic/export.h | 2 +- > >> include/linux/export.h | 83 +++++++++++++++++++++++++++--------- > >> include/linux/module.h | 13 ++++++ > >> kernel/module.c | 79 ++++++++++++++++++++++++++++++++++ > >> 4 files changed, 156 insertions(+), 21 deletions(-) > >> > >> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h > >> index 68efb950a918..4c3d1afb702f 100644 > >> --- a/include/asm-generic/export.h > >> +++ b/include/asm-generic/export.h > >> @@ -29,7 +29,7 @@ > >> .section ___ksymtab\sec+\name,"a" > >> .balign KSYM_ALIGN > >> __ksymtab_\name: > >> - __put \val, __kstrtab_\name > >> + __put \val, __kstrtab_\name, 0 > >> .previous > >> .section __ksymtab_strings,"a" > >> __kstrtab_\name: > >> diff --git a/include/linux/export.h b/include/linux/export.h > >> index ad6b8e697b27..9f6e70eeb85f 100644 > >> --- a/include/linux/export.h > >> +++ b/include/linux/export.h > >> @@ -22,6 +22,11 @@ struct kernel_symbol > >> { > >> unsigned long value; > >> const char *name; > >> + const char *namespace; > >> +}; > >> + > >> +struct namespace_import { > >> + const char *namespace; > >> }; > >> > >> #ifdef MODULE > >> @@ -54,18 +59,28 @@ extern struct module __this_module; > >> #define __CRC_SYMBOL(sym, sec) > >> #endif > >> > >> +#define NS_SEPARATOR "." > >> + > >> +#define MODULE_IMPORT_NS(ns) \ > >> + static const struct namespace_import __knsimport_##ns \ > >> + asm("__knsimport_" #ns) \ > >> + __attribute__((section("__knsimport"), used)) \ > >> + = { #ns } > >> + > >> /* For every exported symbol, place a struct in the __ksymtab section */ > >> -#define ___EXPORT_SYMBOL(sym, sec) \ > >> +#define ___EXPORT_SYMBOL(sym, sec, ns, nspost, nspost2) \ > >> extern typeof(sym) sym; \ > >> __CRC_SYMBOL(sym, sec) \ > >> - static const char __kstrtab_##sym[] \ > >> + static const char __kstrtab_##sym##nspost[] \ > >> __attribute__((section("__ksymtab_strings"), aligned(1))) \ > >> = #sym; \ > >> - static const struct kernel_symbol __ksymtab_##sym \ > >> + static const struct kernel_symbol __ksymtab_##sym##nspost \ > >> + asm("__ksymtab_" #sym nspost2) \ > >> __used \ > >> - __attribute__((section("___ksymtab" sec "+" #sym), used)) \ > >> + __attribute__((section("___ksymtab" sec "+" #sym #nspost))) \ > >> + __attribute__((used)) \ > >> __attribute__((aligned(sizeof(void *)))) \ > >> - = { (unsigned long)&sym, __kstrtab_##sym } > >> + = { (unsigned long)&sym, __kstrtab_##sym##nspost, ns } > >> > >> #if defined(__KSYM_DEPS__) > >> > >> @@ -76,52 +91,80 @@ extern struct module __this_module; > >> * system filters out from the preprocessor output (see ksym_dep_filter > >> * in scripts/Kbuild.include). > >> */ > >> -#define __EXPORT_SYMBOL(sym, sec) === __KSYM_##sym === > >> +#define __EXPORT_SYMBOL(sym, sec, ns, nspost, nspost2) === __KSYM_##sym === > >> > >> #elif defined(CONFIG_TRIM_UNUSED_KSYMS) > >> > >> #include > >> > >> -#define __EXPORT_SYMBOL(sym, sec) \ > >> - __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(sym, sec, ns, nspost, nspost2) \ > >> + __cond_export_sym(sym, sec, ns, nspost, nspost2, \ > >> + __is_defined(__KSYM_##sym)) > >> +#define __cond_export_sym(sym, sec, ns, nspost, nspost2, conf) \ > >> + ___cond_export_sym(sym, sec, ns, nspost, nspost2, conf) > >> +#define ___cond_export_sym(sym, sec, ns, nspost, nspost2, enabled) \ > >> + __cond_export_sym_##enabled(sym, sec, ns, nspost, nspost2) > >> +#define __cond_export_sym_1(sym, sec, ns, nspost, nspost2) \ > >> + ___EXPORT_SYMBOL(sym, sec, ns, nspost, nspost2) > >> +#define __cond_export_sym_0(sym, sec, ns, nspost, nspost2) /* nothing */ > >> > >> #else > >> #define __EXPORT_SYMBOL ___EXPORT_SYMBOL > >> #endif > >> > >> #define EXPORT_SYMBOL(sym) \ > >> - __EXPORT_SYMBOL(sym, "") > >> + __EXPORT_SYMBOL(sym, "", NULL, ,) > >> > >> #define EXPORT_SYMBOL_GPL(sym) \ > >> - __EXPORT_SYMBOL(sym, "_gpl") > >> + __EXPORT_SYMBOL(sym, "_gpl", NULL, ,) > >> > >> #define EXPORT_SYMBOL_GPL_FUTURE(sym) \ > >> - __EXPORT_SYMBOL(sym, "_gpl_future") > >> + __EXPORT_SYMBOL(sym, "_gpl_future", NULL, ,) > >> + > >> +#define EXPORT_SYMBOL_NS(sym, ns) \ > >> + __EXPORT_SYMBOL(sym, "", #ns, __##ns, NS_SEPARATOR #ns) > >> + > >> +#define EXPORT_SYMBOL_NS_GPL(sym, ns) \ > >> + __EXPORT_SYMBOL(sym, "_gpl", #ns, __##ns, NS_SEPARATOR #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) > >> #endif > >> > >> -#endif /* __GENKSYMS__ */ > >> +#endif /* __KERNEL__ && !__GENKSYMS__ */ > >> + > >> +#if defined(__GENKSYMS__) > >> +/* > >> + * When we're running genksyms, ignore the namespace and make the _NS > >> + * variants look like the normal ones. There are two reasons for this: > >> + * 1) In the normal definition of EXPORT_SYMBOL_NS, the 'ns' macro > >> + * argument is itself not expanded because it's always tokenized or > >> + * concatenated; but when running genksyms, a blank definition of the > >> + * macro does allow the argument to be expanded; if a namespace > >> + * happens to collide with a #define, this can cause issues. > >> + * 2) There's no need to modify genksyms to deal with the _NS variants > >> + */ > >> +#define EXPORT_SYMBOL_NS(sym, ns) \ > >> + EXPORT_SYMBOL(sym) > >> +#define EXPORT_SYMBOL_NS_GPL(sym, ns) \ > >> + EXPORT_SYMBOL_GPL(sym) > >> +#endif > >> > >> #else /* !CONFIG_MODULES... */ > >> > >> #define EXPORT_SYMBOL(sym) > >> +#define EXPORT_SYMBOL_NS(sym, ns) > >> +#define EXPORT_SYMBOL_NS_GPL(sym, ns) > >> #define EXPORT_SYMBOL_GPL(sym) > >> #define EXPORT_SYMBOL_GPL_FUTURE(sym) > >> #define EXPORT_UNUSED_SYMBOL(sym) > >> #define EXPORT_UNUSED_SYMBOL_GPL(sym) > >> > >> +#define MODULE_IMPORT_NS(ns) > >> #endif /* CONFIG_MODULES */ > >> #endif /* !__ASSEMBLY__ */ > >> > >> diff --git a/include/linux/module.h b/include/linux/module.h > >> index d44df9b2c131..afab4e8fa188 100644 > >> --- a/include/linux/module.h > >> +++ b/include/linux/module.h > >> @@ -268,6 +268,12 @@ void *__symbol_get(const char *symbol); > >> void *__symbol_get_gpl(const char *symbol); > >> #define symbol_get(x) ((typeof(&x))(__symbol_get(VMLINUX_SYMBOL_STR(x)))) > >> > >> +/* namespace dependencies of the module */ > >> +struct module_ns_dep { > >> + struct list_head ns_dep; > >> + const char *namespace; > >> +}; > >> + > >> /* modules using other modules: kdb wants to see this. */ > >> struct module_use { > >> struct list_head source_list; > >> @@ -359,6 +365,13 @@ struct module { > >> const struct kernel_symbol *gpl_syms; > >> const s32 *gpl_crcs; > >> > >> + /* Namespace imports */ > >> + unsigned int num_ns_imports; > >> + const struct namespace_import *ns_imports; > >> + > >> + /* Namespace dependencies */ > >> + struct list_head ns_dependencies; > >> + > >> #ifdef CONFIG_UNUSED_SYMBOLS > >> /* unused exported symbols. */ > >> const struct kernel_symbol *unused_syms; > >> diff --git a/kernel/module.c b/kernel/module.c > >> index f475f30eed8c..63de0fe849f9 100644 > >> --- a/kernel/module.c > >> +++ b/kernel/module.c > >> @@ -1166,6 +1166,51 @@ static inline int module_unload_init(struct module *mod) > >> } > >> #endif /* CONFIG_MODULE_UNLOAD */ > >> > >> +static bool module_has_ns_dependency(struct module *mod, const char *ns) > >> +{ > >> + struct module_ns_dep *ns_dep; > >> + > >> + list_for_each_entry(ns_dep, &mod->ns_dependencies, ns_dep) { > >> + if (strcmp(ns_dep->namespace, ns) == 0) > >> + return true; > >> + } > >> + > >> + return false; > >> +} > >> + > >> +static int add_module_ns_dependency(struct module *mod, const char *ns) > >> +{ > >> + struct module_ns_dep *ns_dep; > >> + > >> + if (module_has_ns_dependency(mod, ns)) > >> + return 0; > >> + > >> + ns_dep = kmalloc(sizeof(*ns_dep), GFP_ATOMIC); > >> + if (!ns_dep) > >> + return -ENOMEM; > >> + > >> + ns_dep->namespace = ns; > >> + > >> + list_add(&ns_dep->ns_dep, &mod->ns_dependencies); > >> + > >> + return 0; > >> +} > >> + > >> +static bool module_imports_ns(struct module *mod, const char *ns) > >> +{ > >> + size_t i; > >> + > >> + if (!ns) > >> + return true; > >> + > >> + for (i = 0; i < mod->num_ns_imports; ++i) { > >> + if (!strcmp(mod->ns_imports[i].namespace, ns)) > >> + return true; > >> + } > >> + > >> + return false; > >> +} > >> + > >> static size_t module_flags_taint(struct module *mod, char *buf) > >> { > >> size_t l = 0; > >> @@ -1415,6 +1460,18 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod, > >> goto getname; > >> } > >> > >> + /* > >> + * We can't yet verify that the module actually imports this > >> + * namespace, because the imports themselves are only available > >> + * after processing the symbol table and doing relocation; so > >> + * instead just record the dependency and check later. > >> + */ > >> + if (sym->namespace) { > >> + err = add_module_ns_dependency(mod, sym->namespace); > >> + if (err) > >> + sym = ERR_PTR(err); > >> + } > >> + > >> 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 > >> -- Lucas De Marchi