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: <20250930161252-ea116bda-293e-4f81-ab0f-075dd03dee52@linutronix.de>
Date: Tue, 30 Sep 2025 16:18:32 +0200
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: Nam Cao <namcao@...utronix.de>, Gabriele Monaco <gmonaco@...hat.com>
Cc: 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 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