hal/{aarch64,arm}: fix possible race condition in hal_cpuReschedule

[jmaksymowicz]

Fix a possible race condition between hal_cpuReschedule and proc_reap.

Description

A race condition has been identified in hal_cpuReschedule implementation on aarch64 and armv7a that can manifest itself at the end of the exit syscall.

The unwanted interleave starts when hal_cpuReschedule, is called at the end of handling the exit syscall on core A. Within the function the spinlock is cleared, then on core B the thread running the proc_reap function is woken up. proc_reap then proceeds to free the resources belonging to the exited thread, including unmapping its kernel stack. However, the kernel stack is still in use by core A. This results in either:

1. An infinite loop of exception handling: the first step of exception handling involves pushing registers onto the stack - the stack is inaccessible due to being unmapped - this causes another exception in an infinite loop.
2. If another thread's kernel stack is mapped into the same memory various other errors may occur - e.g. core A handling the exception from step 1 may corrupt the stack contents of another kernel thread or core A may try to restore from an invalid stored state.

The issue is most pronounced on aarch64a53-zynqmp-qemu due to the fact that it uses spinlocks with wfe/ sev and emulation can execute large chunks of code from one CPU before switching to another.

On armv7a-zynq7000-qemu this may not occur due to the fact that emulation is done on many cores in parallel and spinlocks don't use wfe / sev.

On real hardware this may not occur very frequently due to code being executed in parallel, so the first CPU always "wins the race".

The fact that exit is called frequently within the test suite increases the likelihood of hitting this unwanted interleave - which is why the issue occurs frequently in testing, but seemingly not in practical applications on that platform.

The fix from armv7a was also applied to armv7r and armv8r for consistency - even though these platforms don't work in SMP so this race condition will never occur.

On ia32 and riscv64 platforms the implementation of hal_cpuReschedule is different and not susceptible to this error.

Motivation and Context

May fix phoenix-rtos/phoenix-rtos-project#1386 and phoenix-rtos/phoenix-rtos-project#1387 - needs more testing

YT: RTOS-1392

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

• Already covered by automatic testing.
• New test added: (add PR link here).
• Tested by hand on: aarch64a53-zynqmp-qemu, armv7a9-zynq7000-qemu

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing linter checks and tests passed.
• My changes generate no new compilation warnings for any of the targets.

Special treatment

• This PR needs additional PRs to work (list the PRs, preferably in merge-order).
• I will merge this PR by myself when appropriate.

[gemini-code-assist]

Code Review

This pull request updates the context switching and rescheduling logic across multiple ARM architectures (aarch64, armv7a, armv7r, and armv8r). It defers clearing the spinlock until after the new CPU context has been loaded from the stack, preventing potential race conditions. When a spinlock is provided, the code now calls the non-locked _threads_schedule function, loads the new context pointer, and then releases the spinlock. There are no review comments to address, so no additional feedback is provided.

[github-actions]

Unit Test Results

11 307 tests  ±0   10 600 ✅ +1   54m 3s ⏱️ + 1m 36s
   690 suites ±0      707 💤 ±0 
     1 files   ±0        0 ❌  - 1 

Results for commit 39de3d4. ± Comparison against base commit fb914a8.

♻️ This comment has been updated with latest results.

[lukileczo]

Are you sure this doesn't also affect other targets? From what I can see the implementations are pretty similar, e.g. on riscv64 I also clear the spinlock before loading the stack pointer

phoenix-rtos-kernel/hal/riscv64/_interrupts.S

Lines 471 to 494 in 0d8d283

	/* Clear spinlock */
	addi s0, s0, 24
	fence
	amoswap.w.rl zero, zero, (s0)
	tail _interrupts_return
	.LnoSpinlock:
	mv a0, zero
	mv a1, sp
	mv a2, zero
	call threads_schedule
	tail _interrupts_return
	.size hal_cpuReschedule, .-hal_cpuReschedule
	.type _interrupts_return, @function
	_interrupts_return:
	/* Switch stack */
	ld sp, 232(sp)
	MULTILOCK_CLEAR
	.Lmultilock_cleared:

which you deemed the cause of the issue.

[jmaksymowicz]

@lukileczo

Are you sure this doesn't also affect other targets? From what I can see the implementations are pretty similar, e.g. on riscv64 I also clear the spinlock before loading the stack pointer
...
which you deemed the cause of the issue.

Yes, it seems that riscv may also be affected, but to a much lesser extent. After clearing the spinlock there's only one jump instruction and then the new stack pointer is loaded from our current "doomed" stack, so there's much less room for the unwanted interleave described above. Considering that the scheduler is locked until after loading the new SP this would only happen in the rare case that the reaper thread is already running (so doesn't need to be scheduled in) and would need to work very fast to unmap the "doomed" stack before we manage to load from it.

On aarch64 and armv7a the spinlock was cleared a lot earlier, even before calling threads_schedule and taking the scheduler lock, leaving a lot of time and possibility for error.

[agkaminski]

That must've been quite a pain to find, good job. One nitpick about assumptions and comments.

[agkaminski]

This assumption is well based - it is there by design. What should be made is to make it clear and explicit to avoid mistakes. Perhaps skip passing spinlock as an argument and pass flag clear/no clear instead?

[agkaminski]

For now I think the comment about what spinlock is passed to the hal_cpuReschedule may be duplicated in other archs to avoid confusion why we execute _threads_schedule with some spinlock, when in fact we always do with threads_common.spinlock

[jmaksymowicz]

I decided to add a comment to hal/cpu.h explaining how the function should be used - I think that's a good solution for now. Ofc there's possibility of user error but at least the requirement is stated explicitly.

[etiaro]

When we enter hal_cpuReschedule without threads_common.spinlock, after returning from threads_schedule() call the stack we use theoretically could be already freed, similarly to the described riscv64 situation.

This is probably really unlikely, especially since there is few instructions between spinlock clear here, but it could be possible when we consider that threads_schedule can add current thread to threads_common.ghosts (and it really does happen e.g. in process_exception() code path).

We probably could get an interrupt after clearing threads_common spinlock in threads_schedule(?), which would give enough time for the reaper to destroy our stack? (I guess in riscv64 case interrupts are disabled)

Correct me if i'm wrong, this is purely theoretical (meaning: I did not reproduce that)

[jmaksymowicz]

I think you're right, in process_exception we need to call hal_cpuReschedule under spinlock - or just call proc_threadEnd instead, as it will handle things properly.

We probably could get an interrupt after clearing threads_common spinlock in threads_schedule(?), which would give enough time for the reaper to destroy our stack? (I guess in riscv64 case interrupts are disabled)

In all implementations of hal_cpuReschedule threads_schedule is called with interrupts disabled and they aren't enabled again until context is restored.

I will open a separate issue for fixing this and the riscv64 potential race conditions.

[ziemleszcz]

LGTM. Consider updating the comment as @agkaminski suggested.
Also update the PR description to mention that other architectures could theoretically be affected, although the race window so narrow that it's unlikely to occur in practice.
It might also be worth mentioning the similar narrow race windows in interrupt and exception handling that @etiaro pointed out.