| Message ID | 20211029200421.65090-3-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] SUNRPC: Fix races when closing the socket | expand |
On Fri, Oct 29, 2021 at 4:11 PM <trondmy@kernel.org> wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > Move the error handling into a single switch statement for clarity. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > net/sunrpc/xprtsock.c | 68 ++++++++++++++++++------------------------- > 1 file changed, 28 insertions(+), 40 deletions(-) > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > index 291b63136c08..7fb302e202bc 100644 > --- a/net/sunrpc/xprtsock.c > +++ b/net/sunrpc/xprtsock.c > @@ -2159,7 +2159,6 @@ static void xs_tcp_set_connect_timeout(struct rpc_xprt *xprt, > static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) > { > struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt); > - int ret = -ENOTCONN; > > if (!transport->inet) { > struct sock *sk = sock->sk; > @@ -2203,7 +2202,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) > } > > if (!xprt_bound(xprt)) > - goto out; > + return -ENOTCONN; > > xs_set_memalloc(xprt); > > @@ -2211,22 +2210,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) > > /* Tell the socket layer to start connecting... */ > set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state); > - ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK); > - switch (ret) { > - case 0: > - xs_set_srcport(transport, sock); > - fallthrough; > - case -EINPROGRESS: > - /* SYN_SENT! */ > - if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO) > - xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO; > - break; > - case -EADDRNOTAVAIL: > - /* Source port number is unavailable. Try a new one! */ > - transport->srcport = 0; > - } > -out: > - return ret; > + return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK); > } > > /** > @@ -2241,14 +2225,14 @@ static void xs_tcp_setup_socket(struct work_struct *work) > container_of(work, struct sock_xprt, connect_worker.work); > struct socket *sock = transport->sock; > struct rpc_xprt *xprt = &transport->xprt; > - int status = -EIO; > + int status; > > if (!sock) { > sock = xs_create_sock(xprt, transport, > xs_addr(xprt)->sa_family, SOCK_STREAM, > IPPROTO_TCP, true); > if (IS_ERR(sock)) { > - status = PTR_ERR(sock); > + xprt_wake_pending_tasks(xprt, PTR_ERR(sock)); > goto out; > } > } > @@ -2265,21 +2249,21 @@ static void xs_tcp_setup_socket(struct work_struct *work) > xprt, -status, xprt_connected(xprt), > sock->sk->sk_state); > switch (status) { > - default: > - printk("%s: connect returned unhandled error %d\n", > - __func__, status); > - fallthrough; > - case -EADDRNOTAVAIL: > - /* We're probably in TIME_WAIT. Get rid of existing socket, > - * and retry > - */ > - xs_tcp_force_close(xprt); > - break; > case 0: > + xs_set_srcport(transport, sock); > + fallthrough; > case -EINPROGRESS: > + /* SYN_SENT! */ > + if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO) > + xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO; > + fallthrough; > case -EALREADY: > - xprt_unlock_connect(xprt, transport); > - return; > + goto out_unlock; > + case -EADDRNOTAVAIL: > + /* Source port number is unavailable. Try a new one! */ > + transport->srcport = 0; > + status = -EAGAIN; > + break; > case -EINVAL: > /* Happens, for instance, if the user specified a link > * local IPv6 address without a scope-id. > @@ -2291,18 +2275,22 @@ static void xs_tcp_setup_socket(struct work_struct *work) > case -EHOSTUNREACH: > case -EADDRINUSE: > case -ENOBUFS: > - /* xs_tcp_force_close() wakes tasks with a fixed error code. > - * We need to wake them first to ensure the correct error code. > - */ > - xprt_wake_pending_tasks(xprt, status); > - xs_tcp_force_close(xprt); > - goto out; > + break; > + default: > + printk("%s: connect returned unhandled error %d\n", > + __func__, status); > + status = -EAGAIN; > } > - status = -EAGAIN; > + > + /* xs_tcp_force_close() wakes tasks with a fixed error code. > + * We need to wake them first to ensure the correct error code. > + */ > + xprt_wake_pending_tasks(xprt, status); > + xs_tcp_force_close(xprt); Isn't this a problem to call xs_tcp_force_close() unconditionally at the bottom of xs_tcp_setup_socket()? Before this patch we only called this depending on certain returns from kernel_connect(). > out: > xprt_clear_connecting(xprt); > +out_unlock: > xprt_unlock_connect(xprt, transport); > - xprt_wake_pending_tasks(xprt, status); > } > > /** > -- > 2.31.1 >
On Fri, Dec 3, 2021 at 11:10 AM David Wysochanski <dwysocha@redhat.com> wrote: > > On Fri, Oct 29, 2021 at 4:11 PM <trondmy@kernel.org> wrote: > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > Move the error handling into a single switch statement for clarity. > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > net/sunrpc/xprtsock.c | 68 ++++++++++++++++++------------------------- > > 1 file changed, 28 insertions(+), 40 deletions(-) > > > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > > index 291b63136c08..7fb302e202bc 100644 > > --- a/net/sunrpc/xprtsock.c > > +++ b/net/sunrpc/xprtsock.c > > @@ -2159,7 +2159,6 @@ static void xs_tcp_set_connect_timeout(struct rpc_xprt *xprt, > > static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) > > { > > struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt); > > - int ret = -ENOTCONN; > > > > if (!transport->inet) { > > struct sock *sk = sock->sk; > > @@ -2203,7 +2202,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) > > } > > > > if (!xprt_bound(xprt)) > > - goto out; > > + return -ENOTCONN; > > > > xs_set_memalloc(xprt); > > > > @@ -2211,22 +2210,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) > > > > /* Tell the socket layer to start connecting... */ > > set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state); > > - ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK); > > - switch (ret) { > > - case 0: > > - xs_set_srcport(transport, sock); > > - fallthrough; > > - case -EINPROGRESS: > > - /* SYN_SENT! */ > > - if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO) > > - xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO; > > - break; > > - case -EADDRNOTAVAIL: > > - /* Source port number is unavailable. Try a new one! */ > > - transport->srcport = 0; > > - } > > -out: > > - return ret; > > + return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK); > > } > > > > /** > > @@ -2241,14 +2225,14 @@ static void xs_tcp_setup_socket(struct work_struct *work) > > container_of(work, struct sock_xprt, connect_worker.work); > > struct socket *sock = transport->sock; > > struct rpc_xprt *xprt = &transport->xprt; > > - int status = -EIO; > > + int status; > > > > if (!sock) { > > sock = xs_create_sock(xprt, transport, > > xs_addr(xprt)->sa_family, SOCK_STREAM, > > IPPROTO_TCP, true); > > if (IS_ERR(sock)) { > > - status = PTR_ERR(sock); > > + xprt_wake_pending_tasks(xprt, PTR_ERR(sock)); > > goto out; > > } > > } > > @@ -2265,21 +2249,21 @@ static void xs_tcp_setup_socket(struct work_struct *work) > > xprt, -status, xprt_connected(xprt), > > sock->sk->sk_state); > > switch (status) { > > - default: > > - printk("%s: connect returned unhandled error %d\n", > > - __func__, status); > > - fallthrough; > > - case -EADDRNOTAVAIL: > > - /* We're probably in TIME_WAIT. Get rid of existing socket, > > - * and retry > > - */ > > - xs_tcp_force_close(xprt); > > - break; > > case 0: > > + xs_set_srcport(transport, sock); > > + fallthrough; > > case -EINPROGRESS: > > + /* SYN_SENT! */ > > + if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO) > > + xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO; > > + fallthrough; > > case -EALREADY: > > - xprt_unlock_connect(xprt, transport); > > - return; > > + goto out_unlock; > > + case -EADDRNOTAVAIL: > > + /* Source port number is unavailable. Try a new one! */ > > + transport->srcport = 0; > > + status = -EAGAIN; > > + break; > > case -EINVAL: > > /* Happens, for instance, if the user specified a link > > * local IPv6 address without a scope-id. > > @@ -2291,18 +2275,22 @@ static void xs_tcp_setup_socket(struct work_struct *work) > > case -EHOSTUNREACH: > > case -EADDRINUSE: > > case -ENOBUFS: > > - /* xs_tcp_force_close() wakes tasks with a fixed error code. > > - * We need to wake them first to ensure the correct error code. > > - */ > > - xprt_wake_pending_tasks(xprt, status); > > - xs_tcp_force_close(xprt); > > - goto out; > > + break; > > + default: > > + printk("%s: connect returned unhandled error %d\n", > > + __func__, status); > > + status = -EAGAIN; > > } > > - status = -EAGAIN; > > + > > + /* xs_tcp_force_close() wakes tasks with a fixed error code. > > + * We need to wake them first to ensure the correct error code. > > + */ > > + xprt_wake_pending_tasks(xprt, status); > > + xs_tcp_force_close(xprt); > > Isn't this a problem to call xs_tcp_force_close() unconditionally at > the bottom of xs_tcp_setup_socket()? > Before this patch we only called this depending on certain returns > from kernel_connect(). > Nevermind - I see the fallthroughs and out_unlock > > > > out: > > xprt_clear_connecting(xprt); > > +out_unlock: > > xprt_unlock_connect(xprt, transport); > > - xprt_wake_pending_tasks(xprt, status); > > } > > > > /** > > -- > > 2.31.1 > >
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index 291b63136c08..7fb302e202bc 100644 --- a/net/sunrpc/xprtsock.c +++ b/net/sunrpc/xprtsock.c @@ -2159,7 +2159,6 @@ static void xs_tcp_set_connect_timeout(struct rpc_xprt *xprt, static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) { struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt); - int ret = -ENOTCONN; if (!transport->inet) { struct sock *sk = sock->sk; @@ -2203,7 +2202,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) } if (!xprt_bound(xprt)) - goto out; + return -ENOTCONN; xs_set_memalloc(xprt); @@ -2211,22 +2210,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) /* Tell the socket layer to start connecting... */ set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state); - ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK); - switch (ret) { - case 0: - xs_set_srcport(transport, sock); - fallthrough; - case -EINPROGRESS: - /* SYN_SENT! */ - if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO) - xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO; - break; - case -EADDRNOTAVAIL: - /* Source port number is unavailable. Try a new one! */ - transport->srcport = 0; - } -out: - return ret; + return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK); } /** @@ -2241,14 +2225,14 @@ static void xs_tcp_setup_socket(struct work_struct *work) container_of(work, struct sock_xprt, connect_worker.work); struct socket *sock = transport->sock; struct rpc_xprt *xprt = &transport->xprt; - int status = -EIO; + int status; if (!sock) { sock = xs_create_sock(xprt, transport, xs_addr(xprt)->sa_family, SOCK_STREAM, IPPROTO_TCP, true); if (IS_ERR(sock)) { - status = PTR_ERR(sock); + xprt_wake_pending_tasks(xprt, PTR_ERR(sock)); goto out; } } @@ -2265,21 +2249,21 @@ static void xs_tcp_setup_socket(struct work_struct *work) xprt, -status, xprt_connected(xprt), sock->sk->sk_state); switch (status) { - default: - printk("%s: connect returned unhandled error %d\n", - __func__, status); - fallthrough; - case -EADDRNOTAVAIL: - /* We're probably in TIME_WAIT. Get rid of existing socket, - * and retry - */ - xs_tcp_force_close(xprt); - break; case 0: + xs_set_srcport(transport, sock); + fallthrough; case -EINPROGRESS: + /* SYN_SENT! */ + if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO) + xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO; + fallthrough; case -EALREADY: - xprt_unlock_connect(xprt, transport); - return; + goto out_unlock; + case -EADDRNOTAVAIL: + /* Source port number is unavailable. Try a new one! */ + transport->srcport = 0; + status = -EAGAIN; + break; case -EINVAL: /* Happens, for instance, if the user specified a link * local IPv6 address without a scope-id. @@ -2291,18 +2275,22 @@ static void xs_tcp_setup_socket(struct work_struct *work) case -EHOSTUNREACH: case -EADDRINUSE: case -ENOBUFS: - /* xs_tcp_force_close() wakes tasks with a fixed error code. - * We need to wake them first to ensure the correct error code. - */ - xprt_wake_pending_tasks(xprt, status); - xs_tcp_force_close(xprt); - goto out; + break; + default: + printk("%s: connect returned unhandled error %d\n", + __func__, status); + status = -EAGAIN; } - status = -EAGAIN; + + /* xs_tcp_force_close() wakes tasks with a fixed error code. + * We need to wake them first to ensure the correct error code. + */ + xprt_wake_pending_tasks(xprt, status); + xs_tcp_force_close(xprt); out: xprt_clear_connecting(xprt); +out_unlock: xprt_unlock_connect(xprt, transport); - xprt_wake_pending_tasks(xprt, status); } /**