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=-3.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_PASS,T_DKIMWL_WL_HIGH,USER_AGENT_NEOMUTT 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 BC0E8ECDFB8 for ; Fri, 20 Jul 2018 14:49:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 65F96206B7 for ; Fri, 20 Jul 2018 14:49:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="FPPClxwT" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 65F96206B7 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org 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 S1732163AbeGTPiB (ORCPT ); Fri, 20 Jul 2018 11:38:01 -0400 Received: from mail.kernel.org ([198.145.29.99]:57422 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731314AbeGTPiB (ORCPT ); Fri, 20 Jul 2018 11:38:01 -0400 Received: from linux-8ccs (nat.nue.novell.com [195.135.221.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id C6D8D20661; Fri, 20 Jul 2018 14:49:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1532098162; bh=C6PPGI7+rfksOAEL7bp56W+kY9aPEF9hLNt6IMyDdGo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=FPPClxwTJNq0tTm9lNUpwqcGkQKjXfemwJJ7F0HW9MTbbbiJULQcDkbkjTwsbxyqq akFpUQQ4Ggq7jaFwgw0K4O5QKCTY+6yOZRH6wAPpZ5keaHMs1a011LPxE5guOBqddp d59iFgrMKhXhcWeGp/ydS98Q/mUrw5/WQU9MyUs8= Date: Fri, 20 Jul 2018 16:49:16 +0200 From: Jessica Yu To: Martijn Coenen Cc: LKML , Masahiro Yamada , Michal Marek , Geert Uytterhoeven , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , the arch/x86 maintainers , Alan Stern , Greg Kroah-Hartman , Oliver Neukum , Arnd Bergmann , Stephen Boyd , Philippe Ombredanne , Kate Stewart , Sam Ravnborg , linux-kbuild@vger.kernel.org, linux-m68k , USB list , USB Storage list , linux-scsi@vger.kernel.org, Linux-Arch , Martijn Coenen , Sandeep Patil , Iliyan Malchev , Joel Fernandes Subject: Re: [PATCH 2/6] module: add support for symbol namespaces. Message-ID: <20180720144916.fyqpmrtgt23sax6n@linux-8ccs> References: <20180716122125.175792-1-maco@android.com> <20180716122125.175792-3-maco@android.com> <20180719163208.5xvrafugpcbhh7kj@linux-8ccs> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: X-OS: Linux linux-8ccs 4.12.14-lp150.11-default x86_64 User-Agent: NeoMutt/20170912 (1.9.0) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ Martijn Coenen [20/07/18 09:54 +0200]: >Hi Jessica, > >On Thu, Jul 19, 2018 at 6:32 PM, Jessica Yu 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@kernel.org >> >> >>> 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 >>> >>