[PATCH v3 0/4] rv/reactors: fix lockdep warning and add KUnit tests

[PATCH v3 0/4] rv/reactors: fix lockdep warning and add KUnit tests

[wen.yang]

From: Wen Yang <wen.yang@linux.dev>

We occasionally hit a lockdep "Invalid wait context" warning in
production when a reactor callback is preempted by a timer interrupt.
On interrupt exit the scheduler takes rq->__lock (LD_WAIT_SPIN) while
rv_react() still holds its wait-type-override map, which declared
LD_WAIT_FREE.  On any kernel where the task context has preemption
enabled (not just CONFIG_PREEMPT_RT) this triggers a spurious lockdep
report:

    [ BUG: Invalid wait context ]
    1 lock held by kunit_try_catch/209:
     #0: (rv_react_map-wait-type-override){+.+.}-{1:1}
    kunit_try_catch/209 is trying to lock:
    ffff8a743ed3e8a0 (&rq->__lock){-...}-{2:2}

Changes in v3:

Patch 1:
- Rewrite commit message to clarify that the bug is not
  PREEMPT_RT-specific: any preemptible task context can be interrupted
  by a timer, and the scheduler acquires rq->__lock (LD_WAIT_SPIN) on
  interrupt exit while rv_react() still holds its LD_WAIT_FREE override
  map.
- Rewrite the inline comment in rv_react() to make clear.

Patch 3:
- Fix commit message: missing EXPORT_SYMBOL_GPL() is caught by modpost
  at link time, not at load time.

Patch 4:
- Drop test_reactor_registered bool and the unregister_test_reactor()
  wrapper entirely.
- Remove test_register_unregister(); the teardown already covers the
  unregister path, and test_double_register() exercises the full
  register->verify->unregister-via-teardown flow.
- Replace the runtime KUNIT_ASSERT_EQ(strlen(...)) guard in
  test_name_too_long() with a _Static_assert on the name array size.
- In test_react_no_callback(): reuse the existing react_call_count
  atomic to assert that the callback was never invoked.
  This catches a broader class of regressions — e.g. 
  an inverted or missing NULL guard that does not cause a NULL dereference.

No functional change to patches 2-3; the series structure is unchanged.

Tested with CONFIG_PROVE_LOCKING=y and CONFIG_KUNIT=y.

v2: https://lore.kernel.org/lkml/cover.1785695669.git.wen.yang@linux.dev/
v1: https://lore.kernel.org/lkml/cover.1781541556.git.wen.yang@linux.dev/

Wen Yang (4):
  rv/reactors: use context-sensitive lockdep wait type in rv_react()
  rv/reactors: propagate rv_register_reactor() error from reactor init
  rv/reactors: export rv_register_reactor() and rv_unregister_reactor()
  rv/reactors: add KUnit tests for reactor registration and dispatch

 kernel/trace/rv/Kconfig             |  12 +++
 kernel/trace/rv/Makefile            |   1 +
 kernel/trace/rv/reactor_panic.c     |   3 +-
 kernel/trace/rv/reactor_printk.c    |   3 +-
 kernel/trace/rv/rv_reactors.c       |  22 +++--
 kernel/trace/rv/rv_reactors_kunit.c | 119 ++++++++++++++++++++++++++++
 6 files changed, 151 insertions(+), 9 deletions(-)
 create mode 100644 kernel/trace/rv/rv_reactors_kunit.c

base-commit: 785095112f4198de49760552374f364043c8dbdf
-- 
2.25.1

[PATCH v3 1/4] rv/reactors: use context-sensitive lockdep wait type in rv_react()

[wen.yang]

From: Wen Yang <wen.yang@linux.dev>

Reactors must not explicitly take locks, so they should comply with
LD_WAIT_FREE.  However, reactor callbacks can run with preemption
enabled on any kernel (not just PREEMPT_RT).  If a timer interrupt
fires during the callback, the interrupt exit path schedules and
acquires rq->__lock (LD_WAIT_SPIN) while the lockdep override map that
declared LD_WAIT_FREE is still held, triggering a spurious
"Invalid wait context" warning:

    [ BUG: Invalid wait context ]
    context-{5:5}
    1 lock held by kunit_try_catch/209:
     #0: (rv_react_map-wait-type-override){+.+.}-{1:1}
    kunit_try_catch/209 is trying to lock:
    ffff8a743ed3e8a0 (&rq->__lock){-...}-{2:2}

