rds-tcp netns delete related fixes

PATCH In review Sowmini Varadhan 3 participants 8 messages Nov 30, 2017 – Dec 1, 2017

Subsystems (3)

Networking Network drivers RDS - reliable datagram sockets

lore

Share & tools

lore

Sowmini Varadhan

author

2017-11-30 19:25 UTC #0 lore raw

cover rds-tcp netns delete related fixes

Patchset contains cleanup and bug fixes. Patch 1 is the removal of some redundant code/functions. Patch 2 and 3 are fixes for corner cases identified by syzkaller. I've not been able to reproduce the actual use-after-free race flagged in the syzkaller reports, thus these fixes are based on code inspection plus manual testing to make sure the modified code paths are executed without problems in the commonly encountered timing cases.

Sowmini Varadhan (3):
rds: tcp: remove redundant function rds_tcp_conn_paths_destroy()
rds: tcp: correctly sequence cleanup on netns deletion.
rds: tcp: atomically purge entries from rds_tcp_conn_list during
netns delete

 net/rds/connection.c |    3 ++-
 net/rds/rds.h        |    6 +++---
 net/rds/tcp.c        |   38 ++++++++++----------------------------
 net/rds/tcp.h        |    1 +
 4 files changed, 16 insertions(+), 32 deletions(-)

Sowmini Varadhan

author

2017-11-30 19:25 UTC #1 lore raw

PATCH 1/3 rds: tcp: remove redundant function rds_tcp_conn_paths_destroy()

A side-effect of Commit c14b0366813a ("rds: tcp: set linger to 1 when unloading a rds-tcp") is that we always send a RST on the tcp connection for rds_conn_destroy(), so rds_tcp_conn_paths_destroy() is not needed any more and is removed in this patch.

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

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

net/rds/tcp.c +1 ~~-24~~

index 6b7ee71..222cc53 100644
@@ -495,27 +495,6 @@ static void __net_exit rds_tcp_exit_net(struct net *net)
 	.size = sizeof(struct rds_tcp_net),
 };
 
-/* explicitly send a RST on each socket, thereby releasing any socket refcnts
- * that may otherwise hold up netns deletion.
- */
-static void rds_tcp_conn_paths_destroy(struct rds_connection *conn)
-{
-	struct rds_conn_path *cp;
-	struct rds_tcp_connection *tc;
-	int i;
-	struct sock *sk;
-
-	for (i = 0; i < RDS_MPATH_WORKERS; i++) {
-		cp = &conn->c_path[i];
-		tc = cp->cp_transport_data;
-		if (!tc->t_sock)
-			continue;
-		sk = tc->t_sock->sk;
-		sk->sk_prot->disconnect(sk, 0);
-		tcp_done(sk);
-	}
-}
-
 static void rds_tcp_kill_sock(struct net *net)
 {
 	struct rds_tcp_connection *tc, *_tc;

@@ -535,10 +514,8 @@ static void rds_tcp_kill_sock(struct net *net)
 			list_move_tail(&tc->t_tcp_node, &tmp_list);
 	}
 	spin_unlock_irq(&rds_tcp_conn_lock);
-	list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node) {
-		rds_tcp_conn_paths_destroy(tc->t_cpath->cp_conn);
+	list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)
 		rds_conn_destroy(tc->t_cpath->cp_conn);
-	}
 }
 
 void *rds_tcp_listen_sock_def_readable(struct net *net)

Signature · 1 line

1.7.1

Santosh Shilimkar

Acked-by on v1 1/3

2017-11-30 20:20 UTC #2 lore raw

Re: [PATCH net-next 1/3] rds: tcp: remove redundant function rds_tcp_conn_paths_destroy()

Quoted 7 lines · Sowmini Varadhan

A side-effect of Commit c14b0366813a ("rds: tcp: set linger to 1 when unloading a rds-tcp") is that we always send a RST on the tcp connection for rds_conn_destroy(), so rds_tcp_conn_paths_destroy() is not needed any more and is removed in this patch.
Signed-off-by Sowmini Varadhan <[email protected]>
---

Looks good.

Acked-by Santosh Shilimkar <[email protected]>

