rv/reactors: fix lockdep warning and add KUnit tests

From: Wen Yang <[email protected]>

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 <[email protected]>

Signed-off-by Wen Yang <[email protected]>

Cc Thomas Weißschuh <[email protected]>

---
 kernel/trace/rv/rv_reactors.c | 20 +++++++++++++++-----

 1 file changed, 15 insertions(+), 5 deletions(-)

kernel/trace/rv/rv_reactors.c +0 -0

index 2f5fc8d18dea..5830229210e8 100644
@@ -465,18 +465,28 @@ int init_rv_reactors(struct dentry *root_dir)

 void rv_react(struct rv_monitor *monitor, const char *msg, ...) {

kernel/trace/rv/rv_reactors.c +6 -1

-	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

kernel/trace/rv/rv_reactors.c +6 -0

+	 * 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;

kernel/trace/rv/rv_reactors.c +1 -0

+	map = (in_nmi() || in_hardirq()) ? &rv_react_map_atomic :

&rv_react_map;
  va_start(args, msg);

kernel/trace/rv/rv_reactors.c +1 -2

-
-	lock_map_acquire_try(&rv_react_map);
+	lock_map_acquire_try(map);

  monitor->react(msg, args);

kernel/trace/rv/rv_reactors.c +1 -2

-	lock_map_release(&rv_react_map);
-
+	lock_map_release(map);

  va_end(args);
 }
 EXPORT_SYMBOL_GPL(rv_react);

Quoted 9 lines · On Mon, 2026-08-10 at 01:10 +0800, [email protected] wrote:

From: Wen Yang <[email protected]>

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:

Anyway, I'd appreciate comments/acks from the other folks in the loop

Quoted 10 lines · Gabriele Monaco

Quoted 9 lines · On Mon, 2026-08-10 at 01:10 +0800, [email protected] wrote:

From: Wen Yang <[email protected]>

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:

...

Quoted 1 line

Anyway, I'd appreciate comments/acks from the other folks in the loop

Sorry, I do not know enough about lockdep to comment on this.

FWIW, I would rather just use LD_WAIT_SPIN and keep things simple. Context-sensitive code paths "feels wrong" to me. Spinning should either be allowed or forbidden. Making it dynamic "feels like" it will bring further complications down the road.

But that's just my intuition.

Quoted 13 lines · Nam Cao

Quoted 10 lines · Gabriele Monaco

Quoted 9 lines · On Mon, 2026-08-10 at 01:10 +0800, [email protected] wrote:

From: Wen Yang <[email protected]>

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:

...

Quoted 8 lines

Quoted 1 line

Anyway, I'd appreciate comments/acks from the other folks in the loop

Sorry, I do not know enough about lockdep to comment on this.

FWIW, I would rather just use LD_WAIT_SPIN and keep things simple. Context-sensitive code paths "feels wrong" to me. Spinning should either be allowed or forbidden. Making it dynamic "feels like" it will bring further complications down the road.

Quoted 1 line

But that's just my intuition.

I don't have a strong opinion on this, but since there's no one in the kernel using LD_WAIT_FREE as inner type, that feels like a hint to go down the simple route too and allow LD_WAIT_SPIN.

If a reactor ever uses spinlocks, lockdep would already complain on its own if that ends up being an issue, wouldn't it?

Quoted 18 lines · Gabriele Monaco

Quoted 17 lines · Nam Cao

Quoted 10 lines · Gabriele Monaco

Quoted 9 lines · On Mon, 2026-08-10 at 01:10 +0800, [email protected] wrote:

From: Wen Yang <[email protected]>

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:

...

Quoted 1 line

Anyway, I'd appreciate comments/acks from the other folks in the loop

Sorry, I do not know enough about lockdep to comment on this.

Quoted 4 lines

Quoted 4 lines

FWIW, I would rather just use LD_WAIT_SPIN and keep things simple. Context-sensitive code paths "feels wrong" to me. Spinning should either be allowed or forbidden. Making it dynamic "feels like" it will bring further complications down the road.

To me the dynamic logic also feels quite complicated.
We could also disable preemption before overriding the lockdep context when lockdep is enabled to avoid the observed issue.

Quoted 8 lines

Quoted 1 line

But that's just my intuition.

I don't have a strong opinion on this, but since there's no one in the kernel using LD_WAIT_FREE as inner type, that feels like a hint to go down the simple route too and allow LD_WAIT_SPIN.

If a reactor ever uses spinlocks, lockdep would already complain on its own if that ends up being an issue, wouldn't it?

Only if that reactor is actually triggered by a tracepoint in the wrong context. This might not happen during testing. This happened to me in my signal reactor patch, which is why I added the lockdep override.

Thomas

So I don't think LD_WAIT_SPIN for everything is safe, it's not just "less strict on paper".

If a reactor ever does raw_spin_lock() by mistake while running from nrp's path, check_wait_context() takes the wait type straight from the override map:

     if (unlikely(class->lock_type == LD_LOCK_WAIT_OVERRIDE)) curr_inner = prev_inner;        /* SPIN */ if (next_outer > curr_inner)
             return print_lock_invalid_wait_context(...);

SPIN nested in SPIN is 2 > 2, which is false, so it just passes. We'd be silently giving up the one case (hardirq/NMI) where the "no locks" rule for reactors actually matters, which is the same thing that bit you with the signal reactor.