Fixes: 69d8895cb9a9 ("rv: Add explicit lockdep context for reactors")
Suggested-by: Gabriele Monaco <gmonaco@redhat.com>
Signed-off-by: Wen Yang <wen.yang@linux.dev>
Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 kernel/trace/rv/rv_reactors.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 2f5fc8d18dea..5830229210e8 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -465,18 +465,28 @@ int init_rv_reactors(struct dentry *root_dir)
 
 void rv_react(struct rv_monitor *monitor, const char *msg, ...)
 {
-	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_FREE);
+	/*
+	 * Reactors must not explicitly take locks, so they should be
+	 * LD_WAIT_FREE.  However, reactor callbacks can run with preemption
+	 * enabled, meaning the preempting code (e.g. the scheduler taking
+	 * rq->__lock at LD_WAIT_SPIN) may violate that constraint.  Use
+	 * LD_WAIT_SPIN in preemptible contexts to avoid false-positive lockdep
+	 * reports; keep LD_WAIT_FREE in NMI/hardirq where preemption is
+	 * disabled and the scheduler cannot run.
+	 */
+	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map,        LD_WAIT_SPIN);
+	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map_atomic, LD_WAIT_FREE);
+	struct lockdep_map * __maybe_unused map;
 	va_list args;
 
 	if (!rv_reacting_on() || !monitor->react)
 		return;
 
+	map = (in_nmi() || in_hardirq()) ? &rv_react_map_atomic : &rv_react_map;
 	va_start(args, msg);
-
-	lock_map_acquire_try(&rv_react_map);
+	lock_map_acquire_try(map);
 	monitor->react(msg, args);
-	lock_map_release(&rv_react_map);
-
+	lock_map_release(map);
 	va_end(args);
 }
 EXPORT_SYMBOL_GPL(rv_react);
-- 
2.25.1

[PATCH v3 2/4] rv/reactors: propagate rv_register_reactor() error from reactor init

[wen.yang]

From: Wen Yang <wen.yang@linux.dev>

Both register_react_printk() and register_react_panic() ignore the
return value of rv_register_reactor() and always return 0.  If the
registration fails (e.g. a duplicate reactor name), the init functions
silently report success even though the reactor was not registered.

Propagate the error from rv_register_reactor() so a failed registration
is reported instead of being silently ignored.

Suggested-by: Gabriele Monaco <gmonaco@redhat.com>
Reviewed-by: Gabriele Monaco <gmonaco@redhat.com>
Signed-off-by: Wen Yang <wen.yang@linux.dev>
---
 kernel/trace/rv/reactor_panic.c  | 3 +--
 kernel/trace/rv/reactor_printk.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/rv/reactor_panic.c b/kernel/trace/rv/reactor_panic.c
