[PATCH] rv: Add signal reactor

[PATCH] rv: Add signal reactor

[Thomas Weißschuh]

Reactions of the existing reactors are not easily detectable from programs
and also not easily attributable to the triggering task.
This makes it hard to test the monitors themselves programmatically.

The signal reactors allows applications to validate the correct operations
of monitors either by installing custom signal handlers or by forking a
child and waiting for the expected exit code.

For now only SIGBUS is supported, but additional signals can be added.

As the reactors are called from tracepoints they need to be able to run in
any context. To avoid taking any of the looks used during signal delivery
from an invalid context, schedule it through task_work. The delivery will
be delayed, for example when then sleep monitor detects an invalid sleep.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 kernel/trace/rv/Kconfig          |   8 +++
 kernel/trace/rv/Makefile         |   1 +
 kernel/trace/rv/reactor_signal.c | 114 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)

diff --git a/kernel/trace/rv/Kconfig b/kernel/trace/rv/Kconfig
index 5b4be87ba59d3fa5123a64efa746320c9e8b73b1..dc7b546615a67c811ce94c43bb1db2826cd00c77 100644
--- a/kernel/trace/rv/Kconfig
+++ b/kernel/trace/rv/Kconfig
@@ -93,3 +93,11 @@ config RV_REACT_PANIC
 	help
 	  Enables the panic reactor. The panic reactor emits a printk()
 	  message if an exception is found and panic()s the system.
