[PATCH 0/3] lib/bug: concurrency fix, cleanups, and debugfs inspection

[PATCH 0/3] lib/bug: concurrency fix, cleanups, and debugfs inspection

[Josh Law]

Small series for lib/bug.c:

  - Fix a KCSAN data race on bug->flags by using READ_ONCE/WRITE_ONCE
    and correcting the misleading comment about concurrency (patch 1).
  - Clean up const-correctness and bare 'unsigned' types (patch 2).
  - Add /sys/kernel/debug/bug_sites to inspect registered BUG/WARN
    sites and their runtime state, complementing the existing
    clear_warn_once interface (patch 3).

Build-tested with W=123.

Josh Law (3):
  lib/bug: annotate concurrent access to bug->flags with
    READ_ONCE/WRITE_ONCE
  lib/bug: clean up types in bug reporting helpers
  lib/bug: add debugfs interface to list all BUG/WARN sites

 include/linux/bug.h |  6 +--
 lib/bug.c           | 96 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 87 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH 1/3] lib/bug: annotate concurrent access to bug->flags with READ_ONCE/WRITE_ONCE

[Josh Law]

Multiple CPUs can hit a WARN_ON_ONCE simultaneously, causing concurrent
reads and writes to bug->flags without synchronization.  In __report_bug(),
the flags are read to check BUGFLAG_DONE and then BUGFLAG_DONE is set via
a plain read-modify-write.  The race is benign since the store is
idempotent, but KCSAN will flag this as a data race.

