[PATCH 0/4] kernel/panic: add more descriptive logging of kernel taints

[PATCH 0/4] kernel/panic: add more descriptive logging of kernel taints

[Jani Nikula]

Resend of
https://lore.kernel.org/r/cover.1711380724.git.jani.nikula@intel.com

A few years back there was a discussion about it being difficult to
remember all the taint flags [1].

Time flies. I stumbled on my old branch, brushed it up, and here it is.

I'm not entirely happy with the static buf (which was there to begin
with) or how to decide on its size. Thoughts?

BR,
Jani.

[1] https://lore.kernel.org/r/YmvU+/RUhOcL+B1p@kroah.com

Jani Nikula (4):
  kernel/panic: return early from print_tainted() when not tainted
  kernel/panic: convert print_tainted() to use struct seq_buf internally
  kernel/panic: initialize taint_flags[] using a macro
  kernel/panic: add verbose logging of kernel taints in backtraces

 include/linux/panic.h |   8 +--
 kernel/panic.c        | 116 ++++++++++++++++++++++++++++--------------
 lib/dump_stack.c      |   3 ++
 3 files changed, 87 insertions(+), 40 deletions(-)

-- 
2.39.2

[PATCH 1/4] kernel/panic: return early from print_tainted() when not tainted

[Jani Nikula]

Reduce indent to make follow-up changes slightly easier on the eyes.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 kernel/panic.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 6df2ba08d5ae..5c9396418e1c 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -508,22 +508,23 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 const char *print_tainted(void)
 {
 	static char buf[TAINT_FLAGS_COUNT + sizeof("Tainted: ")];
+	char *s;
+	int i;
 
 	BUILD_BUG_ON(ARRAY_SIZE(taint_flags) != TAINT_FLAGS_COUNT);
 
-	if (tainted_mask) {
-		char *s;
-		int i;
-
-		s = buf + sprintf(buf, "Tainted: ");
-		for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
-			const struct taint_flag *t = &taint_flags[i];
-			*s++ = test_bit(i, &tainted_mask) ?
-					t->c_true : t->c_false;
-		}
-		*s = 0;
-	} else
+	if (!tainted_mask) {
 		snprintf(buf, sizeof(buf), "Not tainted");
+		return buf;
+	}
+
+	s = buf + sprintf(buf, "Tainted: ");
+	for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
+		const struct taint_flag *t = &taint_flags[i];
+		*s++ = test_bit(i, &tainted_mask) ?
+			t->c_true : t->c_false;
+	}
+	*s = 0;
 
 	return buf;
 }
-- 
2.39.2

Re: [PATCH 1/4] kernel/panic: return early from print_tainted() when not tainted

[Greg KH]

On Fri, May 31, 2024 at 12:04:54PM +0300, Jani Nikula wrote:
> Reduce indent to make follow-up changes slightly easier on the eyes.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  kernel/panic.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 2/4] kernel/panic: convert print_tainted() to use struct seq_buf internally

[Jani Nikula]

Convert print_tainted() to use struct seq_buf internally in order to be
more aware of the buffer constraints as well as make it easier to extend
in follow-up work.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 kernel/panic.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 5c9396418e1c..b7b2fb8b3625 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -35,6 +35,7 @@
 #include <linux/debugfs.h>
 #include <linux/sysfs.h>
 #include <linux/context_tracking.h>
+#include <linux/seq_buf.h>
 #include <trace/events/error_report.h>
 #include <asm/sections.h>
 
@@ -497,6 +498,25 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 	[ TAINT_TEST ]			= { 'N', ' ', true },
 };
 
