Re: taint/module: Clean up global and module taint flags handling

[Petr Mladek]

On Tue 2016-09-13 16:36:09, Jessica Yu wrote:
> +++ 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@xxxxxxxxxx
> >
> >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@xxxxxxxx>
> >---
> >
> >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

Great catch!
 
> >diff --git a/kernel/panic.c b/kernel/panic.c
> >index ca8cea1ef673..36d4fa264b2c 100644
> >--- a/kernel/panic.c
> >+++ b/kernel/panic.c
> >+/*
> >+ * 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 was not sure whose maintainer tree would be used. So, I rather based
this on the Linus' stable tree. If it will go via the Jikos' livepatch
I could rebase it on top of the commit 2992ef29ae ("livepatch/module: make
TAINT_LIVEPATCH module-specific").

> I think the rest looks fine, thanks for working on the cleanups.

Thanks for review.

Best Regards,
Petr