During connect(), acting on a signal/timeout by disconnecting an already established socket leads to several issues:
1. connect() invoking vsock_transport_cancel_pkt() -> virtio_transport_purge_skbs() may race with sendmsg() invoking virtio_transport_get_credit(). This results in a permanently elevated `vvs->bytes_unsent`. Which, in turn, confuses the SOCK_LINGER handling. 2. connect() resetting a connected socket's state may race with socket being placed in a sockmap. A disconnected socket remaining in a sockmap breaks sockmap's assumptions. And gives rise to WARNs. 3. connect() transitioning SS_CONNECTED -> SS_UNCONNECTED allows for a transport change/drop after TCP_ESTABLISHED. Which poses a problem for any simultaneous sendmsg() or connect() and may result in a use-after-free/null-ptr-deref. Do not disconnect socket on signal/timeout. Keep the logic for unconnected sockets: they don't linger, can't be placed in a sockmap, are rejected by sendmsg(). [1]: https://lore.kernel.org/netdev/[email protected]/ [2]: https://lore.kernel.org/netdev/[email protected]/ [3]: https://lore.kernel.org/netdev/[email protected]/ Fixes: d021c344051a ("VSOCK: Introduce VM Sockets") Signed-off-by: Michal Luczaj <[email protected]> --- Note that this patch does not tackle related problems described in https://lore.kernel.org/netdev/[email protected]/ --- net/vmw_vsock/af_vsock.c | 48 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index 76763247a377..a52e7dbe7878 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -1528,6 +1528,23 @@ static void vsock_connect_timeout(struct work_struct *work) sock_put(sk); } +static void vsock_reset_interrupted(struct sock *sk) +{ + struct vsock_sock *vsk = vsock_sk(sk); + + /* Try to cancel VIRTIO_VSOCK_OP_REQUEST skb sent out by + * transport->connect(). + */ + vsock_transport_cancel_pkt(vsk); + + /* Listener might have already responded with VIRTIO_VSOCK_OP_RESPONSE. + * Its handling expects our sk_state == TCP_SYN_SENT, which hereby we + * break. In such case VIRTIO_VSOCK_OP_RST will follow. + */ + sk->sk_state = TCP_CLOSE; + sk->sk_socket->state = SS_UNCONNECTED; +} + static int vsock_connect(struct socket *sock, struct sockaddr *addr, int addr_len, int flags) { @@ -1661,18 +1678,33 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr, timeout = schedule_timeout(timeout); lock_sock(sk); + /* Connection established. Whatever happens to socket once we + * release it, that's not connect()'s concern. No need to go + * into signal and timeout handling. Call it a day. + * + * Note that allowing to "reset" an already established socket + * here is racy and insecure. + */ + if (sk->sk_state == TCP_ESTABLISHED) + break; + + /* If connection was _not_ established and a signal/timeout came + * to be, we want the socket's state reset. User space may want + * to retry. + * + * sk_state != TCP_ESTABLISHED implies that socket is not on + * vsock_connected_table. We keep the binding and the transport + * assigned. + */ if (signal_pending(current)) { err = sock_intr_errno(timeout); - sk->sk_state = sk->sk_state == TCP_ESTABLISHED ? TCP_CLOSING : TCP_CLOSE; - sock->state = SS_UNCONNECTED; - vsock_transport_cancel_pkt(vsk); - vsock_remove_connected(vsk); + vsock_reset_interrupted(sk); goto out_wait; - } else if ((sk->sk_state != TCP_ESTABLISHED) && (timeout == 0)) { + } + + if (timeout == 0) { err = -ETIMEDOUT; - sk->sk_state = TCP_CLOSE; - sock->state = SS_UNCONNECTED; - vsock_transport_cancel_pkt(vsk); + vsock_reset_interrupted(sk); goto out_wait; } --- base-commit: 5442a9da69789741bfda39f34ee7f69552bf0c56 change-id: 20250815-vsock-interrupted-connect-f92dfa5042cd Best regards, -- Michal Luczaj <[email protected]>
