[PATCH net 0/3] RDS: TCP: socket locking RDS packet assembly fixes

[PATCH net 0/3] RDS: TCP: socket locking RDS packet assembly fixes

[Sowmini Varadhan]

This three part patchset fixes bugs in synchronization between
rds_tcp_accept_one() and the rds-tcp send/recv path.

Patch 1 ensures that the lock_sock() is taken appropriately
and the RDS datagram reassembly state is reset  to synchronize
with the receive path.

Patch 2 ensures that partially sent RDS datagrams will get
retransmitted after rds_tcp_accept_one() switches sockets.

Patch 3 fixes a race window which would prematurely re-enable
rds_send_xmit() before the rds_tcp_connection setup has been
completed in rds_tcp_accept_one().

Sowmini Varadhan (3):
  RDS: TCP: Add/use rds_tcp_reset_callbacks to reset tcp socket safely
  RDS: TCP: Retransmit half-sent datagrams when switching sockets in
    rds_tcp_reset_callbacks
  RDS: TCP: fix race windows in send-path quiescence by
    rds_tcp_accept_one()

 net/rds/rds.h         |    2 +
 net/rds/send.c        |    1 +
 net/rds/tcp.c         |   78 +++++++++++++++++++++++++++++++++++++++++++++++--
 net/rds/tcp.h         |    1 +
 net/rds/tcp_connect.c |    2 +-
 net/rds/tcp_listen.c  |   20 ++++--------
 net/rds/threads.c     |   10 +++++-
 7 files changed, 95 insertions(+), 19 deletions(-)

[PATCH net 1/3] RDS: TCP: Add/use rds_tcp_reset_callbacks to reset tcp socket safely

[Sowmini Varadhan]

When rds_tcp_accept_one() has to replace the existing tcp socket
with a newer tcp socket (duelling-syn resolution), it must lock_sock()
to suppress the rds_tcp_data_recv() path while callbacks are being
changed.  Also, existing RDS datagram reassembly state must be reset,
so that the next datagram on the new socket  does not have corrupted
state. Similarly when resetting the newly accepted socket, appropriate
locks and synchronization is needed.

