[v2,1/1] net:rds: Fix possible deadlock in rds_message_put

Commit Message

Allison Henderson Jan. 31, 2024, 5:54 p.m. UTC

From: Allison Henderson <[e-mail]>

Functions rds_still_queued and rds_clear_recv_queue lock a given socket
in order to safely iterate over the incoming rds messages. However
calling rds_inc_put while under this lock creates a potential deadlock.
rds_inc_put may eventually call rds_message_purge, which will lock
m_rs_lock. This is the incorrect locking order since m_rs_lock is
meant to be locked before the socket. To fix this, we move the message
item to a local list or variable that wont need rs_recv_lock protection.
Then we can safely call rds_inc_put on any item stored locally after
rs_recv_lock is released.

Fixes: possible deadlock in rds_message_put
Reported-by: [e-mail]

Fixes: possible deadlock in rds_wake_sk_sleep
Reported-by: [e-mail]

Signed-off-by: Allison Henderson <[e-mail]>
---
 net/rds/recv.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski Feb. 7, 2024, 2:48 a.m. UTC | #1

On Wed, 31 Jan 2024 10:54:17 -0700 [e-mail] wrote:
> Fixes: possible deadlock in rds_message_put

This is not a correct Fixes tag. Please look at git history

Allison Henderson Feb. 7, 2024, 5:23 p.m. UTC | #2

On Tue, 2024-02-06 at 18:48 -0800, Jakub Kicinski wrote:
> On Wed, 31 Jan 2024 10:54:17 -0700
> [e-mail] wrote:
> > Fixes: possible deadlock in rds_message_put
> 
> This is not a correct Fixes tag. Please look at git history

Sorry, the fixes tag identifies the offending commit?  I think it's an
existing bug since the code was introduced.  Which would be:

Fixes: bdbe6fbc6a2f (RDS: recv.c)

If that's not a useful tag, I can remove it too.  The syzbot bisect
points to a virtio commit 1628c6877f37, but that doesn't look correct
to me.  Let me know what you would prefer.  Thank you!

Allison

Jakub Kicinski Feb. 7, 2024, 5:59 p.m. UTC | #3

On Wed, 7 Feb 2024 17:23:53 +0000 Allison Henderson wrote:
> > This is not a correct Fixes tag. Please look at git history  
> 
> Sorry, the fixes tag identifies the offending commit?  I think it's an
> existing bug since the code was introduced.  Which would be:
> 
> Fixes: bdbe6fbc6a2f (RDS: recv.c)
> 
> If that's not a useful tag, I can remove it too.  The syzbot bisect
> points to a virtio commit 1628c6877f37, but that doesn't look correct
> to me.  Let me know what you would prefer.  Thank you!

The initial commit for the code base is very useful to backporters!
It tells them "you have to backport this all the way down".
Lack of a Fixes tag says "I don't know where the bug was added".

Fixes tags are more about telling backporters how far to backport
than about blame.

Allison Henderson Feb. 7, 2024, 8:17 p.m. UTC | #4

On Wed, 2024-02-07 at 09:59 -0800, Jakub Kicinski wrote:
> On Wed, 7 Feb 2024 17:23:53 +0000 Allison Henderson wrote:
> > > This is not a correct Fixes tag. Please look at git history  
> > 
> > Sorry, the fixes tag identifies the offending commit?  I think it's
> > an
> > existing bug since the code was introduced.  Which would be:
> > 
> > Fixes: bdbe6fbc6a2f (RDS: recv.c)
> > 
> > If that's not a useful tag, I can remove it too.  The syzbot bisect
> > points to a virtio commit 1628c6877f37, but that doesn't look
> > correct
> > to me.  Let me know what you would prefer.  Thank you!
> 
> The initial commit for the code base is very useful to backporters!
> It tells them "you have to backport this all the way down".
> Lack of a Fixes tag says "I don't know where the bug was added".
> 
> Fixes tags are more about telling backporters how far to backport
> than about blame.

Alrighty then, I will update the Fixes tag with the line above and
resend.  Thank you for the reviews!

Allison

Patch

diff --git a/net/rds/recv.c b/net/rds/recv.c
index c71b923764fd..5627f80013f8 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -425,6 +425,7 @@  static int rds_still_queued(struct rds_sock *rs, struct rds_incoming *inc,
 	struct sock *sk = rds_rs_to_sk(rs);
 	int ret = 0;
 	unsigned long flags;
+	struct rds_incoming *to_drop = NULL;
 
 	write_lock_irqsave(&rs->rs_recv_lock, flags);
 	if (!list_empty(&inc->i_item)) {
@@ -435,11 +436,14 @@  static int rds_still_queued(struct rds_sock *rs, struct rds_incoming *inc,
 					      -be32_to_cpu(inc->i_hdr.h_len),
 					      inc->i_hdr.h_dport);
 			list_del_init(&inc->i_item);
-			rds_inc_put(inc);
+			to_drop = inc;
 		}
 	}
 	write_unlock_irqrestore(&rs->rs_recv_lock, flags);
 
+	if (to_drop)
+		rds_inc_put(to_drop);
+
 	rdsdebug("inc %p rs %p still %d dropped %d\n", inc, rs, ret, drop);
 	return ret;
 }
@@ -758,16 +762,21 @@  void rds_clear_recv_queue(struct rds_sock *rs)
 	struct sock *sk = rds_rs_to_sk(rs);
 	struct rds_incoming *inc, *tmp;
 	unsigned long flags;
+	LIST_HEAD(to_drop);
 
 	write_lock_irqsave(&rs->rs_recv_lock, flags);
 	list_for_each_entry_safe(inc, tmp, &rs->rs_recv_queue, i_item) {
 		rds_recv_rcvbuf_delta(rs, sk, inc->i_conn->c_lcong,
 				      -be32_to_cpu(inc->i_hdr.h_len),
 				      inc->i_hdr.h_dport);
+		list_move(&inc->i_item, &to_drop);
+	}
+	write_unlock_irqrestore(&rs->rs_recv_lock, flags);
+
+	list_for_each_entry_safe(inc, tmp, &to_drop, i_item) {
 		list_del_init(&inc->i_item);
 		rds_inc_put(inc);
 	}
-	write_unlock_irqrestore(&rs->rs_recv_lock, flags);
 }
 
 /*