Fix data race in MemTable::GetBloomFilter() pointer publication #423
Conversation
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
The bloom_filter_ptr_ was published using memory_order_relaxed, which does not establish a proper happens-before relationship between the pointer store and the DynamicBloom object initialization. This can lead to readers observing a partially constructed DynamicBloom on weak memory models (e.g., ARM), causing crashes when accessing uninitialized data. The issue was verified using Relacy formal verification tool, which confirmed the data race exists when using relaxed memory ordering. Fix: - Change load from memory_order_relaxed to memory_order_acquire - Change store from memory_order_relaxed to memory_order_release This establishes a proper release-acquire synchronization, ensuring that readers only observe a fully constructed DynamicBloom object after the pointer is published. Fixes: facebook#305 (introduced in commit 405de0e)
ti-chi-bot
bot
added
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Author
|
close because this is duplicated to #424. |
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.
Close tikv/tikv#19106
Fix data race in MemTable::GetBloomFilter() on ARM weak memory model introduced in #305 (commit 405de0e)
Use release-acquire memory ordering instead of relaxed to ensure
DynamicBloom object is fully initialized before pointer publication.
Problem:
to be published before DynamicBloom::data_ is initialized
causing crashes
Solution:
only see fully initialized objects
Performance impact:
~1-3 cycles on ARM (may add memory barrier)
filter operations (hash computation + memory access)
The issue was verified using Relacy formal verification tool, which confirmed the data race exists when using relaxed memory ordering.