And for what it's worth, checking context to decide the wait type isn't something we'd be introducing -- lockdep does the same thing to get the baseline before any override is applied, in the exact function that produced this splat:

     static inline short task_wait_context(struct task_struct *curr) {
             if (lockdep_hardirq_context()) {
                     if (curr->hardirq_threaded || curr->irq_config) return LD_WAIT_CONFIG; return LD_WAIT_SPIN;
             } else if (curr->softirq_context) {
                     return LD_WAIT_CONFIG;
             }
             return LD_WAIT_MAX;
     }

That's four cases. Our in_nmi() || in_hardirq() is a simplification of what's already there, not a new habit.

Thomas, on disabling preemption instead: it does fix the pagefault case, and it's a no-op for nrp since preempt_count is already elevated there. But it changes what a reactor is allowed to do, for every reactor, not just the two above.cspin_lock() on RT checks might_resched() before it even looks at thevlock:

     static __always_inline void __rt_spin_lock(spinlock_t *lock) {
             rtlock_might_resched();   /* unconditional */ rtlock_lock(&lock->lock);
     }

so any future reactor using a plain spinlock would hit that every single time, lock contended or not, on top of whatever it's already called for. That seems worse than the thing we're trying to fix.

Since neither struct rv_reactor nor rv_react() actually documents what context a callback may run in, maybe that's worth spelling out separately regardless of what we do here -- something close to what printk already does for the same reason (reactor_printk's vprintk_deferred() leans on this internally: is_printk_legacy_deferred() checks in_nmi() to decide whether it's safe to take console_lock or whether it has to go through the lock-free irq_work path instead).

From: Wen Yang <[email protected]>

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 <[email protected]>

Reviewed-by Gabriele Monaco <[email protected]>

Signed-off-by Wen Yang <[email protected]>

From: Wen Yang <[email protected]>

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 <[email protected]>

Signed-off-by Wen Yang <[email protected]>

---
 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

kernel/trace/rv/Kconfig +0 -0

index efa930f94ea4..9bfd429ffdea 100644
@@ -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.

kernel/trace/rv/Kconfig +12 -0

+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

kernel/trace/rv/Makefile +0 -0

index cdbf68c84f5a..c895d81dfdad 100644
@@ -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

kernel/trace/rv/rv_reactors_kunit.c new file +26 -0

new file mode 100644
index 000000000000..d5b731eb11f3
@@ -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);

kernel/trace/rv/rv_reactors_kunit.c new file +15 -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");

kernel/trace/rv/rv_reactors_kunit.c new file +78 -0

+
+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");

+/*
+ * Use a fixed-size array so sizeof() gives the exact byte count at
+ * compile time.
+ */

kernel/trace/rv/rv_reactors_kunit.c new file +8 -0

+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,

kernel/trace/rv/rv_reactors_kunit.c new file +6 -0

+	mdelay(5);
+}
+
+static void test_react_no_callback(struct kunit *test)
+{
+	struct rv_monitor monitor = {

kernel/trace/rv/rv_reactors_kunit.c new file +4 -0

+	KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 0);
+}
+
+static void test_react_callback_invoked(struct kunit *test)

Quoted 4 lines · [email protected] writes:

+/*
+ * Use a fixed-size array so sizeof() gives the exact byte count at
+ * compile time.
+ */

There is no need for a comment. This is obvious from the code itself.

Quoted 9 lines

kernel/trace/rv/rv_reactors_kunit.c new file +3 -0

+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");

kernel/trace/rv/rv_reactors_kunit.c new file +5 -0

+
+static void test_name_too_long(struct kunit *test)
+{
+	static struct rv_reactor long_reactor = {
+		.name = long_reactor_name,

kernel/trace/rv/rv_reactors_kunit.c line 56

Quoted 10 lines

kernel/trace/rv/rv_reactors_kunit.c new file +10 -0

+	};
+
+	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),
+	{}
+};

I am not sure about the usefulness of these test cases. rv_register_reactor() is not an user API that we have to prepare for corner case usage. We can expect its users to be sane since the users are us.

But well, if Gabriele wants to keep them..

kernel/trace/rv/rv_reactors_kunit.c line 63

Quoted 7 lines

kernel/trace/rv/rv_reactors_kunit.c new file +7 -0

+
+static struct kunit_suite rv_reactor_registration_suite = {
+	.name		= "rv_reactor_registration",
+	.test_cases	= rv_reactor_registration_cases,
+};
+
+static atomic_t react_call_count;

Do we really need atomic_t? Does int work?

Review

Quoted 12 lines

kernel/trace/rv/rv_reactors_kunit.c new file +8 -0

+
+__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

(quoted diff) +2 -0

+	 * interaction.
+	 */

The comment above mdelay()'s definition already explains what it does.

Quoted 6 lines

kernel/trace/rv/rv_reactors_kunit.c new file +6 -0

+	mdelay(5);
+}
+
+static void test_react_no_callback(struct kunit *test)
+{
+	struct rv_monitor monitor = {

kernel/trace/rv/rv_reactors_kunit.c line 89

Quoted 10 lines

kernel/trace/rv/rv_reactors_kunit.c new file +10 -0

+		.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
+	 */

Obvious comment.

Quoted 10 lines

kernel/trace/rv/rv_reactors_kunit.c new file +10 -0

+	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,
+	};
+

kernel/trace/rv/rv_reactors_kunit.c line 103

Quoted 4 lines

kernel/trace/rv/rv_reactors_kunit.c new file +4 -0

+	atomic_set(&react_call_count, 0);
+	rv_react(&monitor, "callback invocation test");
+	KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 1);
+}

So the test calls rv_react(), and validates that the reactor is called? Honestly I am not sure how useful that is. Especially since Gabriele already made the selftest which validates that the reactor is invoked.