This commit ensures correct synchronization by invoking
kernel_sock_shutdown to reset a newly accepted sock, and by taking
appropriate lock_sock()s (for old and new sockets) when resetting
existing callbacks.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
 net/rds/tcp.c        |   65 +++++++++++++++++++++++++++++++++++++++++++++++--
 net/rds/tcp.h        |    1 +
 net/rds/tcp_listen.c |   13 +++-------
 3 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 86187da..8faa0b1 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -126,9 +126,68 @@ void rds_tcp_restore_callbacks(struct socket *sock,
 }
 
 /*
- * This is the only path that sets tc->t_sock.  Send and receive trust that
- * it is set.  The RDS_CONN_UP bit protects those paths from being
- * called while it isn't set.
+ * rds_tcp_reset_callbacks() switches the to the new sock and
+ * returns the existing tc->t_sock.
+ *
+ * The only functions that set tc->t_sock are rds_tcp_set_callbacks
+ * and rds_tcp_reset_callbacks.  Send and receive trust that
+ * it is set.  The absence of RDS_CONN_UP bit protects those paths
+ * from being called while it isn't set.
+ */
+void rds_tcp_reset_callbacks(struct socket *sock,
+			     struct rds_connection *conn)
+{
+	struct rds_tcp_connection *tc = conn->c_transport_data;
+	struct socket *osock = tc->t_sock;
+
+	if (!osock)
+		goto newsock;
+
+	/* Need to resolve a duelling SYN between peers.
+	 * We have an outstanding SYN to this peer, which may
+	 * potentially have transitioned to the RDS_CONN_UP state,
+	 * so we must quiesce any send threads before resetting
+	 * c_transport_data. We quiesce these threads by setting
+	 * cp_state to something other than RDS_CONN_UP, and then
+	 * waiting for any existing threads in rds_send_xmit to
+	 * complete release_in_xmit(). (Subsequent threads entering
+	 * rds_send_xmit() will bail on !rds_conn_up().
+	 */
+	lock_sock(osock->sk);
+	/* reset receive side state for rds_tcp_data_recv() for osock  */
+	if (tc->t_tinc) {
+		rds_inc_put(&tc->t_tinc->ti_inc);
+		tc->t_tinc = NULL;
+	}
+	tc->t_tinc_hdr_rem = sizeof(struct rds_header);
+	tc->t_tinc_data_rem = 0;
+	tc->t_sock = NULL;
+
+	write_lock_bh(&osock->sk->sk_callback_lock);
+
+	osock->sk->sk_user_data = NULL;
+	osock->sk->sk_data_ready = tc->t_orig_data_ready;
+	osock->sk->sk_write_space = tc->t_orig_write_space;
+	osock->sk->sk_state_change = tc->t_orig_state_change;
+	write_unlock_bh(&osock->sk->sk_callback_lock);
+	release_sock(osock->sk);
+	sock_release(osock);
+newsock:
+	lock_sock(sock->sk);
+	write_lock_bh(&sock->sk->sk_callback_lock);
+	tc->t_sock = sock;
+	sock->sk->sk_user_data = conn;
+	sock->sk->sk_data_ready = rds_tcp_data_ready;
+	sock->sk->sk_write_space = rds_tcp_write_space;
+	sock->sk->sk_state_change = rds_tcp_state_change;
+
+	write_unlock_bh(&sock->sk->sk_callback_lock);
+	release_sock(sock->sk);
+}
+
+/* Add tc to rds_tcp_tc_list and set tc->t_sock. See comments
+ * above rds_tcp_reset_callbacks for notes about synchronization
+ * with data path
  */
 void rds_tcp_set_callbacks(struct socket *sock, struct rds_connection *conn)
 {
diff --git a/net/rds/tcp.h b/net/rds/tcp.h
index 41c2283..ec0602b 100644
--- a/net/rds/tcp.h
+++ b/net/rds/tcp.h
@@ -50,6 +50,7 @@ struct rds_tcp_statistics {
 void rds_tcp_tune(struct socket *sock);
 void rds_tcp_nonagle(struct socket *sock);
 void rds_tcp_set_callbacks(struct socket *sock, struct rds_connection *conn);
+void rds_tcp_reset_callbacks(struct socket *sock, struct rds_connection *conn);
 void rds_tcp_restore_callbacks(struct socket *sock,
 			       struct rds_tcp_connection *tc);
 u32 rds_tcp_snd_nxt(struct rds_tcp_connection *tc);
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index 4bf4bef..d9fe536 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -78,7 +78,6 @@ int rds_tcp_accept_one(struct socket *sock)
 	struct inet_sock *inet;
 	struct rds_tcp_connection *rs_tcp = NULL;
 	int conn_state;
-	struct sock *nsk;
 
 	if (!sock) /* module unload or netns delete in progress */
 		return -ENETUNREACH;
@@ -139,23 +138,19 @@ int rds_tcp_accept_one(struct socket *sock)
 			atomic_set(&conn->c_state, RDS_CONN_CONNECTING);
 			wait_event(conn->c_waitq,
 				   !test_bit(RDS_IN_XMIT, &conn->c_flags));
-			rds_tcp_restore_callbacks(rs_tcp->t_sock, rs_tcp);
+			rds_tcp_reset_callbacks(new_sock, conn);
 			conn->c_outgoing = 0;
 		}
+	} else {
+		rds_tcp_set_callbacks(new_sock, conn);
 	}
-	rds_tcp_set_callbacks(new_sock, conn);
 	rds_connect_complete(conn); /* marks RDS_CONN_UP */
 	new_sock = NULL;
 	ret = 0;
 	goto out;
 rst_nsk:
 	/* reset the newly returned accept sock and bail */
-	nsk = new_sock->sk;
-	rds_tcp_stats_inc(s_tcp_listen_closed_stale);
-	nsk->sk_user_data = NULL;
-	nsk->sk_prot->disconnect(nsk, 0);
-	tcp_done(nsk);
-	new_sock = NULL;
+	kernel_sock_shutdown(new_sock, SHUT_RDWR);
 	ret = 0;
 out:
 	if (rs_tcp)
-- 
1.7.1

Re: [PATCH net 1/3] RDS: TCP: Add/use rds_tcp_reset_callbacks to reset tcp socket safely

