rv/reactors: fix lockdep warning and add tests

From: Wen Yang <[email protected]>

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}

This series fixes the wait type (patch 1), makes reactor registration failures propagate (patch 2), adds support for module-based reactors (patch 3), and adds KUnit/kselftest coverage (patch 4-5).

Changes in v5:
- Patch 1: the locking rules no longer go to Documentation/trace/rv/monitor_synthesis.rst, which is not about reactors (Gabriele); they are documented in code comments. Functional code unchanged.
- Patch 2: unchanged
- Patch 3: redesigned. v4 only exported the registration helpers, which is harmless while all in-tree reactors are built-in, but would let a loudable reactor module be unloaded while a monitor still refereances its react() callback. Per the suggestion in commit 3d3800b4f7 ("rv: Remove rv_reactor's reference counter"). A new selftest (patch 5) exercises it.
- Patch 4: The test callback is kept busy for two timer ticks so the test also passes with HZ < 200.
- Patch 5: a selftest module that registers a reactor exercises the module pinni g end to end: rmmod must fail while a monitor is attached.

v4: https://lore.kernel.org/lkml/[email protected]/
v3: https://lore.kernel.org/lkml/[email protected]/
v2: https://lore.kernel.org/lkml/[email protected]/
v1: https://lore.kernel.org/lkml/[email protected]/

Wen Yang (5):
rv/reactors: use LD_WAIT_SPIN as the reactor lockdep wait type 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 selftests/verification: Test loadable module-based reactor

 include/linux/rv.h                            |   3 +
 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.c                          |   5 +
 kernel/trace/rv/rv_reactors.c                 |  48 ++++++--
 kernel/trace/rv/rv_reactors_kunit.c           | 110 ++++++++++++++++++
 tools/testing/selftests/verification/Makefile |   1 +
 tools/testing/selftests/verification/config   |   2 +
 .../test.d/rv_reactor_loadable.tc             |  46 ++++++++
 .../verification/test_modules/Makefile        |  16 +++
 .../test_modules/rv_test_reactor.c            |  37 ++++++
 13 files changed, 276 insertions(+), 11 deletions(-)
 create mode 100644 kernel/trace/rv/rv_reactors_kunit.c
 create mode 100644 tools/testing/selftests/verification/test.d/rv_reactor_loadable.tc
 create mode 100644 tools/testing/selftests/verification/test_modules/Makefile
 create mode 100644 tools/testing/selftests/verification/test_modules/rv_test_reactor.c

base-commit: 785095112f4198de49760552374f364043c8dbdf

2.25.1

From: Wen Yang <[email protected]>

rv_react() overrides the lockdep wait type to LD_WAIT_FREE to enforce that reactor callbacks take no locks. But callbacks run in the context of the triggering tracepoint, which can be preemptible task context on any kernel. A timer interrupt firing during the callback makes the interrupt-exit path schedule and take rq->__lock (LD_WAIT_SPIN) while the LD_WAIT_FREE override is still held, producing 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}

Use LD_WAIT_SPIN instead of LD_WAIT_FREE, which causes false-positive warnings in preemptible contexts due to scheduler preemption taking rq->__lock. Add documentation to runtime-verification.rst.

---
 kernel/trace/rv/rv_reactors.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

index 2f5fc8d18dea..ff7d478227c3 100644
@@ -465,7 +465,15 @@ 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 to avoid false-positive lockdep reports.
+	 * But you should still NOT be using locks in reactors.
+	 */
+	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_SPIN);
 	va_list args;
 
 	if (!rv_reacting_on() || !monitor->react)

2.25.1

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.

---
 kernel/trace/rv/reactor_panic.c  | 3 +--
 kernel/trace/rv/reactor_printk.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

index 76537b8a4343..db7116ceafff 100644
@@ -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)

index 48c934e315b3..002a10f6aa7b 100644
@@ -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

From: Wen Yang <[email protected]>

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

Commit 3d3800b4f7f4 ("rv: Remove reactor's reference counter") noted that if module-based reactors are supported, try_module_get()/module_put() should be used. Add struct module *owner to struct rv_reactor so a module cat set owner = THIS_MODULE; pin the module in monitor_swap_reactors_gingle() and release it when a monitor detaches or is unregistered.

In-tree reactors leave owner = NULL and are unaffected.

---
 include/linux/rv.h            |  3 +++
 kernel/trace/rv/rv.c          |  5 +++++
 kernel/trace/rv/rv_reactors.c | 38 +++++++++++++++++++++++++++++------
 3 files changed, 40 insertions(+), 6 deletions(-)

index 541ba404926a..ff3289ba4f02 100644
@@ -128,10 +128,13 @@ union rv_task_monitor {
 };
 
 #ifdef CONFIG_RV_REACTORS
+struct module;
+
 struct rv_reactor {
 	const char		*name;
 	const char		*description;
 	__printf(1, 0) void	(*react)(const char *msg, va_list args);
+	struct module		*owner;
 	struct list_head	list;
 };
 #endif

