linux-kernel - Re: [PATCH] rv: Add signal reactor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250922162900.eNwI7CS0@linutronix.de>
Date: Mon, 22 Sep 2025 18:29:00 +0200
From: Nam Cao <namcao@...utronix.de>
To: Gabriele Monaco <gmonaco@...hat.com>
Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
	Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH] rv: Add signal reactor

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