Sowmini Varadhan

author

2017-11-30 19:25 UTC #3 lore raw

PATCH 2/3 rds: tcp: correctly sequence cleanup on netns deletion.

In reply to Sowmini Varadhan

Commit 8edc3affc077 ("rds: tcp: Take explicit refcounts on struct net") introduces a regression in rds-tcp netns cleanup. The cleanup_net(), (and thus rds_tcp_dev_event notification) is only called from put_net() when all netns refcounts go to 0, but this cannot happen if the rds_connection itself is holding a c_net ref that it expects to release in rds_tcp_kill_sock.

Instead, the rds_tcp_kill_sock callback should make sure to tear down state carefully, ensuring that the socket teardown is only done after all data-structures and workqs that depend on it are quiesced.

The original motivation for commit 8edc3affc077 ("rds: tcp: Take explicit refcounts on struct net") was to resolve a race condition reported by syzkaller where workqs for tx/rx/connect were triggered after the namespace was deleted. Those worker threads should have been cancelled/flushed before socket tear-down and indeed, rds_conn_path_destroy() does try to sequence this by doing

     /* cancel cp_send_w */
     /* cancel cp_recv_w */
     /* flush cp_down_w */
     /* free data structures */

Here the "flush cp_down_w" will trigger rds_conn_shutdown and thus invoke rds_tcp_conn_path_shutdown() to close the tcp socket, so that we ought to have satisfied the requirement that "socket-close is done after all other dependent state is quiesced". However, rds_conn_shutdown has a bug in that it *always* triggers the reconnect workq (and if connection is successful, we always restart tx/rx workqs so with the right timing, we risk the race conditions reported by syzkaller).

Netns deletion is like module teardown- no need to restart a reconnect in this case. We can use the c_destroy_in_prog bit to avoid restarting the reconnect.

Fixes 8edc3affc077 ("rds: tcp: Take explicit refcounts on struct net")

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

---
 net/rds/connection.c |    3 ++-
 net/rds/rds.h        |    6 +++---
 net/rds/tcp.c        |    4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

net/rds/connection.c +2 -1

index 7ee2d5d..9efc82c 100644
@@ -366,6 +366,8 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
 	 * to the conn hash, so we never trigger a reconnect on this
 	 * conn - the reconnect is always triggered by the active peer. */
 	cancel_delayed_work_sync(&cp->cp_conn_w);
