possible SMP race with k_thread_join()

[andrewboie]

Describe the bug
From code inspection I think I have found a potential race condition in k_thread_join().

The specific scenario:

1. Thread A is running
2. Thread B blocks with k_thread_join(A, K_FOREVER);
3. Thread A self-exits via k_thread_abort(k_current_get()) either via explicit call or by returning from its entry function
4. In the code path for k_thread_abort(), we enter z_thread_single_abort(). While holding the sched_spinlock, we wake up waiters, in this case thread B. The sched_spinlock is released.
5. Two things then happen simultaneously:

• On the current CPU, thread A finishes its aborting process. We return from z_thread_single_abort() and then call z_swap_unlocked() to finally context switch out. This happens quickly but isn't instantaneous.
• On another CPU, as soon as sched_spinlock was released in step 4, thread B wakes up. Since thread A has joined, thread B tries to launch a new thread re-using thread A's k_thread object.

The race is step 5. If thread B manages to instantiate the new thread using thread A's object, before thread A finishes its aborting process, we will end up with the same thread object (and the stack too if it was re-used) being active on two different CPUs causing data corruption.

This only affects SMP. In UP, if Thread A gets preempted after returning from z_thread_single_abort() it is never scheduled again and there is no race. This issue requires physical concurrency AFAICT.

To Reproduce
I don't have test code to demonstrate this reliably but it would explain some very intermittent crashes I see in tests that re-use thread objects after a join where both CPUs are crashing with the same thread object as current.

Expected behavior
We need to close this race somehow. Thread B needs to not wake up until thread A's k_thread and thread stack objects are truly unused.

It would of course be simplest to somehow have thread A keep holding sched_spinlock until it completely finishes aborting, but this is not feasible.

A promising alternative is to somehow bounce context off of Thread A to complete the aborting process; we could enter a special exception state (sort of like what irq_offload() does although this API was never intended for anything but test usage), or wake up and raise the priority of the idle thread and have the idle thread perform the logic in z_thread_single_abort().

I like the idea of using the idle thread since we already have one per CPU and we could avoid having thread abort tasks being queued up.

In general, we never want z_thread_single_abort() to run in the context of the thread being aborted.

Impact
This is concerning, but to be clear this only affects SMP and it requires a race to re-use a thread object, something that I expect happens more often in tests than in a real application.

[andrewboie]

Fixing this is I think a pre-condition to getting #23062 and #23063 working.

[andrewboie]

This will also help with code that wants to use the thread->fn_abort to free thread object or stack memory back to a heap, this can't be done if fn_abort function runs in the context of the thread being aborted. See #23062

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.