Skip to contentSkip to conversation

rds bug fixes

PATCH In review Sowmini Varadhan 3 participants 8 messages Dec 22, 2017 – Dec 27, 2017

Subsystems (3)
Networking Network drivers RDS - reliable datagram sockets
lore
Share & tools
lore

Sowmini Varadhan

author
#0 lore raw

v2 cover rds bug fixes

Ran into pre-existing bugs when working on the fix for
https://www.spinics.net/lists/netdev/msg472849.html

The bugs fixed in this patchset are unrelated to the syzbot failure (which I'm still testing and trying to reproduce) but meanwhile, let's get these fixes out of the way.

V2: target net-next (rds:tcp patches have a dependancy on changes that are in net-next, but not yet in net)

Sowmini Varadhan (3):
rds; Reset rs->rs_bound_addr in rds_add_bound() failure path
rds: tcp: initialized t_tcp_detached to false
rds: tcp: cleanup if kmem_cache_alloc fails in rds_tcp_conn_alloc()

 net/rds/bind.c |    1 +
 net/rds/tcp.c  |   47 +++++++++++++++++++++++++++--------------------
 2 files changed, 28 insertions(+), 20 deletions(-)
author
#1 lore raw

PATCH v2 1/3 rds; Reset rs->rs_bound_addr in rds_add_bound() failure path

If the rds_sock is not added to the bind_hash_table, we must reset rs_bound_addr so that rds_remove_bound will not trip on this rds_sock.

rds_add_bound() does a rds_sock_put() in this failure path, so failing to reset rs_bound_addr will result in a socket refcount bug, and will trigger a WARN_ON with the stack shown below when the application subsequently tries to close the PF_RDS socket.

     WARNING: CPU: 20 PID: 19499 at net/rds/af_rds.c:496 \
		rds_sock_destruct+0x15/0x30 [rds]
       :
     __sk_destruct+0x21/0x190
     rds_remove_bound.part.13+0xb6/0x140 [rds]
     rds_release+0x71/0x120 [rds]
     sock_release+0x1a/0x70
     sock_close+0xe/0x20
     __fput+0xd5/0x210
     task_work_run+0x82/0xa0
     do_exit+0x2ce/0xb30
     ? syscall_trace_enter+0x1cc/0x2b0
     do_group_exit+0x39/0xa0
     SyS_exit_group+0x10/0x10
     do_syscall_64+0x61/0x1a0
Signed-off-by Sowmini Varadhan <[email protected]>

---
v2: target net-next, not net

 net/rds/bind.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
net/rds/bind.c +1 -0
index 75d43dc..5aa3a64 100644
@@ -114,6 +114,7 @@ static int rds_add_bound(struct rds_sock *rs, __be32 addr, __be16 *port)
 			  rs, &addr, (int)ntohs(*port));
 			break;
 		} else {
+			rs->rs_bound_addr = 0;
 			rds_sock_put(rs);
 			ret = -ENOMEM;
 			break;
Signature · 1 line
1.7.1
Acked-by on v2 1/3
#2 lore raw

Re: [PATCH V2 net-next 1/3] rds; Reset rs->rs_bound_addr in rds_add_bound() failure path

Quoted 25 lines · Sowmini Varadhan

If the rds_sock is not added to the bind_hash_table, we must reset rs_bound_addr so that rds_remove_bound will not trip on this rds_sock.

rds_add_bound() does a rds_sock_put() in this failure path, so failing to reset rs_bound_addr will result in a socket refcount bug, and will trigger a WARN_ON with the stack shown below when the application subsequently tries to close the PF_RDS socket.

      WARNING: CPU: 20 PID: 19499 at net/rds/af_rds.c:496 \
		rds_sock_destruct+0x15/0x30 [rds]
        :
      __sk_destruct+0x21/0x190
      rds_remove_bound.part.13+0xb6/0x140 [rds]
      rds_release+0x71/0x120 [rds]
      sock_release+0x1a/0x70
      sock_close+0xe/0x20
      __fput+0xd5/0x210
      task_work_run+0x82/0xa0
      do_exit+0x2ce/0xb30
      ? syscall_trace_enter+0x1cc/0x2b0
      do_group_exit+0x39/0xa0
      SyS_exit_group+0x10/0x10
      do_syscall_64+0x61/0x1a0
Quoted 4 lines
Signed-off-by Sowmini Varadhan <[email protected]>

---
v2: target net-next, not net

Acked-by Santosh Shilimkar <[email protected]>
author
#3 lore raw

PATCH v2 2/3 rds: tcp: initialize t_tcp_detached to false

In reply to Sowmini Varadhan

Commit f10b4cff98c6 ("rds: tcp: atomically purge entries from rds_tcp_conn_list during netns delete") adds the field t_tcp_detached, but this needs to be initialized explicitly to false.

Signed-off-by Sowmini Varadhan <[email protected]>

---
v2: target net-next, not net

 net/rds/tcp.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
net/rds/tcp.c +1 -0
index 39f502d..a61a498 100644
@@ -290,6 +290,7 @@ static int rds_tcp_conn_alloc(struct rds_connection *conn, gfp_t gfp)
 		tc->t_cpath = &conn->c_path[i];
 
 		spin_lock_irq(&rds_tcp_conn_lock);
+		tc->t_tcp_node_detached = false;
 		list_add_tail(&tc->t_tcp_node, &rds_tcp_conn_list);
 		spin_unlock_irq(&rds_tcp_conn_lock);
 		rdsdebug("rds_conn_path [%d] tc %p\n", i,
Signature · 1 line
1.7.1
author
#5 lore raw

PATCH v2 3/3 rds: tcp: cleanup if kmem_cache_alloc fails in rds_tcp_conn_alloc()

In reply to Sowmini Varadhan

If kmem_cache_alloc() fails in the middle of the for() loop, cleanup anything that might have been allocated so far.

Signed-off-by Sowmini Varadhan <[email protected]>

---
v2: target net-next, not net

 net/rds/tcp.c |   46 ++++++++++++++++++++++++++--------------------
 1 files changed, 26 insertions(+), 20 deletions(-)
net/rds/tcp.c +26 -20
index a61a498..2e554ef 100644
@@ -270,16 +270,33 @@ static int rds_tcp_laddr_check(struct net *net, __be32 addr)
 	return -EADDRNOTAVAIL;
 }
 
+static void rds_tcp_conn_free(void *arg)
+{
+	struct rds_tcp_connection *tc = arg;
+	unsigned long flags;
+
+	rdsdebug("freeing tc %p\n", tc);
+
+	spin_lock_irqsave(&rds_tcp_conn_lock, flags);
+	if (!tc->t_tcp_node_detached)
+		list_del(&tc->t_tcp_node);
+	spin_unlock_irqrestore(&rds_tcp_conn_lock, flags);
+
+	kmem_cache_free(rds_tcp_conn_slab, tc);
+}
+
 static int rds_tcp_conn_alloc(struct rds_connection *conn, gfp_t gfp)
 {
 	struct rds_tcp_connection *tc;
-	int i;
+	int i, j;
+	int ret = 0;
 
 	for (i = 0; i < RDS_MPATH_WORKERS; i++) {
 		tc = kmem_cache_alloc(rds_tcp_conn_slab, gfp);
-		if (!tc)
-			return -ENOMEM;
-
+		if (!tc) {
+			ret = -ENOMEM;
+			break;
+		}
 		mutex_init(&tc->t_conn_path_lock);
 		tc->t_sock = NULL;
 		tc->t_tinc = NULL;

@@ -296,22 +313,11 @@ static int rds_tcp_conn_alloc(struct rds_connection *conn, gfp_t gfp)
 		rdsdebug("rds_conn_path [%d] tc %p\n", i,
 			 conn->c_path[i].cp_transport_data);
 	}
-
-	return 0;
-}
-
-static void rds_tcp_conn_free(void *arg)
-{
-	struct rds_tcp_connection *tc = arg;
-	unsigned long flags;
-	rdsdebug("freeing tc %p\n", tc);
-
-	spin_lock_irqsave(&rds_tcp_conn_lock, flags);
-	if (!tc->t_tcp_node_detached)
-		list_del(&tc->t_tcp_node);
-	spin_unlock_irqrestore(&rds_tcp_conn_lock, flags);
-
-	kmem_cache_free(rds_tcp_conn_slab, tc);
+	if (ret) {
+		for (j = 0; j < i; j++)
+			rds_tcp_conn_free(conn->c_path[j].cp_transport_data);
+	}
+	return ret;
 }
 
 static bool list_has_conn(struct list_head *list, struct rds_connection *conn)
Signature · 1 line
1.7.1
maintainer
#7 lore raw
In reply to Sowmini Varadhan

From: Sowmini Varadhan <[email protected]>
Date: Fri, 22 Dec 2017 09:38:58 -0800

Quoted 9 lines

Ran into pre-existing bugs when working on the fix for
https://www.spinics.net/lists/netdev/msg472849.html

The bugs fixed in this patchset are unrelated to the syzbot failure (which I'm still testing and trying to reproduce) but meanwhile, let's get these fixes out of the way.

V2: target net-next (rds:tcp patches have a dependancy on changes that are in net-next, but not yet in net)

Series applied, thanks.