+static void print_tainted_seq(struct seq_buf *s)
+{
+	int i;
+
+	if (!tainted_mask) {
+		seq_buf_puts(s, "Not tainted");
+		return;
+	}
+
+	seq_buf_printf(s, "Tainted: ");
+	for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
+		const struct taint_flag *t = &taint_flags[i];
+		bool is_set = test_bit(i, &tainted_mask);
+		char c = is_set ? t->c_true : t->c_false;
+
+		seq_buf_putc(s, c);
+	}
+}
+
 /**
  * print_tainted - return a string to represent the kernel taint state.
  *
@@ -508,25 +528,15 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 const char *print_tainted(void)
 {
 	static char buf[TAINT_FLAGS_COUNT + sizeof("Tainted: ")];
-	char *s;
-	int i;
+	struct seq_buf s;
 
 	BUILD_BUG_ON(ARRAY_SIZE(taint_flags) != TAINT_FLAGS_COUNT);
 
-	if (!tainted_mask) {
-		snprintf(buf, sizeof(buf), "Not tainted");
-		return buf;
-	}
+	seq_buf_init(&s, buf, sizeof(buf));
 
-	s = buf + sprintf(buf, "Tainted: ");
-	for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
-		const struct taint_flag *t = &taint_flags[i];
-		*s++ = test_bit(i, &tainted_mask) ?
-			t->c_true : t->c_false;
-	}
-	*s = 0;
+	print_tainted_seq(&s);
 
-	return buf;
+	return seq_buf_str(&s);
 }
 
 int test_taint(unsigned flag)
-- 
2.39.2

Re: [PATCH 2/4] kernel/panic: convert print_tainted() to use struct seq_buf internally

[Greg KH]

On Fri, May 31, 2024 at 12:04:55PM +0300, Jani Nikula wrote:
> Convert print_tainted() to use struct seq_buf internally in order to be
> more aware of the buffer constraints as well as make it easier to extend
> in follow-up work.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  kernel/panic.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 3/4] kernel/panic: initialize taint_flags[] using a macro

[Jani Nikula]

Make it easier to extend struct taint_flags in follow-up.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 kernel/panic.c | 46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index b7b2fb8b3625..8f7bb94ad7d8 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -472,32 +472,40 @@ void panic(const char *fmt, ...)
 
 EXPORT_SYMBOL(panic);
 
+#define TAINT_FLAG(taint, _c_true, _c_false, _module)			\
+	[ TAINT_##taint ] = {						\
+		.c_true = _c_true, .c_false = _c_false,			\
+		.module = _module,					\
+	}
+
 /*
  * 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] = {
-	[ TAINT_PROPRIETARY_MODULE ]	= { 'P', 'G', true },
-	[ TAINT_FORCED_MODULE ]		= { 'F', ' ', true },
-	[ TAINT_CPU_OUT_OF_SPEC ]	= { 'S', ' ', false },
-	[ TAINT_FORCED_RMMOD ]		= { 'R', ' ', false },
-	[ TAINT_MACHINE_CHECK ]		= { 'M', ' ', false },
-	[ TAINT_BAD_PAGE ]		= { 'B', ' ', false },
-	[ TAINT_USER ]			= { 'U', ' ', false },
-	[ TAINT_DIE ]			= { 'D', ' ', false },
-	[ TAINT_OVERRIDDEN_ACPI_TABLE ]	= { 'A', ' ', false },
-	[ TAINT_WARN ]			= { 'W', ' ', false },
-	[ TAINT_CRAP ]			= { 'C', ' ', true },
-	[ TAINT_FIRMWARE_WORKAROUND ]	= { 'I', ' ', false },
-	[ TAINT_OOT_MODULE ]		= { 'O', ' ', true },
-	[ TAINT_UNSIGNED_MODULE ]	= { 'E', ' ', true },
-	[ TAINT_SOFTLOCKUP ]		= { 'L', ' ', false },
-	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
-	[ TAINT_AUX ]			= { 'X', ' ', true },
-	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
-	[ TAINT_TEST ]			= { 'N', ' ', true },
+	TAINT_FLAG(PROPRIETARY_MODULE,		'P', 'G', true),
+	TAINT_FLAG(FORCED_MODULE,		'F', ' ', true),
+	TAINT_FLAG(CPU_OUT_OF_SPEC,		'S', ' ', false),
+	TAINT_FLAG(FORCED_RMMOD,		'R', ' ', false),
+	TAINT_FLAG(MACHINE_CHECK,		'M', ' ', false),
+	TAINT_FLAG(BAD_PAGE,			'B', ' ', false),
+	TAINT_FLAG(USER,			'U', ' ', false),
+	TAINT_FLAG(DIE,				'D', ' ', false),
+	TAINT_FLAG(OVERRIDDEN_ACPI_TABLE,	'A', ' ', false),
+	TAINT_FLAG(WARN,			'W', ' ', false),
+	TAINT_FLAG(CRAP,			'C', ' ', true),
+	TAINT_FLAG(FIRMWARE_WORKAROUND,		'I', ' ', false),
+	TAINT_FLAG(OOT_MODULE,			'O', ' ', true),
+	TAINT_FLAG(UNSIGNED_MODULE,		'E', ' ', true),
+	TAINT_FLAG(SOFTLOCKUP,			'L', ' ', false),
+	TAINT_FLAG(LIVEPATCH,			'K', ' ', true),
+	TAINT_FLAG(AUX,				'X', ' ', true),
+	TAINT_FLAG(RANDSTRUCT,			'T', ' ', true),
+	TAINT_FLAG(TEST,			'N', ' ', true),
 };
 
+#undef TAINT_FLAG
+
 static void print_tainted_seq(struct seq_buf *s)
 {
 	int i;
-- 
2.39.2

Re: [PATCH 3/4] kernel/panic: initialize taint_flags[] using a macro

[Greg KH]

On Fri, May 31, 2024 at 12:04:56PM +0300, Jani Nikula wrote:
> Make it easier to extend struct taint_flags in follow-up.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>


Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 4/4] kernel/panic: add verbose logging of kernel taints in backtraces

[Jani Nikula]

With nearly 20 taint flags and respective characters, it's getting a bit
difficult to remember what each taint flag character means. Add verbose
logging of the set taints in the format:

Tainted: [P]=PROPRIETARY_MODULE, [W]=WARN

in dump_stack_print_info() when there are taints.

Note that the "negative flag" G is not included.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 include/linux/panic.h |  8 +++++---
 kernel/panic.c        | 45 ++++++++++++++++++++++++++++++++-----------
 lib/dump_stack.c      |  3 +++
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/include/linux/panic.h b/include/linux/panic.h
index 6717b15e798c..3130e0b5116b 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -77,9 +77,10 @@ static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
 #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
 
 struct taint_flag {
-	char c_true;	/* character printed when tainted */
-	char c_false;	/* character printed when not tainted */
-	bool module;	/* also show as a per-module taint flag */
+	char c_true;		/* character printed when tainted */
+	char c_false;		/* character printed when not tainted */
+	bool module;		/* also show as a per-module taint flag */
+	const char *desc;	/* verbose description of the set taint flag */
 };
 
 extern const struct taint_flag taint_flags[TAINT_FLAGS_COUNT];