index 29f155c6968b..458b17c005b3 100644
@@ -803,6 +803,11 @@ int rv_unregister_monitor(struct rv_monitor *monitor)
 	guard(mutex)(&rv_interface_lock);
 
 	rv_disable_monitor(monitor);
+#ifdef CONFIG_RV_REACTORS
+	if (monitor->reactor)
+		module_put(monitor->reactor->owner);
+
+#endif
 	list_del(&monitor->list);
 	destroy_monitor_dir(monitor);
 

index ff7d478227c3..136eb7f47c4a 100644
@@ -62,6 +62,7 @@
  */
 
 #include <linux/lockdep.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 
 #include "rv.h"

@@ -159,7 +160,7 @@ static const struct seq_operations monitor_reactors_seq_ops = {
 	.show	= monitor_reactor_show
 };
 
-static void monitor_swap_reactors_single(struct rv_monitor *mon,
+static int monitor_swap_reactors_single(struct rv_monitor *mon,
 					 struct rv_reactor *reactor,
 					 bool nested)
 {

@@ -167,29 +168,39 @@ static void monitor_swap_reactors_single(struct rv_monitor *mon,
 
 	/* nothing to do */
 	if (mon->reactor == reactor)
-		return;
+		return 0;
+
+	if (reactor->owner && !try_module_get(reactor->owner))
+		return -EBUSY;
 
 	monitor_enabled = mon->enabled;
 	if (monitor_enabled)
 		rv_disable_monitor(mon);
 
+	if (mon->reactor)
+		module_put(mon->reactor->owner);
 	mon->reactor = reactor;
 	mon->react = reactor->react;
 
 	/* enable only once if iterating through a container */
 	if (monitor_enabled && !nested)
 		rv_enable_monitor(mon);
+
+	return 0;
 }
 
-static void monitor_swap_reactors(struct rv_monitor *mon, struct rv_reactor *reactor)
+static int monitor_swap_reactors(struct rv_monitor *mon, struct rv_reactor *reactor)
 {
 	struct rv_monitor *p = mon;
+	int ret;
 
 	if (rv_is_container_monitor(mon))
 		list_for_each_entry_continue(p, &rv_monitors_list, list) {
 			if (p->parent != mon)
 				break;
-			monitor_swap_reactors_single(p, reactor, true);
+			ret = monitor_swap_reactors_single(p, reactor, true);
+			if (ret)
+				return ret;
 		}
 	/*
 	 * This call enables and disables the monitor if they were active.

@@ -197,7 +208,7 @@ static void monitor_swap_reactors(struct rv_monitor *mon, struct rv_reactor *rea
 	 * All nested monitors are enabled also if they were off, we may refine
 	 * this logic in the future.
 	 */
-	monitor_swap_reactors_single(mon, reactor, false);
+	return monitor_swap_reactors_single(mon, reactor, false);
 }
 
 static ssize_t

@@ -236,10 +247,14 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
 	guard(mutex)(&rv_interface_lock);
 
 	list_for_each_entry(reactor, &rv_reactors_list, list) {
+		int ret;
+
 		if (strcmp(ptr, reactor->name) != 0)
 			continue;
 
-		monitor_swap_reactors(mon, reactor);
+		ret = monitor_swap_reactors(mon, reactor);
+		if (ret)
+			return ret;
 
 		return count;
 	}

@@ -314,6 +329,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 +343,7 @@ int rv_unregister_reactor(struct rv_reactor *reactor)
 	list_del(&reactor->list);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(rv_unregister_reactor);
 
 /*
  * reacting_on interface.

@@ -421,6 +438,15 @@ int reactor_populate_monitor(struct rv_monitor *mon, struct dentry *root)
 	 * Configure as the rv_nop reactor.
 	 */
 	mon->reactor = get_reactor_rdef_by_name("nop");
+	if (WARN_ON(!mon->reactor)) {
+		rv_remove(tmp);
+		return -EINVAL;
+	}
+
+	if (mon->reactor->owner && !try_module_get(mon->reactor->owner)) {
+		rv_remove(tmp);
+		return -EBUSY;
+	}
 
 	return 0;
 }

2.25.1

You needed these symbols in KUnit and we are exporting them for /potential/ future support of reactors as modules. I don't see any technical reason why we shouldn't support this, but they are currently /not/ supported.

I know sashiko and other LLMs complain about this, and they have a point, but you can ignore them. At most state in this commit message that this does NOT add support for reactors as modules.

Let's focus this series on its original intent (fix a lockdep warning and add some KUnit tests that expose a reproducer), then if adding support for reactors as modules is so simple, you can do it in another series.

If you really want to /also/ add support for reactors as modules in this series, you need to make that very explicit (not just a vague line in the changelog, but rather rewrite the entire cover letter and commit message).
And mind that this would mean your series needs to go through another round of review and serious testing: you are adding a new feature.

Please, whenever you significantly change an already reviewed patch, remove the reviewed-by, so I can quickly see I need to review it again.

Thanks,
Gabriele

Thanks for the review, Gabriele, I agree with your points and will follow them.

Just to clarify the background : the struct module * field and the try_module_get()/module_put() logic were added to address the point raised by sashiko. After this change sashiko no longer complains about it.

The module-based reactor support, together with its kselftest, will be send as a separate series in a few days.

As a side note, we are looking into using the rv framework for functional safety in autonomous driving. Depending on how internal validation progress, there will likely be further changes to the rv subsystem in the near future. I'll be glad to discuss them as they come.

--
Best wishes,
Wen

Yeah I imagined it, sashiko has a point, whatever is EXPORTed can be used by modules, so it may imply modules are supported.
What sashiko ignores is maintainability requirements.
A human is reviewing anyone trying to use reactors as modules, and adding that change without properly advertising it may do more harm than good.

Sounds good, so I can review and test it as it deserves.

Nice, thanks for sharing, I had a few conversations about RV in functional safety domains over the past weeks, I now better understand the challenges. It'd be very interesting to discuss your requirements too.

Thanks,
Gabriele

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 lands inside rv_react()'s lockdep context, exercising the LD_WAIT_SPIN wait type from the previous patch; a spurious lockdep splat there would show up in the test output.

The dispatch tests rely o reacting_on being enabled, since rv_react() returns early when it if off.

---
 kernel/trace/rv/Kconfig             |  12 +++
 kernel/trace/rv/Makefile            |   1 +
 kernel/trace/rv/rv_reactors_kunit.c | 110 ++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)
 create mode 100644 kernel/trace/rv/rv_reactors_kunit.c

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

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