[Santosh Shilimkar]

On 6/4/2016 1:59 PM, Sowmini Varadhan wrote:
> When rds_tcp_accept_one() has to replace the existing tcp socket
> with a newer tcp socket (duelling-syn resolution), it must lock_sock()
> to suppress the rds_tcp_data_recv() path while callbacks are being
> changed.  Also, existing RDS datagram reassembly state must be reset,
> so that the next datagram on the new socket  does not have corrupted
> state. Similarly when resetting the newly accepted socket, appropriate
> locks and synchronization is needed.
>
> This commit ensures correct synchronization by invoking
> kernel_sock_shutdown to reset a newly accepted sock, and by taking
> appropriate lock_sock()s (for old and new sockets) when resetting
> existing callbacks.
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>

[PATCH net 2/3] RDS: TCP: Retransmit half-sent datagrams when switching sockets in rds_tcp_reset_callbacks

[Sowmini Varadhan]

When we switch a connection's sockets in rds_tcp_rest_callbacks,
any partially sent datagram must be retransmitted on the new
socket so that the receiver can correctly reassmble the RDS
datagram. Use rds_send_reset() which is designed for this purpose.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
 net/rds/send.c |    1 +
 net/rds/tcp.c  |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/rds/send.c b/net/rds/send.c
index c9cdb35..b1962f8 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -99,6 +99,7 @@ void rds_send_reset(struct rds_connection *conn)
 	list_splice_init(&conn->c_retrans, &conn->c_send_queue);
 	spin_unlock_irqrestore(&conn->c_lock, flags);
 }
