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] [day] [month] [year] [list]

Message-ID: <6a5fde33-b3e3-44e2-8ea5-5f4cf350cf35@linutronix.de>
Date: Thu, 25 Sep 2025 16:39:18 +0200 (GMT+02:00)
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: Gabriele Monaco <gmonaco@...hat.com>, Nam Cao <namcao@...utronix.de>
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

Hi Gabriele and Nam,

Sep 23, 2025 09:43:05 Gabriele Monaco <gmonaco@...hat.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