[Drbd-dev] [PATCH] drbd: fix a race of drbd_free_peer_req

[Drbd-dev] [PATCH] drbd: fix a race of drbd_free_peer_req

[Rui Xu]

Commit e061feb8 and 9a11ebeb5 introduce a bug to race with drbd_free_peer_req()
in got_peer_ack.

drbd_free_page_chain in drbd_finish_peer_reqs and got_peer_ack may happen
concurrently， the sequence is as follows:

drbd_finish_peer_reqs                 got_peer_ack
drbd_free_page_chain		      drbd_free_peer_req
spin_lock()
page_chain_add
spin_unlock()                         spin_lock()
				      page_chain_add
				      spin_unlock()
We can see that page_chain_add will be called twice which will lead
to crash.

Revert those two commits to ensure that drbd_free_page_chain will only
called once for one peer_req.

Signed-off-by: Rui Xu <rui.xu@easystack.cn>
---
 drbd/drbd_receiver.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drbd/drbd_receiver.c b/drbd/drbd_receiver.c
index f077f2b..8cac89b 100644
--- a/drbd/drbd_receiver.c
+++ b/drbd/drbd_receiver.c
@@ -653,10 +653,12 @@ static int drbd_finish_peer_reqs(struct drbd_connection *connection)
 
 		++n;
 		/* list_del not necessary, next/prev members not touched */
-		/* The callback may free peer_req. */
 		err2 = peer_req->w.cb(&peer_req->w, !!err);
 		if (!err)
 			err = err2;
+	
+		if (list_empty(&peer_req->recv_order)) {
+			drbd_free_peer_req(peer_req);
 	}
 	if (atomic_sub_and_test(n, &connection->done_ee_cnt))
 		wake_up(&connection->ee_wait);
@@ -2293,7 +2295,6 @@ static int e_end_resync_block(struct drbd_work *w, int unused)
 	}
 	dec_unacked(peer_device);
 
-	drbd_free_peer_req(peer_req);
 	return err;
 }
 
@@ -2562,7 +2563,6 @@ static int e_end_block(struct drbd_work *w, int cancel)
 
 	drbd_may_finish_epoch(peer_device->connection, peer_req->epoch, EV_PUT + (cancel ? EV_CLEANUP : 0));
 
-	drbd_free_page_chain(&peer_device->connection->transport, &peer_req->page_chain, 0);
 	return err;
 }
 
-- 
1.8.3.1

Re: [Drbd-dev] [PATCH] drbd: fix a race of drbd_free_peer_req

[Joel Colledge]

Hi Xu,

I think there is a potential race here, but I am not convinced that
this is a general solution. The peer request could still be freed by
got_peer_ack() between checking "list_empty(&peer_req->recv_order)"
and freeing it in drbd_finish_peer_reqs(). Also, this solution keeps
the page chain for peer requests for an unnecessarily long time, which
is not ideal in memory constrained situations.

The underlying race, as far as I understand it, is that got_peer_ack()
can be called while still processing the request in
drbd_finish_peer_reqs(). This is only relevant for peer writes and not
resync, so only the e_end_block() path is of interest. got_peer_ack()
will only be called after we have sent the corresponding barrier ack
for the peer request.

On the basis of this reasoning, I think a simple solution is to swap
drbd_may_finish_epoch() and drbd_free_page_chain() in e_end_block().
Please try this and send it as a patch if it solves your problem.

By the way, this patch doesn't compile due to a mismatched brace.

Best regards,
Joel

Re: [Drbd-dev] [PATCH] drbd: fix a race of drbd_free_peer_req

[rui.xu]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

Hi Joel,


It looks good to me and i have sent a new patch for it.



Thanks,
Xu







From: Joel Colledge <joel.colledge@linbit.com>
Date: 2022-04-20 18:22:32
To:  Rui Xu <rui.xu@easystack.cn>
Cc:  Philipp Reisner <philipp.reisner@linbit.com>,drbd-dev@lists.linbit.com,dongsheng.yang@easystack.cn
Subject: Re: [PATCH] drbd: fix a race of drbd_free_peer_req>Hi Xu,
>
>I think there is a potential race here, but I am not convinced that
>this is a general solution. The peer request could still be freed by
>got_peer_ack() between checking "list_empty(&peer_req->recv_order)"
>and freeing it in drbd_finish_peer_reqs(). Also, this solution keeps
>the page chain for peer requests for an unnecessarily long time, which
>is not ideal in memory constrained situations.
>
>The underlying race, as far as I understand it, is that got_peer_ack()
>can be called while still processing the request in
>drbd_finish_peer_reqs(). This is only relevant for peer writes and not
>resync, so only the e_end_block() path is of interest. got_peer_ack()
>will only be called after we have sent the corresponding barrier ack
>for the peer request.
>
>On the basis of this reasoning, I think a simple solution is to swap
>drbd_may_finish_epoch() and drbd_free_page_chain() in e_end_block().
>Please try this and send it as a patch if it solves your problem.
>
>By the way, this patch doesn't compile due to a mismatched brace.
>
>Best regards,
>Joel





[-- Attachment #2: Type: text/html, Size: 1773 bytes --]