throttleRecord: take the record lock in delayFuncCallback before valuePut#27
Open
physwkim wants to merge 1 commit into
Open
throttleRecord: take the record lock in delayFuncCallback before valuePut#27physwkim wants to merge 1 commit into
physwkim wants to merge 1 commit into
Conversation
delayFuncCallback() runs on the general callback thread pool (armed via callbackRequestDelayed() from valuePut() and enterValue()) and called valuePut() with no dbScanLock. valuePut() mutates record fields (wait_flag, delay_flag, sts, sent, wait), fires the output link with dbPutLink(&prec->out), and runs recGblFwdLink(prec), recGblResetAlarms() and db_post_events() -- all of which EPICS requires the record lock to be held for. dbPutLink/recGblFwdLink additionally reach into another record's lockset, which is memory-unsafe when performed unlocked on SMP. valuePut() is reentrant against process(): a scan thread runs process() holding the record lock and reaches valuePut() through enterValue(), while the callback thread runs valuePut() with no lock at all. The two paths race on the same record. Observable failure: process()->enterValue() reads delay_flag while the callback's valuePut() is concurrently clearing it (delay_flag = 0); enterValue() sees the stale delay_flag == 1 and returns without writing, yet the callback has already passed its wait_flag test -- so the value is left in prec->val with wait_flag set, no OUT write, no FLNK, and no timer re-armed, and the throttle stalls until an unrelated later process. The torn writes to sts/sent are the same race's other face. Taking the record lock around valuePut() serializes the callback-thread mutation against the scan-thread process()/enterValue() path, closing the race. The omission is a slip, proven by this file against itself: the other callback, checkLinkCallback(), already wraps its body in dbScanLock()/ dbScanUnlock(). delayFuncCallback is armed only during record processing, never at init, so unlike checkLinkCallback it needs no interruptAccept guard -- only the lock, matching checkLinkCallback's else branch.
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
1 participant
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.
delayFuncCallback()runs on the general callback thread pool and callsvaluePut()with nodbScanLock.valuePut()mutates record fields and callsdbPutLink(&prec->out),recGblFwdLink(),recGblResetAlarms()anddb_post_events()— all of which require the record lock;dbPutLink/recGblFwdLinkalso reach into another record's lockset (memory-unsafe unlocked on SMP).It races
process()→enterValue()ondelay_flag: the scan thread can read the stale flag while the callback clears it, stranding a value inprec->valwith no OUT write, no FLNK and no timer re-arm — a stalled throttle — plus tornsts/sentwrites.The sibling callback
checkLinkCallback()in the same file already wraps its body indbScanLock/dbScanUnlock; this makesdelayFuncCallbackmatch it.delayFuncCallbackis armed only during processing (never at init), so unlikecheckLinkCallbackit needs nointerruptAcceptguard.