2.25.1

From: Wen Yang <[email protected]>

Add a selftest module that register an RV reactor and a test that exercises the module pinning: rmmod must fail while a monitor is attached to the reactor and succeed after it is detached. A trap unloads the module on failure so it does not break the next insmod.

THe module is built through the kselftest TEST_GEN_MODS_DIR mechanism, like the livepatch selftests.

---
 tools/testing/selftests/verification/Makefile |  1 +
 tools/testing/selftests/verification/config   |  2 +
 .../test.d/rv_reactor_loadable.tc             | 46 +++++++++++++++++++
 .../verification/test_modules/Makefile        | 16 +++++++
 .../test_modules/rv_test_reactor.c            | 37 +++++++++++++++
 5 files changed, 102 insertions(+)
 create mode 100644 tools/testing/selftests/verification/test.d/rv_reactor_loadable.tc
 create mode 100644 tools/testing/selftests/verification/test_modules/Makefile
 create mode 100644 tools/testing/selftests/verification/test_modules/rv_test_reactor.c

index aa8790c22a71..7ff7382d11f0 100644
@@ -3,6 +3,7 @@ all:
 
 TEST_PROGS := verificationtest-ktap
 TEST_FILES := test.d settings
+TEST_GEN_MODS_DIR := test_modules
 EXTRA_CLEAN := $(OUTPUT)/logs/*
 
 include ../lib.mk

index 43072c1c38f4..ddd7581f07b9 100644
@@ -1 +1,3 @@
 CONFIG_RV=y
+CONFIG_MODULES=y
+CONFIG_MODULES_UNLOAD=y

new file mode 100644
index 000000000000..0af95207a9a9
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0
+
+TESTMODS_DIR := $(realpath $(dir $(abspath $(lastword $(MAKEFILE_LIST)))))
+KDIR ?= /lib/modules/$(shell uname -r)/build
+
+obj-m += rv_test_reactor.o
+
+modules:
+ifneq ("$(wildcard $(KDIR))", "")
+	$(Q)$(MAKE) -C $(KDIR) modules KBUILD_EXTMOD=$(TESTMODS_DIR)
+endif
+
+clean:
+ifneq ("$(wildcard $(KDIR))", "")
+	$(Q)$(MAKE) -C $(KDIR) clean KBUILD_EXTMOD=$(TESTMODS_DIR)
+endif

new file mode 100644
index 000000000000..a243df81fa52
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Loadable RV reactor for the verification selftests. Register a
+ * reactor with owner = THIS_MODULE so the selftests can exercise the
+ * module pinning: unloading is refused while a monitor is attached
+ * to the reactor.
+ */
+
+#include <linux/module.h>
+#include <linux/rv.h>
+
+__printf(1, 0) static void rv_test_reaction(const char *msg, va_list args)
+{
+}
+
+static struct rv_reactor rv_test_reactor = {
+	.name = "test_reactor",
+	.description = "selftest reactor: exercise module-based reactors.",
+	.react = rv_test_reaction,
+	.owner = THIS_MODULE,
+};
+
+static int __init rv_test_reactor_init(void)
+{
+	return rv_register_reactor(&rv_test_reactor);
+}
+
+static void __exit rv_test_reactor_exit(void)
+{
+	rv_unregister_reactor(&rv_test_reactor);
+}
+
+module_init(rv_test_reactor_init);
+module_exit(rv_test_reactor_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Loadable RV reactor for verification selftests");

2.25.1