On Mon, Nov 17, 2025 at 09:57:25PM +0100, Michal Luczaj wrote:
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]/
Ooops, it seems I forgot to reply. Thanks for bringing this to my
attention agan. Next time feel free to ping me :-)
I'll reply in that thread.
---
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;
I'm fine with the change, but now both code blocks are the same, so
can we unify them?
I mean something like this:
if (signal_pending(current) || timeout == 0 {
err = timeout == 0 ? -ETIMEDOUT :
sock_intr_errno(timeout);
...
}
Maybe at that point we can also remove the vsock_reset_interrupted()
function and put the code right there.
BTW I don't have a strong opinion, what do you prefer?
Thanks,
Stefano
