michaelh/zephyr
1
0
Fork 0
Code Releases Activity

kernel/sched: Fix race with switch handle

Browse source 
The "null out the switch handle and put it back" code in the swap
implementation is a holdover from some defensive coding (not wanting
to break the case where we picked our current thread), but it hides a
subtle SMP race: when that field goes NULL, another CPU that may have
selected that thread (which is to say, our current thread) as its next
to run will be spinning on that to detect when the field goes
non-NULL.  So it will get the signal to move on when we revert the
value, when clearly we are still running on the stack!

In practice this was found on x86 which poisons the switch context
such that it crashes instantly.

Instead, be firm about state and always set the switch handle of a
currently running thread to NULL immediately before it starts running:
right before entering arch_switch() and symmetrically on the interrupt
exit path.

Fixes #28105

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
This commit is contained in:
Andy Ross 2021-02-05 08:15:02 -08:00 • committed by Anas Nashif
commit dd43221540
2 changed files with 10 additions and 20 deletions
Download patch file Download diff file Expand all files Collapse all files

23
kernel/include/kswap.h
View file

@ -72,25 +72,8 @@ static ALWAYS_INLINE unsigned int do_swap(unsigned int key,
k_spin_release(lock);
}
#ifdef CONFIG_SMP
/* Null out the switch handle, see wait_for_switch() above.
* Note that we set it back to a non-null value if we are not
* switching! The value itself doesn't matter, because by
* definition _current is running and has no saved state.
*/
volatile void **shp = (void *)&old_thread->switch_handle;
*shp = NULL;
#endif
new_thread = z_get_next_ready_thread();
#ifdef CONFIG_SMP
if (new_thread == old_thread) {
*shp = old_thread;
}
#endif
if (new_thread != old_thread) {
#ifdef CONFIG_TIMESLICING
z_reset_time_slice();
@ -111,8 +94,10 @@ static ALWAYS_INLINE unsigned int do_swap(unsigned int key,
arch_cohere_stacks(old_thread, NULL, new_thread);
_current_cpu->current = new_thread;
arch_switch(new_thread->switch_handle,
&old_thread->switch_handle);
void *newsh = new_thread->switch_handle;
new_thread->switch_handle = NULL;
arch_switch(newsh, &old_thread->switch_handle);
}
if (is_spinlock) {

7
kernel/sched.c
View file

@ -941,6 +941,8 @@ void *z_get_next_switch_handle(void *interrupted)
z_check_stack_sentinel();
#ifdef CONFIG_SMP
void *ret = NULL;
LOCKED(&sched_spinlock) {
struct k_thread *old_thread = _current, *new_thread;
@ -968,12 +970,15 @@ void *z_get_next_switch_handle(void *interrupted)
#endif
}
old_thread->switch_handle = interrupted;
ret = new_thread->switch_handle;
new_thread->switch_handle = NULL;
}
return ret;
#else
_current->switch_handle = interrupted;
set_current(z_get_next_ready_thread());
#endif
return _current->switch_handle;
#endif
}
#endif