Messages in this thread Patch in this message | | | Date | Tue, 13 Sep 2016 16:36:09 -0400 | From | Jessica Yu <> | Subject | Re: taint/module: Clean up global and module taint flags handling |
| |
+++ Petr Mladek [12/09/16 16:13 +0200]: >The commit 66cc69e34e86a231 ("Fix: module signature vs tracepoints: >add new TAINT_UNSIGNED_MODULE") updated module_taint_flags() to >potentially print one more character. But it did not increase the >size of the corresponding buffers in m_show() and print_modules(). > >We have recently done the same mistake when adding a taint flag >for livepatching, see >https://lkml.kernel.org/g/cfba2c823bb984690b73572aaae1db596b54a082.1472137475.git.jpoimboe@redhat.com > >Also struct module uses an incompatible type for mod-taints flags. >It survived from the commit 2bc2d61a9638dab670d ("[PATCH] list module >taint flags in Oops/panic"). There was used "int" for the global taint >flags at these times. But only the global tain flags was later changed >to "unsigned long" by the commit 25ddbb18aae33ad2 ("Make the taint >flags reliable"). > >This patch defines TAINT_FLAGS_COUNT that can be used to create >arrays and buffers of the right size. Note that we could not use >enum because the taint flag indexes are used also in assembly code. > >Then it reworks the table that describes the taint flags. The TAINT_* >numbers can be used as the index. Instead, we add information >if the taint flag is also shown per-module. > >Finally, it uses "unsigned long", bit operations, and the updated >taint_flags table also for mod->taints. > >It is not optimal because only few taint flags can be printed by >module_taint_flags(). But better be on the safe side. IMHO, it is >not worth the optimization and this is a good compromise. > >Signed-off-by: Petr Mladek <pmladek@suse.com> >--- > >Changes against v1: > > + reverted the change to enums because it broke asm code > > + instead, forced the size of the taint_flags table; > used taint numbers as the index; used the table also > in module.c > > + fixed the type of mod->taints > > > include/linux/kernel.h | 9 +++++++++ > include/linux/module.h | 2 +- > kernel/module.c | 31 +++++++++++++---------------- > kernel/panic.c | 53 ++++++++++++++++++++++++-------------------------- > 4 files changed, 48 insertions(+), 47 deletions(-) > >diff --git a/include/linux/kernel.h b/include/linux/kernel.h >index d96a6118d26a..33e88ff3af40 100644 >--- a/include/linux/kernel.h >+++ b/include/linux/kernel.h >@@ -509,6 +509,15 @@ extern enum system_states { > #define TAINT_UNSIGNED_MODULE 13 > #define TAINT_SOFTLOCKUP 14 > #define TAINT_LIVEPATCH 15 >+#define TAINT_FLAGS_COUNT 16 >+ >+struct taint_flag { >+ char true; /* character printed when tained */ >+ char false; /* character printed when not tained */
s/tained/tainted
>+ bool module; /* also show as a per-module taint flag */ >+}; >+ >+extern const struct taint_flag taint_flags[TAINT_FLAGS_COUNT]; > > extern const char hex_asc[]; > #define hex_asc_lo(x) hex_asc[((x) & 0x0f)] >diff --git a/include/linux/module.h b/include/linux/module.h >index 0c3207d26ac0..f6ee569c62bb 100644 >--- a/include/linux/module.h >+++ b/include/linux/module.h >@@ -399,7 +399,7 @@ struct module { > /* Arch-specific module values */ > struct mod_arch_specific arch; > >- unsigned int taints; /* same bits as kernel:tainted */ >+ unsigned long taints; /* same bits as kernel:taint_flags */ > > #ifdef CONFIG_GENERIC_BUG > /* Support for BUG */ >diff --git a/kernel/module.c b/kernel/module.c >index 529efae9f481..303e03e08b51 100644 >--- a/kernel/module.c >+++ b/kernel/module.c >@@ -330,7 +330,7 @@ static inline void add_taint_module(struct module *mod, unsigned flag, > enum lockdep_ok lockdep_ok) > { > add_taint(flag, lockdep_ok); >- mod->taints |= (1U << flag); >+ set_bit(flag, &mod->taints); > } > > /* >@@ -1138,22 +1138,13 @@ static inline int module_unload_init(struct module *mod) > static size_t module_flags_taint(struct module *mod, char *buf) > { > size_t l = 0; >+ int i; >+ >+ for (i = 0; i < TAINT_FLAGS_COUNT; i++) { >+ if (taint_flags[i].module && test_bit(i, &mod->taints)) >+ buf[l++] = taint_flags[i].true; >+ } > >- if (mod->taints & (1 << TAINT_PROPRIETARY_MODULE)) >- buf[l++] = 'P'; >- if (mod->taints & (1 << TAINT_OOT_MODULE)) >- buf[l++] = 'O'; >- if (mod->taints & (1 << TAINT_FORCED_MODULE)) >- buf[l++] = 'F'; >- if (mod->taints & (1 << TAINT_CRAP)) >- buf[l++] = 'C'; >- if (mod->taints & (1 << TAINT_UNSIGNED_MODULE)) >- buf[l++] = 'E'; >- /* >- * TAINT_FORCED_RMMOD: could be added. >- * TAINT_CPU_OUT_OF_SPEC, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't >- * apply to modules. >- */ > return l; > } > >@@ -4036,6 +4027,10 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *, > } > #endif /* CONFIG_KALLSYMS */ > >+/* Maximum number of characters written by module_flags() */ >+#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4) >+ >+/* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */ > static char *module_flags(struct module *mod, char *buf) > { > int bx = 0; >@@ -4080,7 +4075,7 @@ static void m_stop(struct seq_file *m, void *p) > static int m_show(struct seq_file *m, void *p) > { > struct module *mod = list_entry(p, struct module, list); >- char buf[8]; >+ char buf[MODULE_FLAGS_BUF_SIZE]; > > /* We always ignore unformed modules. */ > if (mod->state == MODULE_STATE_UNFORMED) >@@ -4251,7 +4246,7 @@ EXPORT_SYMBOL_GPL(__module_text_address); > void print_modules(void) > { > struct module *mod; >- char buf[8]; >+ char buf[MODULE_FLAGS_BUF_SIZE]; > > printk(KERN_DEFAULT "Modules linked in:"); > /* Most callers should already have preempt disabled, but make sure */ >diff --git a/kernel/panic.c b/kernel/panic.c >index ca8cea1ef673..36d4fa264b2c 100644 >--- a/kernel/panic.c >+++ b/kernel/panic.c >@@ -265,30 +265,27 @@ void panic(const char *fmt, ...) > > EXPORT_SYMBOL(panic); > >- >-struct tnt { >- u8 bit; >- char true; >- char false; >-}; >- >-static const struct tnt tnts[] = { >- { TAINT_PROPRIETARY_MODULE, 'P', 'G' }, >- { TAINT_FORCED_MODULE, 'F', ' ' }, >- { TAINT_CPU_OUT_OF_SPEC, 'S', ' ' }, >- { TAINT_FORCED_RMMOD, 'R', ' ' }, >- { TAINT_MACHINE_CHECK, 'M', ' ' }, >- { TAINT_BAD_PAGE, 'B', ' ' }, >- { TAINT_USER, 'U', ' ' }, >- { TAINT_DIE, 'D', ' ' }, >- { TAINT_OVERRIDDEN_ACPI_TABLE, 'A', ' ' }, >- { TAINT_WARN, 'W', ' ' }, >- { TAINT_CRAP, 'C', ' ' }, >- { TAINT_FIRMWARE_WORKAROUND, 'I', ' ' }, >- { TAINT_OOT_MODULE, 'O', ' ' }, >- { TAINT_UNSIGNED_MODULE, 'E', ' ' }, >- { TAINT_SOFTLOCKUP, 'L', ' ' }, >- { TAINT_LIVEPATCH, 'K', ' ' }, >+/* >+ * TAINT_FORCED_RMMOD could be a per-module flag but the module >+ * is being removed anyway. >+ */ >+const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = { >+ { 'P', 'G', true }, /* TAINT_PROPRIETARY_MODULE */ >+ { 'F', ' ', true }, /* TAINT_FORCED_MODULE */ >+ { 'S', ' ', false }, /* TAINT_CPU_OUT_OF_SPEC */ >+ { 'R', ' ', false }, /* TAINT_FORCED_RMMOD */ >+ { 'M', ' ', false }, /* TAINT_MACHINE_CHECK */ >+ { 'B', ' ', false }, /* TAINT_BAD_PAGE */ >+ { 'U', ' ', false }, /* TAINT_USER */ >+ { 'D', ' ', false }, /* TAINT_DIE */ >+ { 'A', ' ', false }, /* TAINT_OVERRIDDEN_ACPI_TABLE */ >+ { 'W', ' ', false }, /* TAINT_WARN */ >+ { 'C', ' ', true }, /* TAINT_CRAP */ >+ { 'I', ' ', false }, /* TAINT_FIRMWARE_WORKAROUND */ >+ { 'O', ' ', true }, /* TAINT_OOT_MODULE */ >+ { 'E', ' ', true }, /* TAINT_UNSIGNED_MODULE */ >+ { 'L', ' ', false }, /* TAINT_SOFTLOCKUP */ >+ { 'K', ' ', false }, /* TAINT_LIVEPATCH */
This should be true here, right? TAINT_LIVEPATCH has been made a module-specific taint by commit 2992ef29ae ("livepatch/module: make TAINT_LIVEPATCH module-specific").
I think the rest looks fine, thanks for working on the cleanups.
Jessica
> }; > > /** >@@ -315,16 +312,16 @@ static const struct tnt tnts[] = { > */ > const char *print_tainted(void) > { >- static char buf[ARRAY_SIZE(tnts) + sizeof("Tainted: ")]; >+ static char buf[TAINT_FLAGS_COUNT + sizeof("Tainted: ")]; > > if (tainted_mask) { > char *s; > int i; > > s = buf + sprintf(buf, "Tainted: "); >- for (i = 0; i < ARRAY_SIZE(tnts); i++) { >- const struct tnt *t = &tnts[i]; >- *s++ = test_bit(t->bit, &tainted_mask) ? >+ for (i = 0; i < TAINT_FLAGS_COUNT; i++) { >+ const struct taint_flag *t = &taint_flags[i]; >+ *s++ = test_bit(i, &tainted_mask) ? > t->true : t->false; > } > *s = 0; >-- >1.8.5.6 >
| |