sched: take the lock before checking the wait queue in condvar wake_one/wake_all - #1480
Open
gburd wants to merge 1 commit into
Open
sched: take the lock before checking the wait queue in condvar wake_one/wake_all#1480gburd wants to merge 1 commit into
gburd wants to merge 1 commit into
Conversation
condvar::wake_one() and wake_all() early-returned on an unlocked read of _waiters_fifo.oldest before taking the internal mutex _m. A waiter links its wait_record into the FIFO under _m and then releases _m, but the waker did not take _m for that pre-check, so there is no acquire to pair with the waiter's release: the just-linked wait_record can be read as a stale nullptr and the wakeup silently dropped. The window is wider under CONF_fork, where a forked child's wait_record lives in another address space and the signaller may run in a different one. Take _m first, then read and act on _waiters_fifo, so the emptiness check is synchronized with the waiter's link. The prior unlocked read was only an optimization for the no-waiter case; correctness comes first. (wake_one already re-read the head under the lock, so only its early-return was unsound; wake_all had no post-lock recheck at all.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
condvar::wake_one()andcondvar::wake_all()begin with an early-return on anunlocked read of
_waiters_fifo.oldest, before taking the condvar'sinternal mutex
_m:A waiter (
condvar::wait) links itswait_recordinto_waiters_fifounder_m, then releases_m, then sleeps. The waker does not take_mfor thepre-check, so there is no acquire to pair with the waiter's release: the
just-linked
wait_recordcan be read as a stalenullptr, and the wakerreturns without waking anyone (a dropped wakeup).
wake_one()re-read the head under the lock afterwards, so only its unlockedearly-return was unsound;
wake_all()had no post-lock recheck at all once theearly-return was taken.
The window is wider under
CONF_fork: a forked child'swait_recordisallocated for cross-address-space coherence, and a broadcast may come from a
different address space than the waiter, with no
_msynchronizing the two.Fix
Take
_mfirst, then read and act on_waiters_fifo, so the emptiness check issynchronized with the waiter's link under the same lock. The prior unlocked
read was only an optimization for the no-waiter case.
Note
Found while investigating an I/O-completion hang; this is submitted on its own
as a self-contained correctness fix for the condvar primitive.