[Drbd-dev] [PATCH v3] drbd: fix a race condition in update_sync_bits() and receive_bitmap()

[Rui Xu]

There was a race condition involving update_sync_bits() and
receive_bitmap(). Consider this scenario:

Primary: node-3, Secondary: node-1, node-2

(1) Network failure occurs on node-1
(2) node-1 network recovers
(3) node-1 connects to node-2, and starts resync (node-1 is SyncTarget,
node-2 is SyncSource)
(4) Before resync in (3) finishes, node-1 connects to node-3 and starts
resync (node-1 is PausedSyncT, node-3 is PausedSyncS)

The following sequence can occur on node-1 while it is syncing from
node-2:

* ack_receiver thread processes P_PEERS_IN_SYNC
* ack_receiver: call update_sync_bits()
* ack_receiver: clear the last bitmap bits for node-3
* ack_receiver: set rs_is_done to 1
* receiver thread processes P_*BITMAP
* receiver: call receive_bitmap()
* receiver: set bitmap bits for node-3
* receiver: set the repl_state towards node-3 to PausedSyncT
* ack_receiver: set RS_DONE flag

This causes the resync from node-3 to node-1 to finish even though
bitmap bits are still set, and node-1 will finish the resync as
PausedSyncT, but the resync from node-2 to node-1 is still ongoing,
at last, it will finish as a unstable resync, and leaving the disk
state end into INCONSISTENT finally.

Fix this by evaluating is_sync_target_state before getting the bitmap
total weight in update_sync_bits.

Signed-off-by: Rui Xu <rui.xu at easystack.cn>
Signed-off-by: Joel Colledge <joel.colledge at linbit.com>
---
changelog:
	-From v1: fix typo in commit message
	-From v2: add a brief comment to the code
	and added the bug description in commit message

 drbd/drbd_actlog.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drbd/drbd_actlog.c b/drbd/drbd_actlog.c
index 841e5149..01e64823 100644
--- a/drbd/drbd_actlog.c
+++ b/drbd/drbd_actlog.c
@@ -1044,11 +1044,11 @@ static bool lazy_bitmap_update_due(struct drbd_peer_device *peer_device)
 }
 
 static void maybe_schedule_on_disk_bitmap_update(struct drbd_peer_device *peer_device,
-						 bool rs_done)
+						 bool rs_done, bool is_sync_target)
 {
 	if (rs_done) {
 		if (peer_device->connection->agreed_pro_version <= 95 ||
-		    is_sync_target_state(peer_device, NOW))
+		    is_sync_target)
 			set_bit(RS_DONE, &peer_device->flags);
 
 		/* If sync source: rather wait for explicit notification via
@@ -1105,11 +1105,18 @@ static int update_sync_bits(struct drbd_peer_device *peer_device,
 	}
 	if (count) {
 		if (mode == SET_IN_SYNC) {
+			/* Evaluate is_sync_target_state before getting the bm total
+			 * weight to avoid a scenario: still_to_go is 0 when sync
+			 * state is not sync_target_state, but sync state change to
+			 * sync_target_state later and finish the resync even though
+			 * bitmap bits are still set.
+			 */
+			bool is_sync_target = is_sync_target_state(peer_device, NOW);
 			unsigned long still_to_go = drbd_bm_total_weight(peer_device);
 			bool rs_is_done = (still_to_go <= peer_device->rs_failed);
 			drbd_advance_rs_marks(peer_device, still_to_go);
 			if (cleared || rs_is_done)
-				maybe_schedule_on_disk_bitmap_update(peer_device, rs_is_done);
+				maybe_schedule_on_disk_bitmap_update(peer_device, rs_is_done, is_sync_target);
 		} else if (mode == RECORD_RS_FAILED) {
 			peer_device->rs_failed += count;
 		} else /* if (mode == SET_OUT_OF_SYNC) */ {
-- 
2.25.1