+
+config RV_REACT_SIGNAL
+	bool "Signal reactor"
+	depends on RV_REACTORS
+	default y
+	help
+	  Enables the signal reactor. The signal reactors sends a signal to the
+	  task triggering an exception.
diff --git a/kernel/trace/rv/Makefile b/kernel/trace/rv/Makefile
index 42ff5d5aa9dde3ed2964e0b8d4ab7b236f498319..adf56bbd03ffa0d48de1f0d86ca5fcce1628bdba 100644
--- a/kernel/trace/rv/Makefile
+++ b/kernel/trace/rv/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_RV_MON_OPID) += monitors/opid/opid.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_REACT_SIGNAL) += reactor_signal.o
diff --git a/kernel/trace/rv/reactor_signal.c b/kernel/trace/rv/reactor_signal.c
new file mode 100644
index 0000000000000000000000000000000000000000..e123786d94371a240beb63b2d1b2f3044a466404
--- /dev/null
+++ b/kernel/trace/rv/reactor_signal.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2025 Thomas Weißschuh, Linutronix GmbH
+ *
+ * Signal RV reactor:
+ *   Prints the exception msg to the kernel message log and sends a signal to the offending task.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/cpumask.h>
+#include <linux/init.h>
+#include <linux/mempool.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/rv.h>
+#include <linux/sched/signal.h>
+#include <linux/task_work.h>
+
+struct rv_signal_work {
+	struct callback_head twork;
+	int signal;
+	char message[256];
+};
+
+static mempool_t *rv_signal_task_work_pool;
+
+static void rv_signal_force_sig(int signal, const char *message)
+{
+	/* The message already contains a subsystem prefix, so use raw printk() */
+	printk(KERN_WARNING "%s", message);
+	pr_warn("Killing PID %d with signal %d", task_pid_nr(current), signal);
+	force_sig(signal);
+}
+
+static void rv_signal_task_work(struct callback_head *cbh)
+{
+	struct rv_signal_work *work = container_of_const(cbh, struct rv_signal_work, twork);
+
+	rv_signal_force_sig(work->signal, work->message);
+
+	mempool_free(work, rv_signal_task_work_pool);
+}
+
+static void rv_reaction_signal(int signal, const char *fmt, va_list args)
+{
+	struct rv_signal_work *work;
+	char message[256];
+
+	work = mempool_alloc_preallocated(rv_signal_task_work_pool);
+	if (!work) {
+		pr_warn_ratelimited("Unable to signal through task_work, sending directly\n");
+		vsnprintf(message, sizeof(message), fmt, args);
+		rv_signal_force_sig(signal, message);
+		return;
+	}
+
+	init_task_work(&work->twork, rv_signal_task_work);
+	work->signal = signal;
+	vsnprintf(work->message, sizeof(work->message), fmt, args);
+
+	/*
+	 * The reactor can be called from any context through tracepoints.
+	 * To avoid any locking or other operations not usable from all contexts, use TWA_RESUME.
+	 * The signal might be delayed, but that shouldn't be an issue.
+	 */
+	task_work_add(current, &work->twork, TWA_RESUME);
+}
+
+__printf(1, 2)
+static void rv_reaction_sigbus(const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	rv_reaction_signal(SIGBUS, fmt, args);
+	va_end(args);
+}
+
+static struct rv_reactor rv_sigbus = {
+	.name		= "sigbus",
+	.description	= "Kill the current task with SIGBUS",
+	.react		= rv_reaction_sigbus,
+};
+
+static int __init register_react_signal(void)
+{
+	int ret;
+
+	rv_signal_task_work_pool = mempool_create_kmalloc_pool(num_possible_cpus(),
+							       sizeof(struct rv_signal_work));
+	if (!rv_signal_task_work_pool)
+		return -ENOMEM;
+
+	ret = rv_register_reactor(&rv_sigbus);
+	if (ret) {
+		mempool_destroy(rv_signal_task_work_pool);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void __exit unregister_react_signal(void)
+{
+	rv_unregister_reactor(&rv_sigbus);
+	mempool_destroy(rv_signal_task_work_pool);
+}
+
+module_init(register_react_signal);
+module_exit(unregister_react_signal);
+
+MODULE_AUTHOR("Thomas Weißschuh <thomas.weissschuh@linutronix.de>");
+MODULE_DESCRIPTION("signal rv reactor: send a signal if an exception is found.");

---
base-commit: adbdaac63f5024a9a117ef8ce9732a4272fbc931
change-id: 20250901-rv-reactor-signal-b5568e0722a9

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Re: [PATCH] rv: Add signal reactor

[Gabriele Monaco]

On Fri, 2025-09-19 at 12:49 +0200, Thomas Weißschuh wrote:
> Reactions of the existing reactors are not easily detectable from programs
> and also not easily attributable to the triggering task.
> This makes it hard to test the monitors themselves programmatically.
> 
> The signal reactors allows applications to validate the correct operations
> of monitors either by installing custom signal handlers or by forking a
> child and waiting for the expected exit code.

Thanks, this looks like a really nice addition!

> For now only SIGBUS is supported, but additional signals can be added.

Curious choice of a default signal, is this specific to your use-case?

We may want to add some kind of reactors options in the future to allow
configuring this, but I'd say it isn't needed for now.

> As the reactors are called from tracepoints they need to be able to run in
> any context. To avoid taking any of the looks used during signal delivery

You probably meant "locks"

> from an invalid context, schedule it through task_work. The delivery will
> be delayed, for example when then sleep monitor detects an invalid sleep.
> 
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>  kernel/trace/rv/Kconfig          |   8 +++
>  kernel/trace/rv/Makefile         |   1 +
>  kernel/trace/rv/reactor_signal.c | 114
> +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 123 insertions(+)
> 
> diff --git a/kernel/trace/rv/Kconfig b/kernel/trace/rv/Kconfig
> index
> 5b4be87ba59d3fa5123a64efa746320c9e8b73b1..dc7b546615a67c811ce94c43bb1db2826cd0
> 0c77 100644
> --- a/kernel/trace/rv/Kconfig
> +++ b/kernel/trace/rv/Kconfig
> @@ -93,3 +93,11 @@ config RV_REACT_PANIC
>  	help
>  	  Enables the panic reactor. The panic reactor emits a printk()
>  	  message if an exception is found and panic()s the system.
> +
> +config RV_REACT_SIGNAL
> +	bool "Signal reactor"
> +	depends on RV_REACTORS
> +	default y
> +	help
> +	  Enables the signal reactor. The signal reactors sends a signal to
> the
> +	  task triggering an exception.

This assumption is not always correct, imagine a failure in the sleep monitor
caused by the wakeup event. The offending task is not current but the wakee.

This is a bit tricky because reactors don't have access to that task, just to
keep the same implementation between per-cpu and per-task monitors.

We may want to revise that, perhaps like Nam is doing with the monitor_target
thing [1].

Besides, I'm assuming this reactor is only meaningful for monitors written to
validate userspace tasks (signals and TWA_RESUME are probably
meaningless/dangerous for kernel threads).

I'm fine with that but you should probably mention it here and/or in the reactor
itself, since we have also monitors validating kernel behaviour (see the sched
collection).

[1] -
https://lore.kernel.org/lkml/9a1b5a8c449fcb4f1a671016389c1e4fca49a351.1754900299.git.namcao@linutronix.de

> diff --git a/kernel/trace/rv/Makefile b/kernel/trace/rv/Makefile
> index
> 42ff5d5aa9dde3ed2964e0b8d4ab7b236f498319..adf56bbd03ffa0d48de1f0d86ca5fcce1628
> bdba 100644
> --- a/kernel/trace/rv/Makefile
> +++ b/kernel/trace/rv/Makefile
> @@ -23,3 +23,4 @@ obj-$(CONFIG_RV_MON_OPID) += monitors/opid/opid.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_REACT_SIGNAL) += reactor_signal.o
> diff --git a/kernel/trace/rv/reactor_signal.c
> b/kernel/trace/rv/reactor_signal.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..e123786d94371a240beb63b2d1b2f3044a46
> 6404
> --- /dev/null
> +++ b/kernel/trace/rv/reactor_signal.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2025 Thomas Weißschuh, Linutronix GmbH
> + *
> + * Signal RV reactor:
> + *   Prints the exception msg to the kernel message log and sends a signal to
> the offending task.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/cpumask.h>
> +#include <linux/init.h>
> +#include <linux/mempool.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/rv.h>
> +#include <linux/sched/signal.h>
> +#include <linux/task_work.h>
> +
> +struct rv_signal_work {
> +	struct callback_head twork;
> +	int signal;
> +	char message[256];
> +};
> +
> +static mempool_t *rv_signal_task_work_pool;
> +
> +static void rv_signal_force_sig(int signal, const char *message)
> +{
> +	/* The message already contains a subsystem prefix, so use raw
> printk() */
> +	printk(KERN_WARNING "%s", message);
> +	pr_warn("Killing PID %d with signal %d", task_pid_nr(current),
> signal);
> +	force_sig(signal);
> +}
> +
> +static void rv_signal_task_work(struct callback_head *cbh)
> +{
> +	struct rv_signal_work *work = container_of_const(cbh, struct
> rv_signal_work, twork);
> +
> +	rv_signal_force_sig(work->signal, work->message);
> +
> +	mempool_free(work, rv_signal_task_work_pool);
> +}
> +
> +static void rv_reaction_signal(int signal, const char *fmt, va_list args)
> +{
> +	struct rv_signal_work *work;
> +	char message[256];
> +
> +	work = mempool_alloc_preallocated(rv_signal_task_work_pool);
> +	if (!work) {
> +		pr_warn_ratelimited("Unable to signal through task_work,
> sending directly\n");
> +		vsnprintf(message, sizeof(message), fmt, args);
> +		rv_signal_force_sig(signal, message);
> +		return;
> +	}

Why do you use the task_work at all instead of signalling directly?
If that's something not safe from a (any) tracepoint because it can sleep you
should definitely not call it if allocation fails.

> +
> +	init_task_work(&work->twork, rv_signal_task_work);
> +	work->signal = signal;
> +	vsnprintf(work->message, sizeof(work->message), fmt, args);
> +
> +	/*
> +	 * The reactor can be called from any context through tracepoints.
> +	 * To avoid any locking or other operations not usable from all
> contexts, use TWA_RESUME.
> +	 * The signal might be delayed, but that shouldn't be an issue.
> +	 */
> +	task_work_add(current, &work->twork, TWA_RESUME);
> +}
> +
> +__printf(1, 2)
> +static void rv_reaction_sigbus(const char *fmt, ...)
> +{
> +	va_list args;
> +
> +	va_start(args, fmt);
> +	rv_reaction_signal(SIGBUS, fmt, args);
> +	va_end(args);
> +}
> +
> +static struct rv_reactor rv_sigbus = {
> +	.name		= "sigbus",
> +	.description	= "Kill the current task with SIGBUS",
> +	.react		= rv_reaction_sigbus,
> +};

Let's be consistent and call this reactor "signal", you can use SIGBUS only in
the description until/if we allow different signals.

What do you think?

Thanks,
Gabriele

Re: [PATCH] rv: Add signal reactor

[Nam Cao]

On Fri, Sep 19, 2025 at 02:26:12PM +0200, Gabriele Monaco wrote:
> On Fri, 2025-09-19 at 12:49 +0200, Thomas Weißschuh wrote:
> > Reactions of the existing reactors are not easily detectable from programs
> > and also not easily attributable to the triggering task.
> > This makes it hard to test the monitors themselves programmatically.
> > 
> > The signal reactors allows applications to validate the correct operations
> > of monitors either by installing custom signal handlers or by forking a
> > child and waiting for the expected exit code.
> 
> Thanks, this looks like a really nice addition!

Yeah, this will help us write KUnit or kselftest for the rtapp monitor.

> > For now only SIGBUS is supported, but additional signals can be added.
> 
> Curious choice of a default signal, is this specific to your use-case?

Any signal should do. Looking through the signal list, I think SIGTRAP is
more appropriate.

> > +config RV_REACT_SIGNAL
> > +	bool "Signal reactor"
> > +	depends on RV_REACTORS
> > +	default y
> > +	help
> > +	  Enables the signal reactor. The signal reactors sends a signal to
> > the
> > +	  task triggering an exception.
> 
> This assumption is not always correct, imagine a failure in the sleep monitor
> caused by the wakeup event. The offending task is not current but the wakee.
> 
> This is a bit tricky because reactors don't have access to that task, just to
> keep the same implementation between per-cpu and per-task monitors.

Yeah, this one is tricky. We probably need to pass the correct task_struct
to reactor, but then I'm not sure how to handle the other monitor types,
e.g. per-cpu monitors.

I have no alternative to offer, let me give it some thought.

> > +static void rv_signal_force_sig(int signal, const char *message)
> > +{
> > +	/* The message already contains a subsystem prefix, so use raw
> > printk() */
> > +	printk(KERN_WARNING "%s", message);
> > +	pr_warn("Killing PID %d with signal %d", task_pid_nr(current),
> > signal);

RV reactors have to use printk_deferred() instead. See:
https://lore.kernel.org/lkml/874k50hqmj.fsf@jogness.linutronix.de/

But I suggest dropping the printk from this reactor. We already have a
printk reactor for that.

> > +	force_sig(signal);
> > +}
> > +
> > +static void rv_signal_task_work(struct callback_head *cbh)
> > +{
> > +	struct rv_signal_work *work = container_of_const(cbh, struct
> > rv_signal_work, twork);
> > +
> > +	rv_signal_force_sig(work->signal, work->message);
> > +
> > +	mempool_free(work, rv_signal_task_work_pool);
> > +}
> > +
> > +static void rv_reaction_signal(int signal, const char *fmt, va_list args)
> > +{
> > +	struct rv_signal_work *work;
> > +	char message[256];
> > +
> > +	work = mempool_alloc_preallocated(rv_signal_task_work_pool);
> > +	if (!work) {
> > +		pr_warn_ratelimited("Unable to signal through task_work,
> > sending directly\n");
> > +		vsnprintf(message, sizeof(message), fmt, args);
> > +		rv_signal_force_sig(signal, message);
> > +		return;
> > +	}
> 
> Why do you use the task_work at all instead of signalling directly?
> If that's something not safe from a (any) tracepoint because it can sleep

If I remember correctly, sending signals requires a spinlock and therefore
may sleep on PREEMPT_RT.

> you should definitely not call it if allocation fails.

Yep.

We probably can get away with not reacting at all if allocation fails, by
crafting our tests such that only one reaction happens at a time, and
allocation won't fail.

Nam

Re: [PATCH] rv: Add signal reactor

[Gabriele Monaco]

On Mon, 2025-09-22 at 18:29 +0200, Nam Cao wrote:
> On Fri, Sep 19, 2025 at 02:26:12PM +0200, Gabriele Monaco wrote:
> > On Fri, 2025-09-19 at 12:49 +0200, Thomas Weißschuh wrote:
> > > +static void rv_reaction_signal(int signal, const char *fmt, va_list args)
> > > +{
> > > +	struct rv_signal_work *work;
> > > +	char message[256];
> > > +
> > > +	work = mempool_alloc_preallocated(rv_signal_task_work_pool);
> > > +	if (!work) {
> > > +		pr_warn_ratelimited("Unable to signal through task_work,
> > > sending directly\n");
> > > +		vsnprintf(message, sizeof(message), fmt, args);
> > > +		rv_signal_force_sig(signal, message);
> > > +		return;
> > > +	}
> > 
> > Why do you use the task_work at all instead of signalling directly?
> > If that's something not safe from a (any) tracepoint because it can sleep
> 
> If I remember correctly, sending signals requires a spinlock and therefore
> may sleep on PREEMPT_RT.

Yeah that's what I quickly glanced at. Which seems to be the case also for
mempool_alloc_preallocated by the way, so I'm not sure that's safer than
signalling directly on PREEMPT_RT.

Thomas, did you test your reactor on PREEMPT_RT? I'd expect a few fat warnings
when this is called from sched tracepoints. Unless you're lucky and never get
contention. Lockdep (CONFIG_PROVE_LOCKING) may help here.

Thanks,
Gabriele

> 
> > you should definitely not call it if allocation fails.
> 
> Yep.
> 
> We probably can get away with not reacting at all if allocation fails, by
> crafting our tests such that only one reaction happens at a time, and
> allocation won't fail.
> 
> Nam

Re: [PATCH] rv: Add signal reactor

[Thomas Weißschuh]

Hi Gabriele and Nam,

Sep 23, 2025 09:43:05 Gabriele Monaco <gmonaco@redhat.com>:

> On Mon, 2025-09-22 at 18:29 +0200, Nam Cao wrote:
>> On Fri, Sep 19, 2025 at 02:26:12PM +0200, Gabriele Monaco wrote:
>>> On Fri, 2025-09-19 at 12:49 +0200, Thomas Weißschuh wrote:
>>>> +static void rv_reaction_signal(int signal, const char *fmt, va_list args)
>>>> +{
>>>> +   struct rv_signal_work *work;
>>>> +   char message[256];
>>>> +
>>>> +   work = mempool_alloc_preallocated(rv_signal_task_work_pool);
>>>> +   if (!work) {
>>>> +       pr_warn_ratelimited("Unable to signal through task_work,
>>>> sending directly\n");
>>>> +       vsnprintf(message, sizeof(message), fmt, args);
>>>> +       rv_signal_force_sig(signal, message);
>>>> +       return;
>>>> +   }
>>>
>>> Why do you use the task_work at all instead of signalling directly?
>>> If that's something not safe from a (any) tracepoint because it can sleep
>>
>> If I remember correctly, sending signals requires a spinlock and therefore
>> may sleep on PREEMPT_RT.
>
> Yeah that's what I quickly glanced at. Which seems to be the case also for
> mempool_alloc_preallocated by the way, so I'm not sure that's safer than
> signalling directly on PREEMPT_RT.
>
> Thomas, did you test your reactor on PREEMPT_RT? I'd expect a few fat warnings
> when this is called from sched tracepoints. Unless you're lucky and never get
> contention. Lockdep (CONFIG_PROVE_LOCKING) may help here.

I trusted the documentation, which promised not to sleep.
I'll rework it for v2.

> Thanks,
> Gabriele
>
>>
>>> you should definitely not call it if allocation fails.
>>
>> Yep.

Ack.

>>
>> We probably can get away with not reacting at all if allocation fails, by
>> crafting our tests such that only one reaction happens at a time, and
>> allocation won't fail.

Ack.


I am wondering if it would make sense to add a new tracepoint that fires in addition of the reactors.
That would allow multiple simultaneous consumers and also bespoke handlers in userspace.


Thomas

Re: [PATCH] rv: Add signal reactor

[Thomas Weißschuh]

On Mon, Sep 22, 2025 at 06:29:00PM +0200, Nam Cao wrote:
> On Fri, Sep 19, 2025 at 02:26:12PM +0200, Gabriele Monaco wrote:
> > On Fri, 2025-09-19 at 12:49 +0200, Thomas Weißschuh wrote:

(...)

> > > For now only SIGBUS is supported, but additional signals can be added.
> > 
> > Curious choice of a default signal, is this specific to your use-case?
> 
> Any signal should do. Looking through the signal list, I think SIGTRAP is
> more appropriate.

Indeed. Thanks for the hint.

> > > +config RV_REACT_SIGNAL
> > > +	bool "Signal reactor"
> > > +	depends on RV_REACTORS
> > > +	default y
> > > +	help
> > > +	  Enables the signal reactor. The signal reactors sends a signal to
> > > the
> > > +	  task triggering an exception.
> > 
> > This assumption is not always correct, imagine a failure in the sleep monitor
> > caused by the wakeup event. The offending task is not current but the wakee.
> > 
> > This is a bit tricky because reactors don't have access to that task, just to
> > keep the same implementation between per-cpu and per-task monitors.
> 
> Yeah, this one is tricky. We probably need to pass the correct task_struct
> to reactor, but then I'm not sure how to handle the other monitor types,
> e.g. per-cpu monitors.
> 
> I have no alternative to offer, let me give it some thought.

Thanks.

> > > +static void rv_signal_force_sig(int signal, const char *message)
> > > +{
> > > +	/* The message already contains a subsystem prefix, so use raw
> > > printk() */
> > > +	printk(KERN_WARNING "%s", message);
> > > +	pr_warn("Killing PID %d with signal %d", task_pid_nr(current),
> > > signal);
> 
> RV reactors have to use printk_deferred() instead. See:
> https://lore.kernel.org/lkml/874k50hqmj.fsf@jogness.linutronix.de/

When the direct-call path is removed, this will only be used through task_work.
For that direct printk() should be fine, right?

> But I suggest dropping the printk from this reactor. We already have a
> printk reactor for that.

Works for me.

(...)

> > Why do you use the task_work at all instead of signalling directly?
> > If that's something not safe from a (any) tracepoint because it can sleep
> 
> If I remember correctly, sending signals requires a spinlock and therefore
> may sleep on PREEMPT_RT.
> 
> > you should definitely not call it if allocation fails.
> 
> Yep.
> 
> We probably can get away with not reacting at all if allocation fails, by
> crafting our tests such that only one reaction happens at a time, and
> allocation won't fail.

It would be nice if the reactor works without having to worry about its
implementation in the testcases or even general users. In 6.18 we will get
kmalloc_nolock() which is meant to be usable from tracepoint context. My
plan is to use that for the next revision.


Thomas

Re: [PATCH] rv: Add signal reactor

[Nam Cao]

Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:
> I am wondering if it would make sense to add a new tracepoint that
> fires in addition of the reactors. That would allow multiple
> simultaneous consumers and also bespoke handlers in userspace.

We do have tracepoints for each monitor in: kernel/trace/rv/rv_trace.h

And yeah, I think it is a nice idea for all the consumers to use these
tracepoints intead (that includes rtapp testing, and also the existing
reactors). It would simplify things, as the monitors do not have to
worry about the reactors, they only need to invoke tracepoints.

But this also makes me think about the necessity of the existing
reactors. What do they offer that tracepoints do not? Myself I almost
never use the reactors, so I'm thinking about removing them. But maybe
@Gabriele has objections?

Nam

Re: [PATCH] rv: Add signal reactor

[Gabriele Monaco]

2025-10-02T14:56:23Z Nam Cao <namcao@linutronix.de>:

> Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:
>> I am wondering if it would make sense to add a new tracepoint that
>> fires in addition of the reactors. That would allow multiple
>> simultaneous consumers and also bespoke handlers in userspace.
>
> We do have tracepoints for each monitor in: kernel/trace/rv/rv_trace.h
>
> And yeah, I think it is a nice idea for all the consumers to use these
> tracepoints intead (that includes rtapp testing, and also the existing
> reactors). It would simplify things, as the monitors do not have to
> worry about the reactors, they only need to invoke tracepoints.
>
> But this also makes me think about the necessity of the existing
> reactors. What do they offer that tracepoints do not? Myself I almost
> never use the reactors, so I'm thinking about removing them. But maybe
> @Gabriele has objections?

Well, many use cases might be better off with tracepoints, but reactors do
things tracepoints cannot really do.
Printk is much faster (and perhaps more reliable) than the trace buffer for
printing, panic can be used to gather a kernel dump.
One may just attach to tracepoints via libtracefs/BPF and do most of the things
you'd want to do with a new reactor, but I see reactors much easier to use from
scripts, for instance.

LTLs don't benefit as much as they don't print any additional information via
reactors, but DA/HA give hints on what's wrong.

I wouldn't get rid of reactors until they become a huge maintenance burden, but
probably we could think about it twice before extending or making them more
complex.

For instance, what's the exact use case of the signal reactor? Isn't it simpler
to do everything in BPF? Is the signal needed at all or something else (e.g.
perf) would do the job?

Thoughts?

Gabriele

Re: [PATCH] rv: Add signal reactor

[Thomas Weißschuh]

Hi Gabriele,

On Fri, Oct 03, 2025 at 08:30:10AM +0200, Gabriele Monaco wrote:
> 2025-10-02T14:56:23Z Nam Cao <namcao@linutronix.de>:
> 
> > Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes:
> >> I am wondering if it would make sense to add a new tracepoint that
> >> fires in addition of the reactors. That would allow multiple
> >> simultaneous consumers and also bespoke handlers in userspace.
> >
> > We do have tracepoints for each monitor in: kernel/trace/rv/rv_trace.h
> >
> > And yeah, I think it is a nice idea for all the consumers to use these
> > tracepoints intead (that includes rtapp testing, and also the existing
> > reactors). It would simplify things, as the monitors do not have to
> > worry about the reactors, they only need to invoke tracepoints.
> >
> > But this also makes me think about the necessity of the existing
> > reactors. What do they offer that tracepoints do not? Myself I almost
> > never use the reactors, so I'm thinking about removing them. But maybe
> > @Gabriele has objections?
> 
> Well, many use cases might be better off with tracepoints, but reactors do
> things tracepoints cannot really do.
> Printk is much faster (and perhaps more reliable) than the trace buffer for
> printing, panic can be used to gather a kernel dump.
> One may just attach to tracepoints via libtracefs/BPF and do most of the things
> you'd want to do with a new reactor, but I see reactors much easier to use from
> scripts, for instance.
>
> LTLs don't benefit as much as they don't print any additional information via
> reactors, but DA/HA give hints on what's wrong.
> 
> I wouldn't get rid of reactors until they become a huge maintenance burden, but
> probably we could think about it twice before extending or making them more
> complex.

The existing reactors could be implemented on top of the tracepoints.
This should make the RV core a bit simpler.

Note: The current interface between the RV core and the reactors is inflexible.
Passing only opaque varargs requires all reactors to format the string from
within the tracepoint handler, as they can not know how long the objects
referenced by the varargs are valid. Passing the actual objects would allow
for example the signal reactor to format its message as part of the task_work
handler instead of the signal handler and avoid the allocation of a fixed size
message buffer.

> For instance, what's the exact use case of the signal reactor? Isn't it simpler
> to do everything in BPF? Is the signal needed at all or something else (e.g.
> perf) would do the job?

The signal reactor is convenient to write automated tests. It can be used to
validate the monitors by triggering known-bad systemcalls from a test
executable and expect it to be killed with the reactor signal, see below for
an example.
On the other hand it can be used to validate that the kernel itself does not
regress with respect to RV-validated properties. For example the test program
can enable the rtapp monitor and run an example RT application using all kinds
of known-good IPC mechanisms without it being killed.


Thomas



Example selftest to make sure rtapp/sleep monitors detect the invalid
clock_nanosleep(CLOCK_REALTIME). I use this to also test my reactor:

#include "kselftest.h"
#include "test_utils.h" /* Provides

#include <sched.h>
#include <sys/wait.h>
#include <time.h>
#include <unistd.h>

static void test_nanosleep(void)
{
	struct timespec sleep_time;
	pid_t pid;
	int ret;

	pid = fork();
	switch (pid) {
	case -1:
		ksft_test_result_fail("%s: failed to fork: %s\n", __func__, strerror(errno));
		return;
	case 0:
		sleep_time.tv_sec = 2;
		sleep_time.tv_nsec = 0;

		ret = clock_nanosleep(CLOCK_REALTIME, 0, &sleep_time, NULL);
		_exit(ret != 0);
	default:
		waitpid(pid, &ret, 0);
		ksft_print_msg("ret 0x%x\n", ret);
	}

	ksft_test_result(ret == 0, "%s\n", __func__);
}

static void become_realtime(void)
{
	struct sched_param p = {
		.sched_priority = 1,
	};
	int ret;

	ret = sched_setscheduler(0, SCHED_FIFO, &p);
	if (ret == -1) {
		ksft_print_header();
		ksft_set_plan(1);
		ksft_test_result_fail("Failed to become realtime: %s\n", strerror(errno));
		ksft_finished();
	}
}

int main(void)
{
	int ret;

	become_realtime();

	ret = rtapp_enable();
	if (ret) {
		ksft_print_header();
		ksft_set_plan(1);
		ksft_test_result_fail("Failed to enable rtapp: %s\n", strerror(errno));
		ksft_finished();
	}

	ksft_print_header();
	ksft_set_plan(1);

	test_nanosleep();

	rtapp_disable();

	ksft_finished();
}

Re: [PATCH] rv: Add signal reactor

[Gabriele Monaco]

On Mon, 2025-10-06 at 12:10 +0200, Thomas Weißschuh wrote:
> Hi Gabriele,
> 
> > Well, many use cases might be better off with tracepoints, but reactors do
> > things tracepoints cannot really do.
> > Printk is much faster (and perhaps more reliable) than the trace buffer for
> > printing, panic can be used to gather a kernel dump.
> > One may just attach to tracepoints via libtracefs/BPF and do most of the
> > things you'd want to do with a new reactor, but I see reactors much easier
> > to use from scripts, for instance.
> > 
> > LTLs don't benefit as much as they don't print any additional information
> > via reactors, but DA/HA give hints on what's wrong.
> > 
> > I wouldn't get rid of reactors until they become a huge maintenance burden,
> > but probably we could think about it twice before extending or making them
> > more complex.
> 
> The existing reactors could be implemented on top of the tracepoints.
> This should make the RV core a bit simpler.
> 
> Note: The current interface between the RV core and the reactors is
> inflexible.
> Passing only opaque varargs requires all reactors to format the string from
> within the tracepoint handler, as they can not know how long the objects
> referenced by the varargs are valid. Passing the actual objects would allow
> for example the signal reactor to format its message as part of the task_work
> handler instead of the signal handler and avoid the allocation of a fixed size
> message buffer.

Yeah that's right current reactors don't really make sense for things that are
not printing. But as mentioned before, fitting this for all the different
monitors types (per-cpu/per-task or something more exotic) and model types (DA
or LTL) has its challenges.

I personally never really used the panic reactor to get a crash dump, but I'd
probably miss the printk one, since it's much faster than waiting for the
tracepoint stuff (at least when matching with other things on the ringbuffer).

As I get it, extending the reactors integration to support more things might not
be too useful, but I'm not too convinced on removing reactors for good.
I'm gonna give a little more thought on this one.

> > For instance, what's the exact use case of the signal reactor? Isn't it
> > simpler
> > to do everything in BPF? Is the signal needed at all or something else (e.g.
> > perf) would do the job?
> 
> The signal reactor is convenient to write automated tests. It can be used to
> validate the monitors by triggering known-bad systemcalls from a test
> executable and expect it to be killed with the reactor signal, see below for
> an example.
> On the other hand it can be used to validate that the kernel itself does not
> regress with respect to RV-validated properties. For example the test program
> can enable the rtapp monitor and run an example RT application using all kinds
> of known-good IPC mechanisms without it being killed.
> 

Yeah, what I meant is: having a signal isn't your goal. Easily understand if
there was a reaction is.
You could even restructure your test using tracepoints without any signal.

So if I get it correctly, you are both "voting" for removing reactors in favour
of tracepoint-only error reporting.
Am I getting this right?

Thanks,
Gabriele

Re: [PATCH] rv: Add signal reactor

[Thomas Weißschuh]

On Mon, Oct 06, 2025 at 05:19:24PM +0200, Gabriele Monaco wrote:
> On Mon, 2025-10-06 at 12:10 +0200, Thomas Weißschuh wrote:

(...)

> > > For instance, what's the exact use case of the signal reactor? Isn't it
> > > simpler
> > > to do everything in BPF? Is the signal needed at all or something else (e.g.
> > > perf) would do the job?
> > 
> > The signal reactor is convenient to write automated tests. It can be used to
> > validate the monitors by triggering known-bad systemcalls from a test
> > executable and expect it to be killed with the reactor signal, see below for
> > an example.
> > On the other hand it can be used to validate that the kernel itself does not
> > regress with respect to RV-validated properties. For example the test program
> > can enable the rtapp monitor and run an example RT application using all kinds
> > of known-good IPC mechanisms without it being killed.
> > 
> 
> Yeah, what I meant is: having a signal isn't your goal. Easily understand if
> there was a reaction is.
> You could even restructure your test using tracepoints without any signal.

Agreed.

> So if I get it correctly, you are both "voting" for removing reactors in favour
> of tracepoint-only error reporting.
> Am I getting this right?

No, it is a suggestion for a cleanup/optimization where reactors are not
directly called from the monitors but instead from a tracepoint which is
triggered from the monitors. It would decouple the monitor and reactor
subsystems.


Thomas

Re: [PATCH] rv: Add signal reactor

[Nam Cao]

Gabriele Monaco <gmonaco@redhat.com> writes:
> So if I get it correctly, you are both "voting" for removing reactors in favour
> of tracepoint-only error reporting.
> Am I getting this right?

No, I just do not use the reactors much myself and wouldn't care if they
get removed. But if they are useful for you, let's keep it.

But I do like Thomas's proposal to build the reactors on top of the
tracepoints, so that they are decoupled from the monitors.

Nam

Re: [PATCH] rv: Add signal reactor

[Gabriele Monaco]

On Fri, 2025-10-10 at 13:02 +0200, Thomas Weißschuh wrote:
> > So if I get it correctly, you are both "voting" for removing reactors in
> > favour
> > of tracepoint-only error reporting.
> > Am I getting this right?
> 
> No, it is a suggestion for a cleanup/optimization where reactors are not
> directly called from the monitors but instead from a tracepoint which is
> triggered from the monitors. It would decouple the monitor and reactor
> subsystems.

Right, now I understand this better. Interesting idea, we can give it a thought.

Thanks,
Gabriele