Re: [PATCH 1/2] kernel/cobalt: Address SIGSHADOW_ACTION_HARDEN and self-hardening race condition

On 25.05.26 20:19, Philippe Gerum wrote:
> Jan Kiszka <[email protected]> writes:
> 
>> On 18.05.26 11:10, Florian Bezdeka wrote:
>>> On Sat, 2026-05-16 at 17:48 +0200, Philippe Gerum wrote:
>>>> Jan Kiszka <[email protected]> writes:
>>>>
>>>>> From: Jan Kiszka <[email protected]>
>>>>>
>>>>> When a remote thread is suspended, xnthread_suspend checks if it is
>>>>> currently relaxed and then calls it into hardened state via SIGSHADOW.
>>>>> However, this signal may race with the thread entering primary mode on
>>>>> its own, e.g. via a syscall. In that case, the thread will be kicked out
>>>>> of its suspension state by the pending signal, and the migrate syscall
>>>>> it then executes will not perform the expected suspension anymore.
>>>>>
>>>>> Plug this race by introducing two thread state flags, one for pending
>>>>> XNSUSP and another one for XNHELD. After having performed the requested
>>>>> hardening on SIGSHADOW_ACTION_HARDEN, we are now testing for those
>>>>> pending suspension states and replay them via self-suspension as needed.
>>>>> If a resume took place in the meantime, the state flags were cleared
>>>>> again, and nothing will happen after the migration step.
>>>>>
>>>>> Reported-by: Richard Weinberger <[email protected]>
>>>>> Signed-off-by: Jan Kiszka <[email protected]>
>>>>> ---
>>>>>
>>>>> Didn't check yet if EVL solves this topic differently or is exposed to 
>>>>> similar race. But given that Richard found the pattern even in Xenomai 
>>>>> 2, it's likely worth to check EVL carefully.
>>>>>
>>>>
>>>> Unlike v3, v4 does not use signals for internal synchronization, so it
>>>> does not suffer that particular issue related to suspending a thread
>>>> (no SIGSHADOW in evl). However, this issue is only a symptom of a much
>>>> broader problem both v3 and v4 have when it comes to dealing with
>>>> signals pending for suspended threads, and your patch won't solve it.
>>>>
>>>> To illustrate this, let's say (A) and (B) are Cobalt threads running
>>>> oob, (X) may be any in-band context:
>>>>
>>>> CPU0                                          CPU1
>>>> ----                                          ----
>>>>
>>>> A: xnthread_signal(B, SIGxx)                  B: core_syscall()
>>>>    <schedule irq_work>                              core_suspend(B, XNPEND)
>>>> A: core_suspend(B, XNSUSP)                    B: <schedules out>
>>>>       core_add_state(B, T_SUSP)
>>>>    core_reschedule()
>>>>       core_resched_ipi(CPU1)                  
>>>> X: irq_work_handler()
>>>>       send_sig(B, SIGxx)
>>>>          handle_inband_event(B, INBAND_TASK_SIGNAL)
>>>>             core_resume(B, XNPEND)
>>>>             core_resume(B, XNSUSP)
>>>>             core_add_state(B, XNKICKED)
>>>>          core_reschedule()
>>>>             core_resched_ipi(CPU1)                    
>>>>                                               X: <ipi-handler>
>>>>                                                     core_reschedule()
>>>>                                               B: <resuming oob>
>>>>                                               B: XNKICKED: prepare_for_signal()
>>>>                                                     xnthread_relax()
>>>>                                               B: <running in-band>
>>>>                                               B: migrate_syscall()
>>>>                                                  nop (XNPNDSUSP was not set)
>>>>                                               B: <still running in-band>
>>>>
>>>> Remoteness of CPUs does not even matter, e.g. with prio(A) > prio(B),
>>>> we might observe:
>>>>
>>>> CPU0
>>>> ----
>>>>
>>>> B: core_syscall()
>>>>       core_suspend(B, XNPEND)
>>>> B: <schedules out>
>>>> X: send_sig(B, SIGxx)
>>>>       handle_inband_event(B, INBAND_TASK_SIGNAL)
>>>>          core_resume(B, XNPEND) /* B is now fully unblocked */
>>>>          core_add_state(B, XNKICKED)
>>>> <IRQ>: core_resume(A)
>>>> A: <resuming oob>
>>>> A: core_suspend(B, XNSUSP)
>>>>       unblocked + XNKICKED set -> abort: XNSUSP ignored
>>>> B: <resuming oob>
>>>> B: XNKICKED: prepare_for_signal()
>>>>       xnthread_relax()
>>>> B: <still running in-band>
>>>>
>>>> There is clearly no shortage of possible variations of the scenarios
>>>> above especially if we bring up more complex multi-CPU configurations,
>>>> all of them causing some sort of breakage. IOW, there is a general issue
>>>> with signals wrt forcible suspension, not just with SIGSHADOW-based
>>>> synchronization in xnthread_suspend() vs migrate() which in addition,
>>>> has its own set of flaws.
>>>>
>>>> A way to address this properly may require to break an ancient
>>>> invariant, which states that a thread with any blocking bit set must not
>>>> be linked to any runqueue. On the contrary, a way to make sure that a
>>>> suspend request remains pending across a stage migration in order to
>>>> deal with an in-band signal is precisely to allow that request to linger
>>>> as long as necessary in a thread state mask, only to be considered once
>>>> the 'kicked' flag is cleared, ignored and left pending
>>>> otherwise. Meanwhile, that thread should be allowed to run.
>>>>
>>>> I need to have a closer look, but at first glance it seems fairly
>>>> straightforward to implement this in v4 at least.
>>>>
>>>> -- 
>>>> Philippe.
>>>
>>> Uh... All of that sounds a bit scary...
>>>
>>> There was a reprocuder provided by Richard on the list as well, right?
>>> Please make sure that we integrate a test case at the same time as the
>>> fixes. Otherwise the testcase might never be added.
>>
>> Yes, I already asked for a reworked version (Richard and/or the AI).
>>
>> What may also be valuable here is derive a test case that plays with
>> regular signal injections like Philippe sketched above. To my
>> understanding, that would even be (structurally) shareable between
>> Xenomai 3 and 4.
>>
> 
> Agreed. A fix for this issue ([1] and subsequent) is now available for
> v4. As I mentioned, the basic idea behind it is to make the forcible
> 'suspend' request a lazy condition, which comes into force only when the
> 'kicked' condition is cleared, and the latter can only happen as a
> result of switching in-band. Forcing the target thread to switch back
> oob at the first opportunity afterwards causes the suspension to take
> effect eventually. Because this is a 'systemic' approach when it comes
> to the scheduling rules, there is no risk of partial fixing depending on
> particular code locations to enforce the logic or not, so we won't have
> to play whack-a-mole with this issue.
> 
> This change does not broadly affect the overall scheduling logic in the
> core but rather refines it, it is mainly composed of the following
> steps:
> 
> - suspending a kicked thread (XNSUSP|XNHELD for v3, EVL_T_SUSP for v4)
>   requires to link it to its runqueue, which enables lazy handling of
>   the suspend condition.
> 
> - the code releasing a thread must be made aware that a suspended thread
>   being released might be linked to the runqueue (if kicked only).
> 
> - the scheduler in-band tail code should allow a thread to resume
>   execution if a lazy-suspend condition is pending for it. This is
>   merely about filtering out such condition from a sanity
>   check/assertion in that code if any.
> 
> - on preemption by the rescheduling procedure (xnsched_run() /
>   evl_schedule()), a kicked thread must be left into the runqueue, even
>   if suspended.
> 
> - the in-band migration code should clear the 'kicked' marker bit on
>   entry (with all locking provisions in place until dovetail_leave_oob()
>   is called).
> 
> - the thread kicking service (xnthread_kick() / evl_kick_thread())
>   should stop lifting the forcible suspend conditions, obviously, only
>   unblocking threads which are in some synchronous wait state (XNPEND /
>   EVL_T_PEND). This way, a sigwake event won't inadvertently remove the
>   suspend bits like Richard noticed.
> 
> - A (Dovetail) RETUSER request should be used to force a thread to
>   switch oob, getting rid of the flaky SIGSHADOW scheme for
>   this. dovetail_request_ucall() enables a synchronous call from the
>   target thread back to the oob core, which is much better and saner.
> 
> This is still a sketchy and stodgy description, but I would not consider
> this fix as a major change when it comes to v4, and since it does not
> interfere with the locking model at all, I believe v3 could follow the
> same path to address this problem.
> 
> Finally, there is a now a fairly exhaustive test in libevl to exercise
> this fix which could be easily ported to v3, see [2]. It managed to
> trigger several tricky issues when debugging the fix, so this may help.
> 

Thanks for the pointers, I need to drill through them - unless $someone
has more time this week (I'm hanging out at Embedded Recipes).

Jan

> HTH,
> 
> [1]
> https://gitlab.com/Xenomai/xenomai4/linux-evl/-/commit/5e708006ebb97c7c23ed3ee3c6fbebac4a4fd99f
> 
> [2] https://gitlab.com/Xenomai/xenomai4/libevl/-/blob/next/tests/thread-suspend.c?ref_type=heads
> 

-- 
Siemens AG, Foundational Technologies
Linux Expert Center