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
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
