Re: [PATCH 1/2] kernel/cobalt: Address SIGSHADOW_ACTION_HARDEN and self-hardening race condition
Philippe Gerum <[email protected]>
| Newsgroups | dev.linux.lists.xenomai |
|---|---|
| Message-ID | <[email protected]> |
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. 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 -- Philippe.