index 76537b8a4343..db7116ceafff 100644
--- a/kernel/trace/rv/reactor_panic.c
+++ b/kernel/trace/rv/reactor_panic.c
@@ -26,8 +26,7 @@ static struct rv_reactor rv_panic = {
 
 static int __init register_react_panic(void)
 {
-	rv_register_reactor(&rv_panic);
-	return 0;
+	return rv_register_reactor(&rv_panic);
 }
 
 static void __exit unregister_react_panic(void)
diff --git a/kernel/trace/rv/reactor_printk.c b/kernel/trace/rv/reactor_printk.c
index 48c934e315b3..002a10f6aa7b 100644
--- a/kernel/trace/rv/reactor_printk.c
+++ b/kernel/trace/rv/reactor_printk.c
@@ -25,8 +25,7 @@ static struct rv_reactor rv_printk = {
 
 static int __init register_react_printk(void)
 {
-	rv_register_reactor(&rv_printk);
-	return 0;
+	return rv_register_reactor(&rv_printk);
 }
 
 static void __exit unregister_react_printk(void)
-- 
2.25.1

[PATCH v3 3/4] rv/reactors: export rv_register_reactor() and rv_unregister_reactor()

[wen.yang]

From: Wen Yang <wen.yang@linux.dev>

rv_react() is exported to modules, but the reactor registration helpers
are not.  Export them with EXPORT_SYMBOL_GPL() so reactor modules and
the tristate KUnit test module can register and unregister reactors
without hitting undefined symbol errors at link time(modpost).

Reviewed-by: Gabriele Monaco <gmonaco@redhat.com>
Signed-off-by: Wen Yang <wen.yang@linux.dev>
---
 kernel/trace/rv/rv_reactors.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 5830229210e8..5c85e3efe42e 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -314,6 +314,7 @@ int rv_register_reactor(struct rv_reactor *reactor)
 	guard(mutex)(&rv_interface_lock);
 	return __rv_register_reactor(reactor);
 }
+EXPORT_SYMBOL_GPL(rv_register_reactor);
 
 /**
  * rv_unregister_reactor - unregister a rv reactor.
@@ -327,6 +328,7 @@ int rv_unregister_reactor(struct rv_reactor *reactor)
 	list_del(&reactor->list);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(rv_unregister_reactor);
 
 /*
  * reacting_on interface.
-- 
2.25.1

[PATCH v3 4/4] rv/reactors: add KUnit tests for reactor registration and dispatch

[wen.yang]

From: Wen Yang <wen.yang@linux.dev>

Add KUnit tests covering the reactor register/unregister lifecycle
(including duplicate and name-length rejection) and rv_react() dispatch
(a no-op without a callback, exactly one invocation with one; the mdelay
callback keeps the CPU busy so a timer interrupt exercises the LD_WAIT_SPIN
lockdep context).  The Kconfig entry is tristate so the tests can be built
as a module when CONFIG_KUNIT=m; only RV_REACTORS is required.

Suggested-by: Gabriele Monaco <gmonaco@redhat.com>
Signed-off-by: Wen Yang <wen.yang@linux.dev>
---
 kernel/trace/rv/Kconfig             |  12 +++
 kernel/trace/rv/Makefile            |   1 +
 kernel/trace/rv/rv_reactors_kunit.c | 119 ++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+)
 create mode 100644 kernel/trace/rv/rv_reactors_kunit.c

diff --git a/kernel/trace/rv/Kconfig b/kernel/trace/rv/Kconfig
index efa930f94ea4..9bfd429ffdea 100644
--- a/kernel/trace/rv/Kconfig
+++ b/kernel/trace/rv/Kconfig
@@ -113,6 +113,18 @@ config RV_REACT_PANIC
 	  Enables the panic reactor. The panic reactor emits a printk()
 	  message if an exception is found and panic()s the system.
 
+config RV_REACTORS_KUNIT
+	tristate "KUnit tests for RV reactors" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	depends on RV_REACTORS
+	default KUNIT_ALL_TESTS
+	help
+	  Enable KUnit tests for RV reactor registration and dispatch.
+	  These tests verify the register/unregister lifecycle, duplicate
+	  rejection, and that rv_react() correctly invokes callbacks.
+
+	  If unsure, say N.
+
 config RV_MONITORS_KUNIT_TEST
 	tristate "KUnit tests for RV monitors" if !KUNIT_ALL_TESTS
 	depends on KUNIT && RV && RV_REACTORS
diff --git a/kernel/trace/rv/Makefile b/kernel/trace/rv/Makefile
index cdbf68c84f5a..c895d81dfdad 100644
--- a/kernel/trace/rv/Makefile
+++ b/kernel/trace/rv/Makefile
@@ -25,4 +25,5 @@ obj-$(CONFIG_RV_MON_WAKEUP) += monitors/wakeup/wakeup.o
 obj-$(CONFIG_RV_REACTORS) += rv_reactors.o
 obj-$(CONFIG_RV_REACT_PRINTK) += reactor_printk.o
 obj-$(CONFIG_RV_REACT_PANIC) += reactor_panic.o
+obj-$(CONFIG_RV_REACTORS_KUNIT) += rv_reactors_kunit.o
 obj-$(CONFIG_RV_MONITORS_KUNIT_TEST) += rv_monitors_test.o
diff --git a/kernel/trace/rv/rv_reactors_kunit.c b/kernel/trace/rv/rv_reactors_kunit.c
new file mode 100644
index 000000000000..d5b731eb11f3
--- /dev/null
+++ b/kernel/trace/rv/rv_reactors_kunit.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for RV reactor registration and dispatch.
+ */
+
+#include <kunit/test.h>
+#include <linux/rv.h>
+#include <linux/delay.h>
+#include <linux/atomic.h>
+#include "rv.h"
+
+static struct rv_reactor test_reactor = {
+	.name		= "kunit_test_reactor",
+	.description	= "KUnit test reactor",
+};
+
+static void reactor_teardown(void *arg)
+{
+	rv_unregister_reactor(&test_reactor);
+}
+
+static void register_test_reactor(struct kunit *test)
+{
+	KUNIT_ASSERT_EQ(test, rv_register_reactor(&test_reactor), 0);
+	KUNIT_ASSERT_EQ(test,
+			kunit_add_action_or_reset(test, reactor_teardown, NULL), 0);
+}
+
+static void test_double_register(struct kunit *test)
+{
+	register_test_reactor(test);
+	KUNIT_EXPECT_EQ(test, rv_register_reactor(&test_reactor), -EINVAL);
+}
+
+/*
+ * Use a fixed-size array so sizeof() gives the exact byte count at
+ * compile time.
+ */
+static const char long_reactor_name[] = "kunit_reactor_name_too_long_xxx_";
+_Static_assert(sizeof(long_reactor_name) - 1 >= MAX_RV_REACTOR_NAME_SIZE,
+	       "long_reactor_name must be at least MAX_RV_REACTOR_NAME_SIZE chars");
+
+static void test_name_too_long(struct kunit *test)
+{
+	static struct rv_reactor long_reactor = {
+		.name = long_reactor_name,
+	};
+
+	KUNIT_EXPECT_EQ(test, rv_register_reactor(&long_reactor), -EINVAL);
+}
+
+static struct kunit_case rv_reactor_registration_cases[] = {
+	KUNIT_CASE(test_double_register),
+	KUNIT_CASE(test_name_too_long),
+	{}
+};
+
+static struct kunit_suite rv_reactor_registration_suite = {
+	.name		= "rv_reactor_registration",
+	.test_cases	= rv_reactor_registration_cases,
+};
+
+static atomic_t react_call_count;
+
+__printf(1, 0) static void mock_react(const char *msg, va_list args)
+{
+	atomic_inc(&react_call_count);
+	/*
+	 * Hold the CPU for 5 ms so a timer interrupt is likely to fire
+	 * inside rv_react()'s lockdep context, exercising the LD_WAIT_SPIN
+	 * constraint.  mdelay() is a calibrated busy-wait with no scheduler
+	 * interaction.
+	 */
+	mdelay(5);
+}
+
+static void test_react_no_callback(struct kunit *test)
+{
+	struct rv_monitor monitor = {
+		.name = "kunit_null_react",
+	};
+
+	atomic_set(&react_call_count, 0);
+	rv_react(&monitor, "no callback");
+
+	/*
+	 * The only possible failure in this test case is a kernel panic.
+	 * NULL react guard: callback must NOT have been invoked
+	 */
+	KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 0);
+}
+
+static void test_react_callback_invoked(struct kunit *test)
+{
+	struct rv_monitor monitor = {
+		.name	= "kunit_dispatch_monitor",
+		.react	= mock_react,
+	};
+
+	atomic_set(&react_call_count, 0);
+	rv_react(&monitor, "callback invocation test");
+	KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 1);
+}
+
+static struct kunit_case rv_react_dispatch_cases[] = {
+	KUNIT_CASE(test_react_no_callback),
+	KUNIT_CASE(test_react_callback_invoked),
+	{}
+};
+
+static struct kunit_suite rv_react_dispatch_suite = {
+	.name		= "rv_react_dispatch",
+	.test_cases	= rv_react_dispatch_cases,
+};
+
+kunit_test_suites(&rv_reactor_registration_suite, &rv_react_dispatch_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("KUnit tests for RV reactor registration and dispatch");
-- 
2.25.1