From: Ben Hutchings <ben@decadent.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>,
Michal Marek <mmarek@suse.com>
Cc: Adam Borowski <kilobyte@angband.pl>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
Debian kernel maintainers <debian-kernel@lists.debian.org>,
"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
Arnd Bergmann <arnd@arndb.de>, Ingo Molnar <mingo@kernel.org>,
Nicholas Piggin <npiggin@gmail.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm
Date: Tue, 29 Nov 2016 19:57:21 +0000 [thread overview]
Message-ID: <20161129195721.GI2697@decadent.org.uk> (raw)
In-Reply-To: <CA+55aFxOGw57Gm-QiNs13kfy5k=aRrn3+98415V0g73eGc66mg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7361 bytes --]
On Tue, 2016-11-29 at 08:17 -0800, Linus Torvalds wrote:
> > On Tue, Nov 29, 2016 at 8:03 AM, Michal Marek <mmarek@suse.com> wrote:
> >
> > The original and easily observable bug is that were are not generating
> > symbol checksums for the asm-exported symbols, so they default to 0.
> > This can be seen e.g. in the Module.symvers file. This seemed like a
> > minor issue, because with the functions written in asm, the type
> > checking is rather weak (this has been the case even before Al's
> > patches). However, there is another bug that with _some_ toolchains /
> > architectures, the checksums do not default to 0, but they are simply
> > missing in the ___kcrctab* sections and the module loader complains. We
> > can of course research into the details of the second bug, but we
> > already know that we are not generating the checksums while we should be.
>
> So let's just say that "toolchain is buggy" and make a missing kcrctab
> entry mean zero (or mean "matches anything"). And just shut up the
> warning.
>
> I do *not* want to add random bandaids for something like a broken
> toolchain issue when I'd really rather just delete the feature.
If the modversion is missing then the fallback should be to a full
vermagic match, i.e. including the release string. Something like
this (untested):
diff --git a/init/Kconfig b/init/Kconfig
index c4fbc1e55c25..34407f15e6d3 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1945,7 +1945,6 @@ config MODULE_FORCE_UNLOAD
config MODVERSIONS
bool "Module versioning support"
- depends on BROKEN
help
Usually, you have to use modules compiled with your kernel.
Saying Y here makes it sometimes possible to use modules
diff --git a/kernel/module.c b/kernel/module.c
index f57dd63186e6..78d61ae50bc5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -296,6 +296,12 @@ int unregister_module_notifier(struct notifier_block *nb)
}
EXPORT_SYMBOL(unregister_module_notifier);
+enum {
+ MAGIC_NO_MATCH,
+ MAGIC_MATCH_NEED_CRC,
+ MAGIC_MATCH_EXACT
+};
+
struct load_info {
Elf_Ehdr *hdr;
unsigned long len;
@@ -305,6 +311,7 @@ struct load_info {
struct _ddebug *debug;
unsigned int num_debug;
bool sig_ok;
+ int magic_match;
#ifdef CONFIG_KALLSYMS
unsigned long mod_kallsyms_init_off;
#endif
@@ -1268,13 +1275,14 @@ static unsigned long maybe_relocated(unsigned long crc,
return crc;
}
-static int check_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static int check_version(const struct load_info *info,
const char *symname,
struct module *mod,
const unsigned long *crc,
const struct module *crc_owner)
{
+ Elf_Shdr *sechdrs = info->sechdrs;
+ unsigned int versindex = info->index.vers;
unsigned int i, num_versions;
struct modversion_info *versions;
@@ -1294,6 +1302,10 @@ static int check_version(Elf_Shdr *sechdrs,
if (strcmp(versions[i].name, symname) != 0)
continue;
+ /* Ignore dummy zero CRC */
+ if (versions[i].crc == 0)
+ break;
+
if (versions[i].crc == maybe_relocated(*crc, crc_owner))
return 1;
pr_debug("Found checksum %lX vs module %lX\n",
@@ -1301,6 +1313,9 @@ static int check_version(Elf_Shdr *sechdrs,
goto bad_version;
}
+ if (info->magic_match == MAGIC_MATCH_EXACT)
+ return 1;
+
pr_warn("%s: no symbol version for %s\n", mod->name, symname);
return 0;
@@ -1310,8 +1325,7 @@ static int check_version(Elf_Shdr *sechdrs,
return 0;
}
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static inline int check_modstruct_version(const struct load_info *info,
struct module *mod)
{
const unsigned long *crc;
@@ -1327,24 +1341,24 @@ static inline int check_modstruct_version(Elf_Shdr *sechdrs,
BUG();
}
preempt_enable();
- return check_version(sechdrs, versindex,
+ return check_version(info,
VMLINUX_SYMBOL_STR(module_layout), mod, crc,
NULL);
}
-/* First part is kernel version, which we ignore if module has crcs. */
-static inline int same_magic(const char *amagic, const char *bmagic,
- bool has_crcs)
+/* First part is kernel version, which can be ignored if module has crcs. */
+static inline int compare_magic(const char *amagic, const char *bmagic)
{
- if (has_crcs) {
- amagic += strcspn(amagic, " ");
- bmagic += strcspn(bmagic, " ");
- }
- return strcmp(amagic, bmagic) == 0;
+ if (strcmp(amagic, bmagic) == 0)
+ return MAGIC_MATCH_EXACT;
+
+ amagic += strcspn(amagic, " ");
+ bmagic += strcspn(bmagic, " ");
+ return strcmp(amagic, bmagic) == 0 ? MAGIC_MATCH_NEED_CRC : MAGIC_NO_MATCH;
}
+
#else
-static inline int check_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static inline int check_version(const struct load_info *info,
const char *symname,
struct module *mod,
const unsigned long *crc,
@@ -1353,17 +1367,15 @@ static inline int check_version(Elf_Shdr *sechdrs,
return 1;
}
-static inline int check_modstruct_version(Elf_Shdr *sechdrs,
- unsigned int versindex,
+static inline int check_modstruct_version(const struct load_info *info,
struct module *mod)
{
return 1;
}
-static inline int same_magic(const char *amagic, const char *bmagic,
- bool has_crcs)
+static inline int compare_magic(const char *amagic, const char *bmagic)
{
- return strcmp(amagic, bmagic) == 0;
+ return strcmp(amagic, bmagic) == 0 ? MAGIC_MATCH_EXACT : MAGIC_NO_MATCH;
}
#endif /* CONFIG_MODVERSIONS */
@@ -1390,8 +1402,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
if (!sym)
goto unlock;
- if (!check_version(info->sechdrs, info->index.vers, name, mod, crc,
- owner)) {
+ if (!check_version(info, name, mod, crc, owner)) {
sym = ERR_PTR(-EINVAL);
goto getname;
}
@@ -2936,7 +2947,7 @@ static struct module *setup_load_info(struct load_info *info, int flags)
info->index.pcpu = find_pcpusec(info);
/* Check module struct version now, before we try to use module. */
- if (!check_modstruct_version(info->sechdrs, info->index.vers, mod))
+ if (!check_modstruct_version(info, mod))
return ERR_PTR(-ENOEXEC);
return mod;
@@ -2952,13 +2963,19 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
/* This is allowed: modprobe --force will invalidate it. */
if (!modmagic) {
+ info->magic_match = MAGIC_NO_MATCH;
err = try_to_force_load(mod, "bad vermagic");
if (err)
return err;
- } else if (!same_magic(modmagic, vermagic, info->index.vers)) {
- pr_err("%s: version magic '%s' should be '%s'\n",
- mod->name, modmagic, vermagic);
- return -ENOEXEC;
+ } else {
+ info->magic_match = compare_magic(modmagic, vermagic);
+ if (info->magic_match == MAGIC_NO_MATCH ||
+ (info->magic_match == MAGIC_MATCH_NEED_CRC &&
+ !info->index.vers)) {
+ pr_err("%s: version magic '%s' should be '%s'\n",
+ mod->name, modmagic, vermagic);
+ return -ENOEXEC;
+ }
}
if (!get_modinfo(info, "intree")) {
--- END ---
Ben.
--
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of comp.compilers
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
next prev parent reply other threads:[~2016-11-29 19:57 UTC|newest]
Thread overview: 113+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <a73aec83-ddad-2bdf-e612-178c9936a16f@manjaro.org>
[not found] ` <20161102004639.6870806d@roar.ozlabs.ibm.com>
2016-11-23 20:08 ` BUG: 4.9-rc6 Still "no symbol version" on boot Philip Müller
2016-11-23 20:14 ` Robert LeBlanc
2016-11-23 20:27 ` Philip Müller
2016-11-23 20:53 ` Adam Borowski
2016-11-23 21:01 ` Robert LeBlanc
2016-11-23 21:02 ` [PATCH] x86/kbuild: enable modversions for symbols exported from asm Adam Borowski
2016-11-23 23:10 ` Philip Müller
2016-11-24 4:40 ` Ingo Molnar
2016-11-24 5:20 ` Nicholas Piggin
2016-11-24 6:00 ` Ingo Molnar
2016-11-24 7:20 ` Nicholas Piggin
2016-11-24 7:36 ` Greg Kroah-Hartman
2016-11-24 7:53 ` Nicholas Piggin
2016-11-24 9:32 ` Michal Marek
2016-11-24 10:03 ` Nicholas Piggin
2016-11-24 10:51 ` Michal Marek
2016-11-24 9:38 ` Arnd Bergmann
2016-11-24 10:01 ` Nicholas Piggin
2016-11-24 9:56 ` Greg Kroah-Hartman
2016-11-24 10:31 ` Nicholas Piggin
2016-11-24 15:24 ` Greg Kroah-Hartman
2016-11-25 0:40 ` Nicholas Piggin
2016-11-25 18:00 ` Linus Torvalds
2016-11-26 0:37 ` Nicholas Piggin
2016-11-29 1:15 ` Ben Hutchings
2016-11-29 2:31 ` Nicholas Piggin
2016-11-29 9:14 ` Michal Marek
2016-11-29 4:08 ` Linus Torvalds
2016-11-29 13:19 ` Adam Borowski
2016-11-29 13:29 ` Ingo Molnar
2016-11-29 14:24 ` Adam Borowski
2016-11-29 13:51 ` Adam Borowski
[not found] ` <CA+55aFyZiB4YkwvqzrXO=HD8bcnc2xHkAYrek2QHVnhVvAi3Fw@mail.gmail.com>
2016-11-29 16:03 ` Michal Marek
2016-11-29 16:17 ` Linus Torvalds
2016-11-29 19:57 ` Ben Hutchings [this message]
2016-11-29 20:35 ` Linus Torvalds
2016-11-30 18:18 ` Nicholas Piggin
2016-11-30 18:40 ` Linus Torvalds
2016-11-30 21:33 ` Ben Hutchings
2016-12-01 1:55 ` Nicholas Piggin
2016-12-01 2:35 ` Ben Hutchings
2016-12-01 3:39 ` Nicholas Piggin
2016-12-01 16:12 ` Michal Marek
2016-12-02 14:36 ` Hannes Frederic Sowa
2016-12-09 3:33 ` Nicholas Piggin
2016-12-09 15:21 ` Ian Campbell
2016-12-09 16:15 ` Nicholas Piggin
2016-12-09 22:46 ` Dodji Seketeli
2016-12-10 12:41 ` Greg Kroah-Hartman
2016-12-12 3:50 ` Nicholas Piggin
2016-12-12 9:08 ` Ian Campbell
2016-12-14 17:59 ` Don Zickus
2016-12-13 1:07 ` Stanislav Kozina
2016-12-13 22:51 ` Michal Marek
2016-12-14 8:58 ` Dodji Seketeli
2016-12-14 9:15 ` Michal Marek
2016-12-14 9:36 ` Dodji Seketeli
2016-12-14 9:44 ` Michal Marek
2016-12-14 10:02 ` Dodji Seketeli
2016-12-14 10:15 ` Michal Marek
2016-12-14 9:56 ` Dodji Seketeli
2016-12-14 9:37 ` Michal Marek
2016-12-01 4:13 ` Don Zickus
2016-12-01 4:32 ` Nicholas Piggin
2016-12-01 15:20 ` Don Zickus
2016-12-01 15:26 ` Christoph Hellwig
2016-12-01 15:40 ` Don Zickus
2016-12-01 16:06 ` Greg Kroah-Hartman
2016-12-01 18:42 ` Don Zickus
2016-12-09 3:50 ` Nicholas Piggin
2016-12-09 7:55 ` Stanislav Kozina
2016-12-09 8:14 ` Nicholas Piggin
2016-12-09 14:36 ` Stanislav Kozina
2016-12-09 15:56 ` Nicholas Piggin
2016-12-09 16:03 ` Greg Kroah-Hartman
2016-12-12 9:48 ` Stanislav Kozina
2016-12-13 7:25 ` Nicholas Piggin
2016-12-14 14:04 ` Hannes Frederic Sowa
2016-12-15 2:06 ` Nicholas Piggin
2016-12-15 11:19 ` Hannes Frederic Sowa
2016-12-15 12:03 ` Nicholas Piggin
2016-12-15 13:15 ` Hannes Frederic Sowa
2016-12-15 14:15 ` Nicholas Piggin
2016-12-15 15:17 ` Hannes Frederic Sowa
2016-12-15 13:35 ` Stanislav Kozina
2016-12-09 16:16 ` Don Zickus
2016-12-01 10:48 ` Stanislav Kozina
2016-12-01 11:09 ` Nicholas Piggin
2016-12-01 11:33 ` Stanislav Kozina
2016-12-01 12:39 ` Nicholas Piggin
2016-12-01 15:19 ` Dodji Seketeli
2016-12-01 16:14 ` Michal Marek
2016-11-29 17:05 ` Adam Borowski
2016-11-29 17:10 ` Linus Torvalds
2016-11-29 17:14 ` Linus Torvalds
2016-12-01 13:58 ` Arnd Bergmann
2016-12-01 16:21 ` Michal Marek
2016-12-01 18:26 ` Linus Torvalds
2016-12-02 10:55 ` Arnd Bergmann
2016-12-02 12:40 ` [RFC, PATCH, v3.9] default exported asm symbols to zero Arnd Bergmann
2016-12-02 12:59 ` Geert Uytterhoeven
2016-12-02 14:51 ` Arnd Bergmann
2016-12-02 15:35 ` Adam Borowski
2016-12-03 4:36 ` Ben Hutchings
2016-12-03 10:43 ` Arnd Bergmann
2016-12-02 17:04 ` [PATCH] x86/kbuild: enable modversions for symbols exported from asm Linus Torvalds
2016-12-04 7:44 ` Alan Modra
2016-12-04 20:44 ` Linus Torvalds
2016-11-29 21:23 ` Michal Marek
2016-11-24 9:25 ` Michal Marek
2016-11-24 11:42 ` Regression: " Kalle Valo
2016-11-23 23:07 ` BUG: 4.9-rc6 Still "no symbol version" on boot Philip Müller
2016-11-28 17:10 ` Robert LeBlanc
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161129195721.GI2697@decadent.org.uk \
--to=ben@decadent.org.uk \
--cc=arnd@arndb.de \
--cc=debian-kernel@lists.debian.org \
--cc=gregkh@linuxfoundation.org \
--cc=kilobyte@angband.pl \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=mmarek@suse.com \
--cc=npiggin@gmail.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).