Read the flags once with READ_ONCE and reuse the cached value for both
the flag checks and the WRITE_ONCE store.  Also annotate the concurrent
clear in clear_once_table().  Update the misleading comment that claimed
concurrency is not an issue.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bug.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/lib/bug.c b/lib/bug.c
index bbc301097749..037c7370dadf 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -201,6 +201,7 @@ static enum bug_trap_type __report_bug(struct bug_entry *bug, unsigned long buga
 {
 	bool warning, once, done, no_cut, has_args;
 	const char *file, *fmt;
+	unsigned short flags;
 	unsigned line;
 
 	if (!bug) {
@@ -217,20 +218,24 @@ static enum bug_trap_type __report_bug(struct bug_entry *bug, unsigned long buga
 	bug_get_file_line(bug, &file, &line);
 	fmt = bug_get_format(bug);
 
-	warning  = bug->flags & BUGFLAG_WARNING;
-	once     = bug->flags & BUGFLAG_ONCE;
-	done     = bug->flags & BUGFLAG_DONE;
-	no_cut   = bug->flags & BUGFLAG_NO_CUT_HERE;
-	has_args = bug->flags & BUGFLAG_ARGS;
+	flags    = READ_ONCE(bug->flags);
+	warning  = flags & BUGFLAG_WARNING;
+	once     = flags & BUGFLAG_ONCE;
+	done     = flags & BUGFLAG_DONE;
+	no_cut   = flags & BUGFLAG_NO_CUT_HERE;
+	has_args = flags & BUGFLAG_ARGS;
 
 	if (warning && once) {
 		if (done)
 			return BUG_TRAP_TYPE_WARN;
 
 		/*
-		 * Since this is the only store, concurrency is not an issue.
+		 * Multiple CPUs can hit a WARN_ON_ONCE at the same time
+		 * and both read done == false.  The race is benign: setting
+		 * BUGFLAG_DONE is idempotent, and the worst case is that
+		 * the warning prints a few extra times.
 		 */
-		bug->flags |= BUGFLAG_DONE;
+		WRITE_ONCE(bug->flags, flags | BUGFLAG_DONE);
 	}
 
 	/*
@@ -289,7 +294,7 @@ static void clear_once_table(struct bug_entry *start, struct bug_entry *end)
 	struct bug_entry *bug;
 
 	for (bug = start; bug < end; bug++)
-		bug->flags &= ~BUGFLAG_DONE;
+		WRITE_ONCE(bug->flags, READ_ONCE(bug->flags) & ~BUGFLAG_DONE);
 }
 
 void generic_bug_clear_once(void)
-- 
2.34.1

[PATCH 2/3] lib/bug: clean up types in bug reporting helpers

[Josh Law]

Add const to the bug_entry parameter of bug_get_file_line() and
bug_get_format() since they only read from the structure.  Update
the declaration in include/linux/bug.h to match.

While here, replace bare 'unsigned' with 'unsigned int' for the loop
counter in module_find_bug() and the line number in __report_bug().

Signed-off-by: Josh Law <objecting@objecting.org>
---
 include/linux/bug.h | 6 +++---
 lib/bug.c           | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 17a4933c611b..01a996eed8d8 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -36,7 +36,7 @@ static inline int is_warning_bug(const struct bug_entry *bug)
 	return bug->flags & BUGFLAG_WARNING;
 }
 
-void bug_get_file_line(struct bug_entry *bug, const char **file,
+void bug_get_file_line(const struct bug_entry *bug, const char **file,
 		       unsigned int *line);
 
 struct bug_entry *find_bug(unsigned long bugaddr);
@@ -70,8 +70,8 @@ report_bug_entry(struct bug_entry *bug, struct pt_regs *regs)
 	return BUG_TRAP_TYPE_BUG;
 }
 
-static inline void bug_get_file_line(struct bug_entry *bug, const char **file,
-				     unsigned int *line)
+static inline void bug_get_file_line(const struct bug_entry *bug,
+				     const char **file, unsigned int *line)
 {
 	*file = NULL;
 	*line = 0;
diff --git a/lib/bug.c b/lib/bug.c
index 037c7370dadf..9d76703ff7d1 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -71,7 +71,7 @@ static struct bug_entry *module_find_bug(unsigned long bugaddr)
 
 	guard(rcu)();
 	list_for_each_entry_rcu(mod, &module_bug_list, bug_list) {
-		unsigned i;
+		unsigned int i;
 
 		bug = mod->bug_table;
 		for (i = 0; i < mod->num_bugs; ++i, ++bug)
@@ -123,7 +123,7 @@ static inline struct bug_entry *module_find_bug(unsigned long bugaddr)
 }
 #endif
 
-void bug_get_file_line(struct bug_entry *bug, const char **file,
+void bug_get_file_line(const struct bug_entry *bug, const char **file,
 		       unsigned int *line)
 {
 #ifdef CONFIG_DEBUG_BUGVERBOSE
@@ -139,7 +139,7 @@ void bug_get_file_line(struct bug_entry *bug, const char **file,
 #endif
 }
 
-static const char *bug_get_format(struct bug_entry *bug)
+static const char *bug_get_format(const struct bug_entry *bug)
 {
 	const char *format = NULL;
 #ifdef HAVE_ARCH_BUG_FORMAT
@@ -202,7 +202,7 @@ static enum bug_trap_type __report_bug(struct bug_entry *bug, unsigned long buga
 	bool warning, once, done, no_cut, has_args;
 	const char *file, *fmt;
 	unsigned short flags;
-	unsigned line;
+	unsigned int line;
 
 	if (!bug) {
 		if (!is_valid_bugaddr(bugaddr))
-- 
2.34.1

[PATCH 3/3] lib/bug: add debugfs interface to list all BUG/WARN sites

[Josh Law]

Currently there is no way to inspect the runtime state of WARN_ONCE
sites.  The existing clear_warn_once debugfs file lets operators reset
them, but there is no counterpart to see which sites exist or which
have already fired.  On production systems this matters: when a
WARN_ONCE fires during a transient event, the only evidence is a
single dmesg line that may have already rotated out.  Operators
investigating later have no way to tell which WARN_ONCE sites have
tripped without rebooting or reproducing the issue.

Add /sys/kernel/debug/bug_sites which lists every registered BUG()
and WARN() site in the kernel, including those from loaded modules.
Each line shows the symbolized address, source location (file:line
when CONFIG_DEBUG_BUGVERBOSE is enabled), type and state flags, and
the module name if applicable.  Example output:

  func+0x10/0x20  kernel/foo.c:123  warn,once,done
  bar+0x5/0x10    drivers/baz.c:456 warn         [baz_mod]

The "done" flag indicates that a WARN_ONCE site has fired at least
once since boot (or since the last clear_warn_once), giving operators
a reliable way to audit warning state on a running system without
depending on log retention.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bug.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/lib/bug.c b/lib/bug.c
index 9d76703ff7d1..ddf4531d6661 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -48,6 +48,8 @@
 #include <linux/rculist.h>
 #include <linux/ftrace.h>
 #include <linux/context_tracking.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 extern struct bug_entry __start___bug_table[], __stop___bug_table[];
 
@@ -311,3 +313,68 @@ void generic_bug_clear_once(void)
 
 	clear_once_table(__start___bug_table, __stop___bug_table);
 }
+
+#ifdef CONFIG_DEBUG_FS
+static void bug_show_entry(struct seq_file *m, const struct bug_entry *bug,
+			   const char *modname)
+{
+	const char *file;
+	unsigned int line;
+	unsigned short flags = READ_ONCE(bug->flags);
+
+	bug_get_file_line(bug, &file, &line);
+
+	seq_printf(m, "%pS\t", (void *)bug_addr(bug));
+
+	if (file)
+		seq_printf(m, "%s:%u\t", file, line);
+	else
+		seq_puts(m, "-\t");
+
+	if (flags & BUGFLAG_WARNING)
+		seq_puts(m, "warn");
+	else
+		seq_puts(m, "bug");
+	if (flags & BUGFLAG_ONCE)
+		seq_puts(m, ",once");
+	if (flags & BUGFLAG_DONE)
+		seq_puts(m, ",done");
+
+	if (modname)
+		seq_printf(m, "\t[%s]", modname);
+
+	seq_putc(m, '\n');
+}
+
+static int bug_sites_show(struct seq_file *m, void *v)
+{
+	struct bug_entry *bug;
+
+	for (bug = __start___bug_table; bug < __stop___bug_table; bug++)
+		bug_show_entry(m, bug, NULL);
+
+#ifdef CONFIG_MODULES
+	{
+		struct module *mod;
+		unsigned int i;
+
+		rcu_read_lock();
+		list_for_each_entry_rcu(mod, &module_bug_list, bug_list)
+			for (i = 0; i < mod->num_bugs; i++)
+				bug_show_entry(m, &mod->bug_table[i],
+					       mod->name);
+		rcu_read_unlock();
+	}
+#endif
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(bug_sites);
+
+static int __init bug_debugfs_init(void)
+{
+	debugfs_create_file("bug_sites", 0444, NULL, NULL, &bug_sites_fops);
+	return 0;
+}
+device_initcall(bug_debugfs_init);
+#endif /* CONFIG_DEBUG_FS */
-- 
2.34.1