Fix some race conditions in `BanMan::DumpBanlist()` #24168

hebasto · 2022-01-26T15:32:35Z

This PR split from #24097 with some additions. This makes the following switch from RecursiveMutex to Mutex a pure refactoring.

See details in commit messages.

maflcko · 2022-01-26T16:52:39Z

src/banman.cpp

 {
+    static Mutex dump_mutex;
+    TRY_LOCK(dump_mutex, dump_lock);
+    if (!dump_lock) return;


why is is ok to refuse to dump when two threads call this function?

It is an optimization. Calling this function in the destructor guarantees that the final state will be dumped to the disk.

Then it should be mentioned in the release notes that calling the ban rpc no longer implies the stuff is written to disk and when the node crashes, the state will be lost.

Not relevant since the recent update, I guess.

DrahtBot · 2022-01-26T23:17:17Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24097 (Replace RecursiveMutex m_cs_banned with Mutex, and rename it by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

w0xlt

tACK e06d092

This PR :

. adds m_banned_mutex to prevent the m_is_dirty value from being read in a different state in BannedSetIsDirty() and SweepBanned().

. gets rid of GetBanned() so it doesn't call SweepBanned() twice and sets banmap = m_banned. This works because the m_banned_mutex is already held in this operation.

. adds the same strategy used in DumpMempool to avoid concurrent writing to disk, but adds an optimization to return early if a write is in progress.

. Since there is only m_is_dirty = true; (not = false) outside of BanMan::DumpBanlist(), the last commit makes sense to avoid inconsistent values for m_dirty in a race condition.

shaavan

Concept ACK

Going commit wise:

Concurrent calls to Write() have been prevented by "Trying" to lock dump_lock and exit if locking fails. However, as @MarcoFalke pointed out, it can lead to ban rpc calls in which DumpBanList() could not write anything to disk, leading to potential data loss. This behavior should be appropriately documented.
Other than this, it could also lead to writing failure when called through other functions as well. What do you think about introducing a mutex for this function that allows calling DumpBanList only when we AssertLockNotHeld(new_mutex).
SweepBanned and BannedSetIsDirty are brought under the same lock block to prevent race conditions for m_is_dirty. I agree with this approach.
Prevented SweepBanned to be called twice by directly setting the value of banmap (= m_banned) instead of calling GetBanned(). I think this is an improvement over the current code.
Modifying the code to call SetBannedSetDirty(true); throughout the codebase prevents the risk of inconsistent value due to race conditions. I agree with this change.

w0xlt · 2022-01-28T13:29:33Z

Perhaps the same strategy used in DumpMempool would be a safer approach in 2c77427 .

static Mutex dump_mutex;
LOCK(dump_mutex);

The m_is_dirty value being read in BannedSetIsDirty() can differ from the value being set in SweepBanned(), i.e., be inconsistent with a BanMan instance internal state.

Another thread can `SetBannedSetDirty(true)` while `CBanDB::Write()` call being executed. The following `SetBannedSetDirty(false)` effectively makes `m_is_dirty` flag value inconsistent with the actual `m_banned` state. Such behavior can result in data loss, e.g., during shutdown.

hebasto · 2022-01-28T19:40:17Z

Updated e06d092 -> 99a6b69 (pr24168.01 -> pr24168.02, diff).

Lock optimization dropped as an any optimization must be justified with benchmarking, at least. That was not done in this case.
So sorry for a premature optimization.

w0xlt

reACK 99a6b69

Optimization removed, but the dump behavior now is more predictable.

jonatack · 2022-01-28T20:54:36Z

Concept ACK

shaavan

ACK 99a6b69

Changes since my last review:

Changed logic of preventing concurrent write by creating a static mutex and locking it.

@hebasto
We can move on to the optimization logic after we have some benchmark results. Until then, we can stick with the current logic, which is more reliable and predictable.

luke-jr

utACK

maflcko · 2022-01-31T08:37:22Z

src/banman.cpp


+    int64_t n_start = GetTimeMillis();
+    if (!m_ban_db.Write(banmap)) {
+        SetBannedSetDirty(true);


unrelated: Is m_dirty even used on shutdown when true? Shouldn't the below log be adjusted when it fails?

99a6b69 Fix race condition for SetBannedSetDirty() calls (Hennadii Stepanov) 83c7646 Avoid calling BanMan::SweepBanned() twice in a row (Hennadii Stepanov) 33bda6a Fix data race condition in BanMan::DumpBanlist() (Hennadii Stepanov) 5e20e9e Prevent possible concurrent CBanDB::Write() calls (Hennadii Stepanov) Pull request description: This PR split from bitcoin#24097 with some additions. This makes the following switch from `RecursiveMutex` to `Mutex` a pure refactoring. See details in commit messages. ACKs for top commit: w0xlt: reACK 99a6b69 shaavan: ACK 99a6b69 Tree-SHA512: da4e7268c7bd3424491f446145f18af4ccfc804023d0a7fe70e1462baab550a5e44f9159f8b9f9c7820d2c6cb6447b63883616199e4d9d439ab9ab1b67c7201b

hebasto mentioned this pull request Jan 26, 2022

Replace RecursiveMutex m_cs_banned with Mutex, and rename it #24097

Merged

maflcko reviewed Jan 26, 2022

View reviewed changes

w0xlt approved these changes Jan 28, 2022

View reviewed changes

shaavan reviewed Jan 28, 2022

View reviewed changes

hebasto added 4 commits January 28, 2022 19:26

Prevent possible concurrent CBanDB::Write() calls

5e20e9e

Fix data race condition in BanMan::DumpBanlist()

33bda6a

The m_is_dirty value being read in BannedSetIsDirty() can differ from the value being set in SweepBanned(), i.e., be inconsistent with a BanMan instance internal state.

Avoid calling BanMan::SweepBanned() twice in a row

83c7646

hebasto force-pushed the 220126-dump branch from e06d092 to 99a6b69 Compare January 28, 2022 19:30

w0xlt approved these changes Jan 28, 2022

View reviewed changes

shaavan approved these changes Jan 30, 2022

View reviewed changes

luke-jr approved these changes Jan 31, 2022

View reviewed changes

maflcko merged commit eacc0e8 into bitcoin:master Jan 31, 2022

maflcko reviewed Jan 31, 2022

View reviewed changes

hebasto deleted the 220126-dump branch January 31, 2022 08:49

bitcoin locked and limited conversation to collaborators Jan 31, 2023

Fix some race conditions in BanMan::DumpBanlist() #24168

Fix some race conditions in BanMan::DumpBanlist() #24168

Uh oh!

Conversation

hebasto commented Jan 26, 2022

Uh oh!

maflcko Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 26, 2022

Conflicts

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 28, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jan 28, 2022

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Fix some race conditions in `BanMan::DumpBanlist()` #24168

Fix some race conditions in `BanMan::DumpBanlist()` #24168

hebasto Jan 26, 2022 •

edited

Loading

w0xlt commented Jan 28, 2022 •

edited

Loading