Skip to content

Commit 002541e

Browse files
mmhalkuba-moo
authored andcommitted
vsock: Ignore signal/timeout on connect() if already established
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/e07fd95c-9a38-4eea-9638-133e38c2ec9b@rbox.co/ [2]: https://lore.kernel.org/netdev/20250317-vsock-trans-signal-race-v4-0-fc8837f3f1d4@rbox.co/ [3]: https://lore.kernel.org/netdev/60f1b7db-3099-4f6a-875e-af9f6ef194f6@rbox.co/ Fixes: d021c34 ("VSOCK: Introduce VM Sockets") Signed-off-by: Michal Luczaj <mhal@rbox.co> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Link: https://patch.msgid.link/20251119-vsock-interrupted-connect-v2-1-70734cf1233f@rbox.co Signed-off-by: Jakub Kicinski <kuba@kernel.org>
1 parent 7d277a7 commit 002541e

1 file changed

Lines changed: 31 additions & 9 deletions

File tree

net/vmw_vsock/af_vsock.c

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1661,18 +1661,40 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,
16611661
timeout = schedule_timeout(timeout);
16621662
lock_sock(sk);
16631663

1664-
if (signal_pending(current)) {
1665-
err = sock_intr_errno(timeout);
1666-
sk->sk_state = sk->sk_state == TCP_ESTABLISHED ? TCP_CLOSING : TCP_CLOSE;
1667-
sock->state = SS_UNCONNECTED;
1668-
vsock_transport_cancel_pkt(vsk);
1669-
vsock_remove_connected(vsk);
1670-
goto out_wait;
1671-
} else if ((sk->sk_state != TCP_ESTABLISHED) && (timeout == 0)) {
1672-
err = -ETIMEDOUT;
1664+
/* Connection established. Whatever happens to socket once we
1665+
* release it, that's not connect()'s concern. No need to go
1666+
* into signal and timeout handling. Call it a day.
1667+
*
1668+
* Note that allowing to "reset" an already established socket
1669+
* here is racy and insecure.
1670+
*/
1671+
if (sk->sk_state == TCP_ESTABLISHED)
1672+
break;
1673+
1674+
/* If connection was _not_ established and a signal/timeout came
1675+
* to be, we want the socket's state reset. User space may want
1676+
* to retry.
1677+
*
1678+
* sk_state != TCP_ESTABLISHED implies that socket is not on
1679+
* vsock_connected_table. We keep the binding and the transport
1680+
* assigned.
1681+
*/
1682+
if (signal_pending(current) || timeout == 0) {
1683+
err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);
1684+
1685+
/* Listener might have already responded with
1686+
* VIRTIO_VSOCK_OP_RESPONSE. Its handling expects our
1687+
* sk_state == TCP_SYN_SENT, which hereby we break.
1688+
* In such case VIRTIO_VSOCK_OP_RST will follow.
1689+
*/
16731690
sk->sk_state = TCP_CLOSE;
16741691
sock->state = SS_UNCONNECTED;
1692+
1693+
/* Try to cancel VIRTIO_VSOCK_OP_REQUEST skb sent out by
1694+
* transport->connect().
1695+
*/
16751696
vsock_transport_cancel_pkt(vsk);
1697+
16761698
goto out_wait;
16771699
}
16781700

0 commit comments

Comments
 (0)