posix: pthread: race condition between pthread_create() and pthread_join() #56163
Comments
|
On commit 336803e, I'm unable to reproduce this on west build -p always -b qemu_cortex_a53 -t run tests/posix/common -- \
-DCONFIG_TICKLESS_KERNEL=n -DCONFIG_NEWLIB_LIBC=yOddly, what I have found with for i in tests/posix/common/src/*.c; do
j=$(basename $i)
if [ "$j" = "main.c" -o $j = "pthread.c" ]; then
continue
fi
k=$i.ignore
mv $i $k
done
west build -p always -b qemu_cortex_r5 -t run tests/posix/common -- \
-DCONFIG_TICKLESS_KERNEL=n -DCONFIG_NEWLIB_LIBC=yI am able to reproduce this on That leads me to believe we are looking at possible stack corruption (stack overflow, ..). Another alternative is that there may be some uninitialized variable. |
|
I tried this [1], which sets stack sizes to a minimum size, but the problem persisted. That makes me think it's more of an uninitialized variable issue or maybe an alignment issue with the architecture. If it's an uninitialized variable, it could take some time to pinpoint, unfortunately. That's very odd that this issue only presents itself with @microbuilder, do you know if Cortex-R5 has any special requirements w.r.t. thread stack alignment? [1] git checkout .
west build -p always -b qemu_cortex_r5 tests/posix/common -- \
-DCONFIG_TICKLESS_KERNEL=n -DCONFIG_NEWLIB_LIBC=y
grep STACK build/zephyr/.config | \
grep -v "^#" | \
grep -v "=\(y\|=n\)$" | \
while read -r LINE; do \
VAL="$(echo "$LINE" | awk '{split($0,a,"="); print a[2];}')" \
if [ $((2*VAL)) -lt 4096 ]; then \
NEWVAL=8192 \
else \
NEWVAL=$((2*VAL)) \
fi \
CONFIG="$(echo "$LINE" | awk '{split($0,a,"="); print a[1];}')" \
echo "$CONFIG=$NEWVAL" \
done | \
tee -a tests/posix/common/prj.conf
west build -p always -b qemu_cortex_r5 -t run tests/posix/common -- \
-DCONFIG_TICKLESS_KERNEL=n -DCONFIG_NEWLIB_LIBC=y |
no, only in QEMU or renode platform |
if the pthread test is removed, all other will not fail, and per my testing it is only the last test case in the pthread which will cause problem for itself or for following test, a simple way to check is add a sleep right after the test. which will trigger the problem. |
|
@hakehuang @dleach02 - please take a look at #57403 - it seems the solution was a lot easier than we anticipated. |
cfriedt
added
backport v2.7-branch
|
Re-opening because it still is not fixed. It should not be necessary to re-initialize the same attribute over and over again as well. Just to be clear, we are not actually leaking descriptors, but we are seeing the effects of some kind of race condition between ZTEST(posix_apis, test_pthread_descriptor_leak)
{
pthread_t pthread1;
pthread_attr_t attr;
zassert_ok(pthread_attr_init(&attr));
zassert_ok(pthread_attr_setstack(&attr, &stack_e[0][0], STACKS));
/* If we are leaking descriptors, then this loop will never complete */
for (size_t i = 0; i < CONFIG_MAX_PTHREAD_COUNT * 100; ++i) {
zassert_ok(pthread_create(&pthread1, &attr, create_thread1, NULL),
"unable to create thread %zu", i);
zassert_ok(pthread_join(pthread1, NULL), "unable to join thread %zu", i);
}
}The same thing definitely does not occur with It will likely help to create a reliable stress test where we're not executing the entire |
cfriedt
added
priority: medium
|
Setting to priority medium as it affects multiple platforms. |
cfriedt
added
priority: high
|
On second thought - affects multiple platforms and is breaking CI - setting to high priority. |
|
#57637 can be used to more reliably reproduce the race condition. Still working on a fix. |
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
|
See commit message in linked PR. The root cause was a hand-rolled attempt at pthread_join() when it needed to be using the underlying k_thread_join() instead. |
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
The pthread_join() implementation was done using a condition variable and not k_thread_join(), and that's actually fatal. It's not possible in Zephyr to guarantee a thread has exited in a race-free way without calling either k_thread_abort() or k_thread_join(). This opened a hole where an app could call pthread_join(), which would go to sleep waiting on the condition variable, which would be signaled from the exit path of the thread, and then return to call pthread_create() (and thus k_thread_create()) on the same pthread_t object (and thus the same struct k_thread) BEFORE THE EARLIER THREAD HAD ACTUALLY EXITED. This makes the scheduler blow up, at least on SMP (I wasn't personally able to make it happen on uniprocessor configs, though the race is present always). Rework to call k_thread_join(). There's no other correct way to do this. Fixes zephyrproject-rtos#56163 Signed-off-by: Andy Ross <andyross@google.com>
|
As it turns out, there is also a race between |
|
Can you file that as a bug? There really shouldn't be as I'm reading the code. |
|
Comment left in #58116 for anyone interested. FWIW: it seems not to be a kernel race in thread lifecycle, more like a arm64-specific race in context handling (and maybe x86_64, but with 1000x lower frequency). FWIW: I'd treat this as two separate issues and merge the pthread fixes. If you need to tune the test[1] on arm64 for CI, that seems acceptable as long as we have a bug tracking the instability. [1] SMP tests are already retried in CI because of unavoidable timer skew problems with scheduling "CPUs" onto host threads that can be preempted for 10ms+ in busy CI runs. (Which isn't what we're seeing here -- no timer interaction in this test!). The frequency I'm seeing probably wouldn't even show up above the noise floor. |
cfriedt
added
priority: medium
cfriedt
changed the title
|
Just wanted to note here that although this issue has highlighted an underlying issue with Working on a fix for this which I hope will be accepted into v3.4.0 |
|
Probably should not have been closed |
Previous Title
posix_common derived test fails on some qemu platforms when the CONFIG_TICKLESS_KERNEL=n is added
Describe the bug
posix_common derived test fails on some qemu platforms when the CONFIG_TICKLESS_KERNEL=n is added
Please also mention any information which could help others to understand
the problem you're facing:
qemu_cortex_r5 and qemu_cortex_a53
see discussion test_posix_common: fix test_clock_gettime_rollover failure #55208
specific commit?
No.
To Reproduce
Steps to reproduce the behavior:
Expected behavior
PASS
Impact
some potential race condition in pthread handling
Logs and console output
Environment (please complete the following information):
The text was updated successfully, but these errors were encountered: