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

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

[Rui Xu]

There is a race condition in update_sync_bits() and receive_bitmap(),
please consider this scenario:

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

(1) network failure happend on node-1.
(2) node-1 network recovery.
(3) node-1 connect to node-2, and start resync (node-1 is SyncTarget,
node-2 is SyncSource)
(4) before resync in (3) finished, node-1 connect to node-3 and start
resync.(node-1 is PauseSyncTarget, node-3 is PauseSyncSource)

When node-1(SyncTarget) is resync with node-2(SyncSource), node-1 may
set bitmap for node-3 in receive_resync_read()->drbd_set_all_out_of_sync(),
and clear the bimap for node-3 when got P_PEERS_IN_SYNC from node-2.

Then there is a possibility scenario as below:

thread:ack_receiver (node-1)           thread:receiver (node-1)
update_sync_bits()                     receive_bitmap()

set the rs_is_done to 1                set the bitmap for node-3
				       set the repl_state to PauseSyncTarget
set RS_DONE flag

it will lead the reysnc of node-1 and node-3 to finish in an unexpected way, so
we need to determine the is_sync_target_state before getting the bitmap total
weight in update_sync_bits.
---
 drbd/drbd_actlog.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drbd/drbd_actlog.c b/drbd/drbd_actlog.c
index 841e5149..3d2fd399 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,12 @@ static int update_sync_bits(struct drbd_peer_device *peer_device,
 	}
 	if (count) {
 		if (mode == SET_IN_SYNC) {
+			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

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

[Rui Xu]

There is a race condition in update_sync_bits() and receive_bitmap(),
please consider this scenario:

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

(1) network failure happend on node-1.
(2) node-1 network recovery.
(3) node-1 connect to node-2, and start resync (node-1 is SyncTarget,
node-2 is SyncSource)
(4) before resync in (3) finished, node-1 connect to node-3 and start
resync.(node-1 is PauseSyncTarget, node-3 is PauseSyncSource)

When node-1(SyncTarget) is resync with node-2(SyncSource), node-1 may
set bitmap for node-3 in receive_resync_read()->drbd_set_all_out_of_sync(),
and clear the bitmap for node-3 when got P_PEERS_IN_SYNC from node-2.

Then there is a possibility scenario as below:

thread:ack_receiver (node-1)           thread:receiver (node-1)
update_sync_bits()                     receive_bitmap()

set the rs_is_done to 1
				       set the bitmap for node-3
				       set the repl_state to PauseSyncTarget
set RS_DONE flag

it will lead the reysnc of node-1 and node-3 to finish in an unexpected way, so
we need to determine the is_sync_target_state before getting the bitmap total
weight in update_sync_bits.
---
changelog:
	-v1: fix typo in commit message
 drbd/drbd_actlog.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drbd/drbd_actlog.c b/drbd/drbd_actlog.c
index 841e5149..3d2fd399 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,12 @@ static int update_sync_bits(struct drbd_peer_device *peer_device,
 	}
 	if (count) {
 		if (mode == SET_IN_SYNC) {
+			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

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

[Joel Colledge]

Thanks for the contribution. The problem description and patch look valid to me.

Please add a brief comment to the code explaining why it is important
to evaluate is_sync_target_state at that point.

Here is a cleaned up version of your commit message. Is it still
correct? In particular, I am not sure what the consequences of the bug
are. "resync finishes in an unexpected way" is vague. I wrote "even
though bitmap bits are still set" but maybe there are other effects
that I have not thought of.


drbd: fix a race condition in update_sync_bits() and receive_bitmap()

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. 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@easystack.cn>
Signed-off-by: Joel Colledge <joel.colledge@linbit.com>

[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@easystack.cn>
Signed-off-by: Joel Colledge <joel.colledge@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

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

[Joel Colledge]

Great. Thanks! I'll look into getting this merged.

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

[Joel Colledge]

This was included in drbd-9.0 with:
https://github.com/LINBIT/drbd/commit/a9a54e48f5d6eb0b06d5832246ff7da2305fe5e1

And merged forward into master with:
https://github.com/LINBIT/drbd/commit/6cb88bf2af4a5628d09276e92f905405d01e0270