| Message ID | d858d15746eeb224f4fb27554771e14b58f31306.1442411712.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 16, 2015 at 9:58 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > After a network segmentation that times out the socket, we can get stuck in > a loop where the client repeatedly sends a SYN, then a RST, with the result > that the client will never successfully reconnect to the server. > > When there's a need to re-establish a TCP connection, the scheduled > connect_worker does xs_tcp_setup_socket(), which sends a SYN, then > immediately wakes the waiting task to call_connect_status with a status of > EAGAIN, which proceeds to xprt_connect() and kernel_sock_shutdown() which > will send a RST. > > Fix this loop by deferring xprt_clear_connecting() until the TCP state > change of ESTALISHED or CLOSED, thereby ensuring that we do not race into > xs_connect() while waiting for SYN,ACK from the server. > > Fixes: 099392048c (SUNRPC: Prevent SYN+SYNACK+RST storms) > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > net/sunrpc/xprtsock.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > index 7be90bc..6df51df 100644 > --- a/net/sunrpc/xprtsock.c > +++ b/net/sunrpc/xprtsock.c > @@ -1450,6 +1450,7 @@ static void xs_tcp_state_change(struct sock *sk) > switch (sk->sk_state) { > case TCP_ESTABLISHED: > spin_lock(&xprt->transport_lock); > + xprt_clear_connecting(xprt); > if (!xprt_test_and_set_connected(xprt)) { > struct sock_xprt *transport = container_of(xprt, > struct sock_xprt, xprt); > @@ -1496,6 +1497,7 @@ static void xs_tcp_state_change(struct sock *sk) > smp_mb__after_atomic(); > break; > case TCP_CLOSE: > + xprt_clear_connecting(xprt); > xs_sock_mark_closed(xprt); > } > out: > @@ -2237,10 +2239,10 @@ static void xs_tcp_setup_socket(struct work_struct *work) > xs_tcp_force_close(xprt); > break; > case 0: > - case -EINPROGRESS: > + xprt_clear_connecting(xprt); > case -EALREADY: > + case -EINPROGRESS: > xprt_unlock_connect(xprt, transport); > - xprt_clear_connecting(xprt); > return; > case -EINVAL: > /* Happens, for instance, if the user specified a link > This introduces races. The call to xprt_clear_connecting in TCP_CLOSE can trigger pretty much at any time, and so could interfere with a subsequent connection attempt. I have an alternative patch which addresses this problem and which is undergoing testing. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 16 Sep 2015, Trond Myklebust wrote: > On Wed, Sep 16, 2015 at 9:58 AM, Benjamin Coddington > <bcodding@redhat.com> wrote: > > After a network segmentation that times out the socket, we can get stuck in > > a loop where the client repeatedly sends a SYN, then a RST, with the result > > that the client will never successfully reconnect to the server. > > > > When there's a need to re-establish a TCP connection, the scheduled > > connect_worker does xs_tcp_setup_socket(), which sends a SYN, then > > immediately wakes the waiting task to call_connect_status with a status of > > EAGAIN, which proceeds to xprt_connect() and kernel_sock_shutdown() which > > will send a RST. > > > > Fix this loop by deferring xprt_clear_connecting() until the TCP state > > change of ESTALISHED or CLOSED, thereby ensuring that we do not race into > > xs_connect() while waiting for SYN,ACK from the server. > > > > Fixes: 099392048c (SUNRPC: Prevent SYN+SYNACK+RST storms) > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > > --- > > net/sunrpc/xprtsock.c | 6 ++++-- > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > > index 7be90bc..6df51df 100644 > > --- a/net/sunrpc/xprtsock.c > > +++ b/net/sunrpc/xprtsock.c > > @@ -1450,6 +1450,7 @@ static void xs_tcp_state_change(struct sock *sk) > > switch (sk->sk_state) { > > case TCP_ESTABLISHED: > > spin_lock(&xprt->transport_lock); > > + xprt_clear_connecting(xprt); > > if (!xprt_test_and_set_connected(xprt)) { > > struct sock_xprt *transport = container_of(xprt, > > struct sock_xprt, xprt); > > @@ -1496,6 +1497,7 @@ static void xs_tcp_state_change(struct sock *sk) > > smp_mb__after_atomic(); > > break; > > case TCP_CLOSE: > > + xprt_clear_connecting(xprt); > > xs_sock_mark_closed(xprt); > > } > > out: > > @@ -2237,10 +2239,10 @@ static void xs_tcp_setup_socket(struct work_struct *work) > > xs_tcp_force_close(xprt); > > break; > > case 0: > > - case -EINPROGRESS: > > + xprt_clear_connecting(xprt); > > case -EALREADY: > > + case -EINPROGRESS: > > xprt_unlock_connect(xprt, transport); > > - xprt_clear_connecting(xprt); > > return; > > case -EINVAL: > > /* Happens, for instance, if the user specified a link > > > > This introduces races. The call to xprt_clear_connecting in TCP_CLOSE > can trigger pretty much at any time, and so could interfere with a > subsequent connection attempt. Oh, yeah. I see that now. > I have an alternative patch which addresses this problem and which is > undergoing testing. Great! -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index 7be90bc..6df51df 100644 --- a/net/sunrpc/xprtsock.c +++ b/net/sunrpc/xprtsock.c @@ -1450,6 +1450,7 @@ static void xs_tcp_state_change(struct sock *sk) switch (sk->sk_state) { case TCP_ESTABLISHED: spin_lock(&xprt->transport_lock); + xprt_clear_connecting(xprt); if (!xprt_test_and_set_connected(xprt)) { struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt); @@ -1496,6 +1497,7 @@ static void xs_tcp_state_change(struct sock *sk) smp_mb__after_atomic(); break; case TCP_CLOSE: + xprt_clear_connecting(xprt); xs_sock_mark_closed(xprt); } out: @@ -2237,10 +2239,10 @@ static void xs_tcp_setup_socket(struct work_struct *work) xs_tcp_force_close(xprt); break; case 0: - case -EINPROGRESS: + xprt_clear_connecting(xprt); case -EALREADY: + case -EINPROGRESS: xprt_unlock_connect(xprt, transport); - xprt_clear_connecting(xprt); return; case -EINVAL: /* Happens, for instance, if the user specified a link
After a network segmentation that times out the socket, we can get stuck in a loop where the client repeatedly sends a SYN, then a RST, with the result that the client will never successfully reconnect to the server. When there's a need to re-establish a TCP connection, the scheduled connect_worker does xs_tcp_setup_socket(), which sends a SYN, then immediately wakes the waiting task to call_connect_status with a status of EAGAIN, which proceeds to xprt_connect() and kernel_sock_shutdown() which will send a RST. Fix this loop by deferring xprt_clear_connecting() until the TCP state change of ESTALISHED or CLOSED, thereby ensuring that we do not race into xs_connect() while waiting for SYN,ACK from the server. Fixes: 099392048c (SUNRPC: Prevent SYN+SYNACK+RST storms) Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- net/sunrpc/xprtsock.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)