+	if (conn->c_destroy_in_prog)
+		return;
 	rcu_read_lock();
 	if (!hlist_unhashed(&conn->c_hash_node)) {
 		rcu_read_unlock();

@@ -445,7 +447,6 @@ void rds_conn_destroy(struct rds_connection *conn)
 	 */
 	rds_cong_remove_conn(conn);
 
-	put_net(conn->c_net);
 	kfree(conn->c_path);
 	kmem_cache_free(rds_conn_slab, conn);

net/rds/rds.h +3 -3

index 2e0315b..d11301b 100644
@@ -149,7 +149,7 @@ struct rds_connection {
 
 	/* Protocol version */
 	unsigned int		c_version;
-	struct net		*c_net;
+	possible_net_t		c_net;
 
 	struct list_head	c_map_item;
 	unsigned long		c_map_queued;

@@ -164,13 +164,13 @@ struct rds_connection {
 static inline
 struct net *rds_conn_net(struct rds_connection *conn)
 {
-	return conn->c_net;
+	return read_pnet(&conn->c_net);
 }
 
 static inline
 void rds_conn_net_set(struct rds_connection *conn, struct net *net)
 {
-	conn->c_net = get_net(net);
+	write_pnet(&conn->c_net, net);
 }
 
 #define RDS_FLAG_CONG_BITMAP	0x01

net/rds/tcp.c +2 -2

index 222cc53..f580f72 100644
@@ -506,7 +506,7 @@ static void rds_tcp_kill_sock(struct net *net)
 	rds_tcp_listen_stop(lsock, &rtn->rds_tcp_accept_w);
 	spin_lock_irq(&rds_tcp_conn_lock);
 	list_for_each_entry_safe(tc, _tc, &rds_tcp_conn_list, t_tcp_node) {
-		struct net *c_net = tc->t_cpath->cp_conn->c_net;
+		struct net *c_net = read_pnet(&tc->t_cpath->cp_conn->c_net);
 
 		if (net != c_net || !tc->t_sock)
 			continue;

@@ -563,7 +563,7 @@ static void rds_tcp_sysctl_reset(struct net *net)
 
 	spin_lock_irq(&rds_tcp_conn_lock);
 	list_for_each_entry_safe(tc, _tc, &rds_tcp_conn_list, t_tcp_node) {
-		struct net *c_net = tc->t_cpath->cp_conn->c_net;
+		struct net *c_net = read_pnet(&tc->t_cpath->cp_conn->c_net);
 
 		if (net != c_net || !tc->t_sock)
 			continue;

Signature · 1 line

1.7.1

Santosh Shilimkar

Acked-by on v1 2/3

2017-11-30 20:27 UTC #4 lore raw

Re: [PATCH net-next 2/3] rds: tcp: correctly sequence cleanup on netns deletion.

Quoted 48 lines · Sowmini Varadhan

Commit 8edc3affc077 ("rds: tcp: Take explicit refcounts on struct net") introduces a regression in rds-tcp netns cleanup. The cleanup_net(), (and thus rds_tcp_dev_event notification) is only called from put_net() when all netns refcounts go to 0, but this cannot happen if the rds_connection itself is holding a c_net ref that it expects to release in rds_tcp_kill_sock.
Instead, the rds_tcp_kill_sock callback should make sure to tear down state carefully, ensuring that the socket teardown is only done after all data-structures and workqs that depend on it are quiesced.
The original motivation for commit 8edc3affc077 ("rds: tcp: Take explicit refcounts on struct net") was to resolve a race condition reported by syzkaller where workqs for tx/rx/connect were triggered after the namespace was deleted. Those worker threads should have been cancelled/flushed before socket tear-down and indeed, rds_conn_path_destroy() does try to sequence this by doing
      /* cancel cp_send_w */
      /* cancel cp_recv_w */
      /* flush cp_down_w */
      /* free data structures */
Here the "flush cp_down_w" will trigger rds_conn_shutdown and thus invoke rds_tcp_conn_path_shutdown() to close the tcp socket, so that we ought to have satisfied the requirement that "socket-close is done after all other dependent state is quiesced". However, rds_conn_shutdown has a bug in that it *always* triggers the reconnect workq (and if connection is successful, we always restart tx/rx workqs so with the right timing, we risk the race conditions reported by syzkaller).
Netns deletion is like module teardown- no need to restart a reconnect in this case. We can use the c_destroy_in_prog bit to avoid restarting the reconnect.
Fixes 8edc3affc077 ("rds: tcp: Take explicit refcounts on struct net")
Signed-off-by Sowmini Varadhan <[email protected]>
---
  net/rds/connection.c |    3 ++-
  net/rds/rds.h        |    6 +++---
  net/rds/tcp.c        |    4 ++--
  3 files changed, 7 insertions(+), 6 deletions(-)
net/rds/connection.c +0 -0
index 7ee2d5d..9efc82c 100644

net/rds/connection.c void rds_conn_shutdown(struct rds_conn_path *cp) · line 370

Quoted 5 lines

net/rds/connection.c +2 -0

@@ -366,6 +366,8 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
  	 * to the conn hash, so we never trigger a reconnect on this
  	 * conn - the reconnect is always triggered by the active peer. */
  	cancel_delayed_work_sync(&cp->cp_conn_w);
+	if (conn->c_destroy_in_prog)
+		return;

Not related to this patch but it will be more safe to use cp_flags or if needed add flag and conn level for bundle and use bit wise to avoid possible races to set c_destroy_in_prog.

Something similar to RDS_DESTROY_PENDING etc.

The patch itself looks good to me in terms of netns ref counting.

Acked-by Santosh Shilimkar <[email protected]>

Sowmini Varadhan

author

2017-11-30 19:25 UTC #5 lore raw

PATCH 3/3 rds: tcp: atomically purge entries from rds_tcp_conn_list during netns delete

In reply to Sowmini Varadhan

The rds_tcp_kill_sock() function parses the rds_tcp_conn_list to find the rds_connection entries marked for deletion as part of the netns deletion under the protection of the rds_tcp_conn_lock. Since the rds_tcp_conn_list tracks rds_tcp_connections (which have a 1:1 mapping with rds_conn_path), multiple tc entries in the rds_tcp_conn_list will map to a single rds_connection, and will be deleted as part of the rds_conn_destroy() operation that is done outside the rds_tcp_conn_lock.

The rds_tcp_conn_list traversal done under the protection of rds_tcp_conn_lock should not leave any doomed tc entries in the list after the rds_tcp_conn_lock is released, else another concurrently executiong netns delete (for a differnt netns) thread may trip on these entries.

Reported-by syzbot <[email protected]>

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

---
 net/rds/tcp.c |    9 +++++++--
 net/rds/tcp.h |    1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

net/rds/tcp.c +7 -2

index f580f72..39f502d 100644
@@ -306,7 +306,8 @@ static void rds_tcp_conn_free(void *arg)
 	rdsdebug("freeing tc %p\n", tc);
 
 	spin_lock_irqsave(&rds_tcp_conn_lock, flags);
-	list_del(&tc->t_tcp_node);
+	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);

@@ -510,8 +511,12 @@ static void rds_tcp_kill_sock(struct net *net)
 
 		if (net != c_net || !tc->t_sock)
 			continue;
-		if (!list_has_conn(&tmp_list, tc->t_cpath->cp_conn))
+		if (!list_has_conn(&tmp_list, tc->t_cpath->cp_conn)) {
 			list_move_tail(&tc->t_tcp_node, &tmp_list);
+		} else {
+			list_del(&tc->t_tcp_node);
+			tc->t_tcp_node_detached = true;
+		}
 	}
 	spin_unlock_irq(&rds_tcp_conn_lock);
 	list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)

net/rds/tcp.h +1 -0

index f8800b7..8775349 100644
@@ -11,6 +11,7 @@ struct rds_tcp_incoming {
 struct rds_tcp_connection {
 
 	struct list_head	t_tcp_node;
+	bool			t_tcp_node_detached;
 	struct rds_conn_path	*t_cpath;
 	/* t_conn_path_lock synchronizes the connection establishment between
 	 * rds_tcp_accept_one and rds_tcp_conn_path_connect

Signature · 1 line

1.7.1

Santosh Shilimkar

Acked-by on v1 3/3

2017-11-30 20:37 UTC #6 lore raw

Re: [PATCH net-next 3/3] rds: tcp: atomically purge entries from rds_tcp_conn_list during netns delete

Quoted 15 lines · Sowmini Varadhan

The rds_tcp_kill_sock() function parses the rds_tcp_conn_list to find the rds_connection entries marked for deletion as part of the netns deletion under the protection of the rds_tcp_conn_lock. Since the rds_tcp_conn_list tracks rds_tcp_connections (which have a 1:1 mapping with rds_conn_path), multiple tc entries in the rds_tcp_conn_list will map to a single rds_connection, and will be deleted as part of the rds_conn_destroy() operation that is done outside the rds_tcp_conn_lock.
The rds_tcp_conn_list traversal done under the protection of rds_tcp_conn_lock should not leave any doomed tc entries in the list after the rds_tcp_conn_lock is released, else another concurrently executiong netns delete (for a differnt netns) thread may trip on these entries.

Quoted 3 lines

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

Acked-by Santosh Shilimkar <[email protected]>

David Miller

maintainer

2017-12-01 20:25 UTC #7 lore raw

In reply to Sowmini Varadhan

From: Sowmini Varadhan <[email protected]>
Date: Thu, 30 Nov 2017 11:11:26 -0800

Quoted 7 lines

Patchset contains cleanup and bug fixes. Patch 1 is the removal of some redundant code/functions. Patch 2 and 3 are fixes for corner cases identified by syzkaller. I've not been able to reproduce the actual use-after-free race flagged in the syzkaller reports, thus these fixes are based on code inspection plus manual testing to make sure the modified code paths are executed without problems in the commonly encountered timing cases.

Series applied, thanks.