@@ -90,6 +91,7 @@ enum lockdep_ok {
 };
 
 extern const char *print_tainted(void);
+extern const char *print_tainted_verbose(void);
 extern void add_taint(unsigned flag, enum lockdep_ok);
 extern int test_taint(unsigned flag);
 extern unsigned long get_taint(void);
diff --git a/kernel/panic.c b/kernel/panic.c
index 8f7bb94ad7d8..a914f2fcf9ad 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -476,6 +476,7 @@ EXPORT_SYMBOL(panic);
 	[ TAINT_##taint ] = {						\
 		.c_true = _c_true, .c_false = _c_false,			\
 		.module = _module,					\
+		.desc = #taint,						\
 	}
 
 /*
@@ -506,8 +507,9 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 
 #undef TAINT_FLAG
 
-static void print_tainted_seq(struct seq_buf *s)
+static void print_tainted_seq(struct seq_buf *s, bool verbose)
 {
+	const char *sep = "";
 	int i;
 
 	if (!tainted_mask) {
@@ -521,10 +523,32 @@ static void print_tainted_seq(struct seq_buf *s)
 		bool is_set = test_bit(i, &tainted_mask);
 		char c = is_set ? t->c_true : t->c_false;
 
-		seq_buf_putc(s, c);
+		if (verbose) {
+			if (is_set) {
+				seq_buf_printf(s, "%s[%c]=%s", sep, c, t->desc);
+				sep = ", ";
+			}
+		} else {
+			seq_buf_putc(s, c);
+		}
 	}
 }
 
+static const char *_print_tainted(bool verbose)
+{
+	/* FIXME: what should the size be? */
+	static char buf[sizeof(taint_flags)];
+	struct seq_buf s;
+
+	BUILD_BUG_ON(ARRAY_SIZE(taint_flags) != TAINT_FLAGS_COUNT);
+
+	seq_buf_init(&s, buf, sizeof(buf));
+
+	print_tainted_seq(&s, verbose);
+
+	return seq_buf_str(&s);
+}
+
 /**
  * print_tainted - return a string to represent the kernel taint state.
  *
@@ -535,16 +559,15 @@ static void print_tainted_seq(struct seq_buf *s)
  */
 const char *print_tainted(void)
 {
-	static char buf[TAINT_FLAGS_COUNT + sizeof("Tainted: ")];
-	struct seq_buf s;
-
-	BUILD_BUG_ON(ARRAY_SIZE(taint_flags) != TAINT_FLAGS_COUNT);
-
-	seq_buf_init(&s, buf, sizeof(buf));
-
-	print_tainted_seq(&s);
+	return _print_tainted(false);
+}
 
-	return seq_buf_str(&s);
+/**
+ * print_tainted_verbose - A more verbose version of print_tainted()
+ */
+const char *print_tainted_verbose(void)
+{
+	return _print_tainted(true);
 }
 
 int test_taint(unsigned flag)
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 222c6d6c8281..8b6b70eaf949 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -62,6 +62,9 @@ void dump_stack_print_info(const char *log_lvl)
 	       (int)strcspn(init_utsname()->version, " "),
 	       init_utsname()->version, BUILD_ID_VAL);
 
+	if (get_taint())
+		printk("%s%s\n", log_lvl, print_tainted_verbose());
+
 	if (dump_stack_arch_desc_str[0] != '\0')
 		printk("%sHardware name: %s\n",
 		       log_lvl, dump_stack_arch_desc_str);
-- 
2.39.2

Re: [PATCH 4/4] kernel/panic: add verbose logging of kernel taints in backtraces

[Greg KH]

On Fri, May 31, 2024 at 12:04:57PM +0300, Jani Nikula wrote:
> With nearly 20 taint flags and respective characters, it's getting a bit
> difficult to remember what each taint flag character means. Add verbose
> logging of the set taints in the format:
> 
> Tainted: [P]=PROPRIETARY_MODULE, [W]=WARN
> 
> in dump_stack_print_info() when there are taints.
> 
> Note that the "negative flag" G is not included.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>