arch: POSIX: Fix race with unused threads #17614
Conversation
aescolar
added
bug
aescolar
changed the title
Fix a race which seems to have been presenting itself very sporadically on loaded systems. The race seems to have caused tests/kernel/sched/schedule_api to fail at random on native_posix. The case is a bit convoluted: When the kernel calls z_new_thread(), the POSIX arch saves the new thread entry call in that new Zephyr thread stack together with a bit of extra info for the POSIX arch. And spawns a new pthread (posix_thread_starter()) which will eventually (after the Zephyr kernel swapped to it), call that entry function. (Note that in principle a thread spawned by pthreads may be arbitrarily delayed) The POSIX arch does not try to synchronize to that new pthread (because why should it) until the first time the Zephyr kernel tries to swap to that thread. But, the kernel may never try to swap to it. And therefore that thread's posix_thread_starter() may never have got to run before the thread was aborted, and its Zephyr stack reused for something else by the Zephyr app. As posix_thread_starter() was relaying on looking into that thread stack, it may now be looking into another thread stack or anything else. So, this commit fixes it by having posix_thread_starter() get the input it always needs not from the Zephyr stack, but from its own pthread_create() parameter pointing to a structure kept by the POSIX arch. Note that if the thread was aborted before reaching that point posix_thread_starter() will NOT call the Zephyr thread entry function, but just cleanup. With this change all "asynchronous" parts of the POSIX arch should relay only on the POSIX arch own structures. Signed-off-by: Alberto Escolar Piedras <[email protected]>
There was a problem hiding this comment.
Nice catch. This is one of those spurious failures that @marc-hb likes to worry about. As schedule_api is one of the tests that tends to fail spuriously on qemu, santicheck will retry failures and suppress this kind of thing.
|
@andyross @marc-hb : It took me a while to decide myself to dig into it. Because it was failing on all platforms on CI I was just assuming there must have been something rotten with that test (apart from its dependency on the qemu host timing). Neither valgrind, nor the address sanitizer were able to pinpoint the problem, and a quick debug did not reveal anything. Only a couple of days ago I got annoyed enough with it to start digging deeper. |
Actually, PR #17107 is all about [q]emu and this is specific to native_posix, correct? So they're not related. I haven't been testing native_posix much because it uses the random toolchain of the host by design which makes it a non-starter from a reproducible builds perspective. This seems fine: I think very few people if any are interested in signing native_posix builds or security around them in general, it's not in their purpose. Off-topic; zephyrproject-rtos/sdk-ng#81 is one better place. |
Fix a race which seems to have been presenting itself
very sporadically on loaded systems.
The race seems to have caused tests/kernel/sched/schedule_api
to fail at random on native_posix.
The case is a bit convoluted:
When the kernel calls z_new_thread(), the POSIX arch saves
the new thread entry call in that new Zephyr thread stack
together with a bit of extra info for the POSIX arch.
And spawns a new pthread (posix_thread_starter()) which
will eventually (after the Zephyr kernel swapped to it),
call that entry function.
(Note that in principle a thread spawned by pthreads may
be arbitrarily delayed)
The POSIX arch does not try to synchronize to that new
pthread (because why should it) until the first time the
Zephyr kernel tries to swap to that thread.
But, the kernel may never try to swap to it.
And therefore that threat's posix_thread_starter() may never
have got to run before the thread was aborted, and its
Zephyr stack reused for something else by the Zephyr app.
As posix_thread_starter() was relaying on looking into that
thread stack, it may now be looking into another thread stack
or anything else.
So, this commit fixes it by having posix_thread_starter()
get the input it always needs not from the Zephyr stack,
but from its own pthread_create() parameter pointing to a
structure kept by the POSIX arch.
Note that if the thread was aborted before reaching that point
posix_thread_starter() will NOT call the Zephyr thread entry
function, but just cleanup.
With this change all "asynchronous" parts of the POSIX arch
should relay only on the POSIX arch own structures.
Signed-off-by: Alberto Escolar Piedras [email protected]
Fixes #17613