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

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.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center