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

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

[allison.henderson]

From: Allison Henderson <allison.henderson@oracle.com>

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: syzbot+f9db6ff27b9bfdcfeca0@syzkaller.appspotmail.com

Fixes: possible deadlock in rds_wake_sk_sleep
Reported-by: syzbot+dcd73ff9291e6d34b3ab@syzkaller.appspotmail.com

Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
---
 net/rds/recv.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

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);
 }
 
 /*
-- 
2.34.1

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

[Jakub Kicinski]

On Wed, 31 Jan 2024 10:54:17 -0700 allison.henderson@oracle.com wrote:
> Fixes: possible deadlock in rds_message_put

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

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

[Allison Henderson]

On Tue, 2024-02-06 at 18:48 -0800, Jakub Kicinski wrote:
> On Wed, 31 Jan 2024 10:54:17 -0700
> allison.henderson@oracle.com 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

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

[Jakub Kicinski]

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.

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

[Allison Henderson]

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