[PATCH V2 net-next 0/3] rds bug fixes

[PATCH V2 net-next 0/3] rds bug fixes

[Sowmini Varadhan]

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(-)

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

[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

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
v2: target net-next, not net

 net/rds/bind.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/rds/bind.c b/net/rds/bind.c
index 75d43dc..5aa3a64 100644
--- a/net/rds/bind.c
+++ b/net/rds/bind.c
@@ -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;
-- 
1.7.1

[PATCH V2 net-next 2/3] rds: tcp: initialize t_tcp_detached to false

[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 <sowmini.varadhan@oracle.com>
---
v2: target net-next, not net

 net/rds/tcp.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 39f502d..a61a498 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -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,
-- 
1.7.1

[PATCH V2 net-next 3/3] rds: tcp: cleanup if kmem_cache_alloc fails in rds_tcp_conn_alloc()

[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 <sowmini.varadhan@oracle.com>
---
v2: target net-next, not net

 net/rds/tcp.c |   46 ++++++++++++++++++++++++++--------------------
 1 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index a61a498..2e554ef 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -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)
-- 
1.7.1

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

[Santosh Shilimkar]

On 12/22/2017 9:38 AM, Sowmini Varadhan wrote:
> 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 <sowmini.varadhan@oracle.com>
> ---
> v2: target net-next, not net
> 
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>

Re: [PATCH V2 net-next 2/3] rds: tcp: initialize t_tcp_detached to false

[Santosh Shilimkar]

On 12/22/2017 9:39 AM, Sowmini Varadhan wrote:
> 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 <sowmini.varadhan@oracle.com>
> ---
> v2: target net-next, not net
> 
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>

Re: [PATCH V2 net-next 3/3] rds: tcp: cleanup if kmem_cache_alloc fails in rds_tcp_conn_alloc()

[Santosh Shilimkar]

On 12/22/2017 9:39 AM, Sowmini Varadhan wrote:
> 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 <sowmini.varadhan@oracle.com>
> ---
> v2: target net-next, not net
> 
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>

Re: [PATCH V2 net-next 0/3] rds bug fixes

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Fri, 22 Dec 2017 09:38:58 -0800

> 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.