+EXPORT_SYMBOL_GPL(rds_send_reset);
 
 static int acquire_in_xmit(struct rds_connection *conn)
 {
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 8faa0b1..7ab1b41 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -173,6 +173,7 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	release_sock(osock->sk);
 	sock_release(osock);
 newsock:
+	rds_send_reset(conn);
 	lock_sock(sock->sk);
 	write_lock_bh(&sock->sk->sk_callback_lock);
 	tc->t_sock = sock;
-- 
1.7.1

Re: [PATCH net 2/3] RDS: TCP: Retransmit half-sent datagrams when switching sockets in rds_tcp_reset_callbacks

[Santosh Shilimkar]

On 6/4/2016 1:59 PM, Sowmini Varadhan wrote:
> When we switch a connection's sockets in rds_tcp_rest_callbacks,
> any partially sent datagram must be retransmitted on the new
> socket so that the receiver can correctly reassmble the RDS
> datagram. Use rds_send_reset() which is designed for this purpose.
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>

[PATCH net 3/3] RDS: TCP: fix race windows in send-path quiescence by rds_tcp_accept_one()

[Sowmini Varadhan]

The send path needs to be quiesced before resetting callbacks from
rds_tcp_accept_one(), and commit eb192840266f ("RDS:TCP: Synchronize
rds_tcp_accept_one with rds_send_xmit when resetting t_sock") achieves
this using the c_state and RDS_IN_XMIT bit following the pattern
used by rds_conn_shutdown(). However this leaves the possibility
of a race window as shown in the sequence below
    take t_conn_lock in rds_tcp_conn_connect
    send outgoing syn to peer
    drop t_conn_lock in rds_tcp_conn_connect
    incoming from peer triggers rds_tcp_accept_one, conn is
	marked CONNECTING
    wait for RDS_IN_XMIT to quiesce any rds_send_xmit threads
    call rds_tcp_reset_callbacks
    [.. race-window where incoming syn-ack can cause the conn
	to be marked UP from rds_tcp_state_change ..]
    lock_sock called from rds_tcp_reset_callbacks, and we set
	t_sock to null
As soon as the conn is marked UP in the race-window above, rds_send_xmit()
threads will proceed to rds_tcp_xmit and may encounter a null-pointer
deref on the t_sock.

Given that rds_tcp_state_change() is invoked in softirq context, whereas
rds_tcp_reset_callbacks() is in workq context, and testing for RDS_IN_XMIT
after lock_sock could result in a deadlock with tcp_sendmsg, this
commit fixes the race by using a new c_state, RDS_TCP_RESETTING, which
will prevent a transition to RDS_CONN_UP from rds_tcp_state_change().

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
 net/rds/rds.h         |    2 ++
 net/rds/tcp.c         |   14 +++++++++++++-
 net/rds/tcp_connect.c |    2 +-
 net/rds/tcp_listen.c  |    7 +++----
 net/rds/threads.c     |   10 ++++++++--
 5 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/net/rds/rds.h b/net/rds/rds.h
index 80256b0..387df5f 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -74,6 +74,7 @@ enum {
 	RDS_CONN_CONNECTING,
 	RDS_CONN_DISCONNECTING,
 	RDS_CONN_UP,
+	RDS_CONN_RESETTING,
 	RDS_CONN_ERROR,
 };
 
@@ -813,6 +814,7 @@ void rds_connect_worker(struct work_struct *);
 void rds_shutdown_worker(struct work_struct *);
 void rds_send_worker(struct work_struct *);
 void rds_recv_worker(struct work_struct *);
+void rds_connect_path_complete(struct rds_connection *conn, int curr);
 void rds_connect_complete(struct rds_connection *conn);
 
 /* transport.c */
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 7ab1b41..74ee126 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -148,11 +148,23 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	 * potentially have transitioned to the RDS_CONN_UP state,
 	 * so we must quiesce any send threads before resetting
 	 * c_transport_data. We quiesce these threads by setting
-	 * cp_state to something other than RDS_CONN_UP, and then
+	 * c_state to something other than RDS_CONN_UP, and then
 	 * waiting for any existing threads in rds_send_xmit to
 	 * complete release_in_xmit(). (Subsequent threads entering
 	 * rds_send_xmit() will bail on !rds_conn_up().
+	 *
+	 * However an incoming syn-ack at this point would end up
+	 * marking the conn as RDS_CONN_UP, and would again permit
+	 * rds_send_xmi() threads through, so ideally we would
+	 * synchronize on RDS_CONN_UP after lock_sock(), but cannot
+	 * do that: waiting on !RDS_IN_XMIT after lock_sock() may
+	 * end up deadlocking with tcp_sendmsg(), and the RDS_IN_XMIT
+	 * would not get set. As a result, we set c_state to
+	 * RDS_CONN_RESETTTING, to ensure that rds_tcp_state_change
+	 * cannot mark rds_conn_path_up() in the window before lock_sock()
 	 */
+	atomic_set(&conn->c_state, RDS_CONN_RESETTING);
+	wait_event(conn->c_waitq, !test_bit(RDS_IN_XMIT, &conn->c_flags));
 	lock_sock(osock->sk);
 	/* reset receive side state for rds_tcp_data_recv() for osock  */
 	if (tc->t_tinc) {
diff --git a/net/rds/tcp_connect.c b/net/rds/tcp_connect.c
index fb82e0a..fba13d0 100644
--- a/net/rds/tcp_connect.c
+++ b/net/rds/tcp_connect.c
@@ -60,7 +60,7 @@ void rds_tcp_state_change(struct sock *sk)
 		case TCP_SYN_RECV:
 			break;
 		case TCP_ESTABLISHED:
-			rds_connect_complete(conn);
+			rds_connect_path_complete(conn, RDS_CONN_CONNECTING);
 			break;
 		case TCP_CLOSE_WAIT:
 		case TCP_CLOSE:
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index d9fe536..686b1d0 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -135,16 +135,15 @@ int rds_tcp_accept_one(struct socket *sock)
 		    !conn->c_outgoing) {
 			goto rst_nsk;
 		} else {
-			atomic_set(&conn->c_state, RDS_CONN_CONNECTING);
-			wait_event(conn->c_waitq,
-				   !test_bit(RDS_IN_XMIT, &conn->c_flags));
 			rds_tcp_reset_callbacks(new_sock, conn);
 			conn->c_outgoing = 0;
+			/* rds_connect_path_complete() marks RDS_CONN_UP */
+			rds_connect_path_complete(conn, RDS_CONN_DISCONNECTING);
 		}
 	} else {
 		rds_tcp_set_callbacks(new_sock, conn);
+		rds_connect_path_complete(conn, RDS_CONN_CONNECTING);
 	}
-	rds_connect_complete(conn); /* marks RDS_CONN_UP */
 	new_sock = NULL;
 	ret = 0;
 	goto out;
diff --git a/net/rds/threads.c b/net/rds/threads.c
index 454aa6d..4a32304 100644
--- a/net/rds/threads.c
+++ b/net/rds/threads.c
@@ -71,9 +71,9 @@
 struct workqueue_struct *rds_wq;
 EXPORT_SYMBOL_GPL(rds_wq);
 
-void rds_connect_complete(struct rds_connection *conn)
+void rds_connect_path_complete(struct rds_connection *conn, int curr)
 {
-	if (!rds_conn_transition(conn, RDS_CONN_CONNECTING, RDS_CONN_UP)) {
+	if (!rds_conn_transition(conn, curr, RDS_CONN_UP)) {
 		printk(KERN_WARNING "%s: Cannot transition to state UP, "
 				"current state is %d\n",
 				__func__,
@@ -90,6 +90,12 @@ void rds_connect_complete(struct rds_connection *conn)
 	queue_delayed_work(rds_wq, &conn->c_send_w, 0);
 	queue_delayed_work(rds_wq, &conn->c_recv_w, 0);
 }
+EXPORT_SYMBOL_GPL(rds_connect_path_complete);
+
+void rds_connect_complete(struct rds_connection *conn)
+{
+	rds_connect_path_complete(conn, RDS_CONN_CONNECTING);
+}
 EXPORT_SYMBOL_GPL(rds_connect_complete);
 
 /*
-- 
1.7.1

Re: [PATCH net 3/3] RDS: TCP: fix race windows in send-path quiescence by rds_tcp_accept_one()

[Santosh Shilimkar]

On 6/4/2016 2:00 PM, Sowmini Varadhan wrote:
> The send path needs to be quiesced before resetting callbacks from
> rds_tcp_accept_one(), and commit eb192840266f ("RDS:TCP: Synchronize
> rds_tcp_accept_one with rds_send_xmit when resetting t_sock") achieves
> this using the c_state and RDS_IN_XMIT bit following the pattern
> used by rds_conn_shutdown(). However this leaves the possibility
> of a race window as shown in the sequence below
>     take t_conn_lock in rds_tcp_conn_connect
>     send outgoing syn to peer
>     drop t_conn_lock in rds_tcp_conn_connect
>     incoming from peer triggers rds_tcp_accept_one, conn is
> 	marked CONNECTING
>     wait for RDS_IN_XMIT to quiesce any rds_send_xmit threads
>     call rds_tcp_reset_callbacks
>     [.. race-window where incoming syn-ack can cause the conn
> 	to be marked UP from rds_tcp_state_change ..]
>     lock_sock called from rds_tcp_reset_callbacks, and we set
> 	t_sock to null
> As soon as the conn is marked UP in the race-window above, rds_send_xmit()
> threads will proceed to rds_tcp_xmit and may encounter a null-pointer
> deref on the t_sock.
>
> Given that rds_tcp_state_change() is invoked in softirq context, whereas
> rds_tcp_reset_callbacks() is in workq context, and testing for RDS_IN_XMIT
> after lock_sock could result in a deadlock with tcp_sendmsg, this
> commit fixes the race by using a new c_state, RDS_TCP_RESETTING, which
> will prevent a transition to RDS_CONN_UP from rds_tcp_state_change().
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
As we discussed off-list, for immediate fix, this patch is fine.
Dual sync/ack issue we have 'is_outgoing' and now 'RDS_TCP_RESETTING'
so will be good to address that later.

Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>

Re: [PATCH net 0/3] RDS: TCP: socket locking RDS packet assembly fixes

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sat,  4 Jun 2016 13:59:57 -0700

> This three part patchset fixes bugs in synchronization between
> rds_tcp_accept_one() and the rds-tcp send/recv path.
> 
> Patch 1 ensures that the lock_sock() is taken appropriately
> and the RDS datagram reassembly state is reset  to synchronize
> with the receive path.
> 
> Patch 2 ensures that partially sent RDS datagrams will get
> retransmitted after rds_tcp_accept_one() switches sockets.
> 
> Patch 3 fixes a race window which would prematurely re-enable
> rds_send_xmit() before the rds_tcp_connection setup has been
> completed in rds_tcp_accept_one().

Series applied, thank you.