[PATCH net] vsock: Ignore signal/timeout on connect() if already established

[PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Michal Luczaj]

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: d021c344051a ("VSOCK: Introduce VM Sockets")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Note that this patch does not tackle related problems described in
https://lore.kernel.org/netdev/70371863-fa71-48e0-a1e5-fee83e7ca37c@rbox.co/
---
 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 <mhal@rbox.co>

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Stefano Garzarella]

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/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: d021c344051a ("VSOCK: Introduce VM Sockets")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
>Note that this patch does not tackle related problems described in
>https://lore.kernel.org/netdev/70371863-fa71-48e0-a1e5-fee83e7ca37c@rbox.co/

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

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Michal Luczaj]

On 11/18/25 10:51, Stefano Garzarella wrote:
> On Mon, Nov 17, 2025 at 09:57:25PM +0100, Michal Luczaj wrote:
>> ...
>> +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?

Sure, no problem.

But I've realized invoking `sock_intr_errno(timeout)` is unnecessary.
`timeout` can't be MAX_SCHEDULE_TIMEOUT, so the call always evaluates to
-EINTR, right?

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Stefano Garzarella]

On Tue, Nov 18, 2025 at 11:02:17PM +0100, Michal Luczaj wrote:
>On 11/18/25 10:51, Stefano Garzarella wrote:
>> On Mon, Nov 17, 2025 at 09:57:25PM +0100, Michal Luczaj wrote:
>>> ...
>>> +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?
>
>Sure, no problem.
>
>But I've realized invoking `sock_intr_errno(timeout)` is unnecessary.
>`timeout` can't be MAX_SCHEDULE_TIMEOUT, so the call always evaluates to
>-EINTR, right?

IIUC currently schedule_timeout() can return MAX_SCHEDULE_TIMEOUT only 
if it was called with that parameter, and I think we never call it in 
that way, so I'd agree with you.

My only concern is if it's true for all the stable branches we will 
backport this patch.

I would probably touch it as little as possible and continue using 
sock_intr_errno() for now, but if you verify that it has always been 
that way, then it's fine to change it.

Thanks,
Stefano

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Michal Luczaj]

On 11/19/25 11:36, Stefano Garzarella wrote:
> On Tue, Nov 18, 2025 at 11:02:17PM +0100, Michal Luczaj wrote:
>> On 11/18/25 10:51, Stefano Garzarella wrote:
>>> On Mon, Nov 17, 2025 at 09:57:25PM +0100, Michal Luczaj wrote:
>>>> ...
>>>> +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?
>>
>> Sure, no problem.
>>
>> But I've realized invoking `sock_intr_errno(timeout)` is unnecessary.
>> `timeout` can't be MAX_SCHEDULE_TIMEOUT, so the call always evaluates to
>> -EINTR, right?
> 
> IIUC currently schedule_timeout() can return MAX_SCHEDULE_TIMEOUT only 
> if it was called with that parameter, and I think we never call it in 
> that way, so I'd agree with you.
> 
> My only concern is if it's true for all the stable branches we will 
> backport this patch.
> 
> I would probably touch it as little as possible and continue using 
> sock_intr_errno() for now, but if you verify that it has always been 
> that way, then it's fine to change it.

All right then, here's a v2 with minimum changes:
https://lore.kernel.org/netdev/20251119-vsock-interrupted-connect-v2-1-70734cf1233f@rbox.co/

Note a detail though: should signal and timeout happen at the same time,
now it's the timeout errno returned.

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Stefano Garzarella]

On Wed, Nov 19, 2025 at 03:09:25PM +0100, Michal Luczaj wrote:
>On 11/19/25 11:36, Stefano Garzarella wrote:
>> On Tue, Nov 18, 2025 at 11:02:17PM +0100, Michal Luczaj wrote:
>>> On 11/18/25 10:51, Stefano Garzarella wrote:
>>>> On Mon, Nov 17, 2025 at 09:57:25PM +0100, Michal Luczaj wrote:
>>>>> ...
>>>>> +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?
>>>
>>> Sure, no problem.
>>>
>>> But I've realized invoking `sock_intr_errno(timeout)` is unnecessary.
>>> `timeout` can't be MAX_SCHEDULE_TIMEOUT, so the call always evaluates to
>>> -EINTR, right?
>>
>> IIUC currently schedule_timeout() can return MAX_SCHEDULE_TIMEOUT only
>> if it was called with that parameter, and I think we never call it in
>> that way, so I'd agree with you.
>>
>> My only concern is if it's true for all the stable branches we will
>> backport this patch.
>>
>> I would probably touch it as little as possible and continue using
>> sock_intr_errno() for now, but if you verify that it has always been
>> that way, then it's fine to change it.
>
>All right then, here's a v2 with minimum changes:
>https://lore.kernel.org/netdev/20251119-vsock-interrupted-connect-v2-1-70734cf1233f@rbox.co/
>

Thanks!

>Note a detail though: should signal and timeout happen at the same time,
>now it's the timeout errno returned.
>

Yeah, I thought about that, but I don't see any problems with that.
I mean, it's something that if it happens, it's still not deterministic,
so we're not really changing anything.

Thanks,
Stefano

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Michal Luczaj]

On 11/19/25 17:31, Stefano Garzarella wrote:
> On Wed, Nov 19, 2025 at 03:09:25PM +0100, Michal Luczaj wrote:
>> On 11/19/25 11:36, Stefano Garzarella wrote:
>>> On Tue, Nov 18, 2025 at 11:02:17PM +0100, Michal Luczaj wrote:
>>>> On 11/18/25 10:51, Stefano Garzarella wrote:
>>>>> On Mon, Nov 17, 2025 at 09:57:25PM +0100, Michal Luczaj wrote:
>>>>>> ...
>>>>>> +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?
>>>>
>>>> Sure, no problem.
>>>>
>>>> But I've realized invoking `sock_intr_errno(timeout)` is unnecessary.
>>>> `timeout` can't be MAX_SCHEDULE_TIMEOUT, so the call always evaluates to
>>>> -EINTR, right?
>>>
>>> IIUC currently schedule_timeout() can return MAX_SCHEDULE_TIMEOUT only
>>> if it was called with that parameter, and I think we never call it in
>>> that way, so I'd agree with you.
>>>
>>> My only concern is if it's true for all the stable branches we will
>>> backport this patch.
>>>
>>> I would probably touch it as little as possible and continue using
>>> sock_intr_errno() for now, but if you verify that it has always been
>>> that way, then it's fine to change it.
>>
>> All right then, here's a v2 with minimum changes:
>> https://lore.kernel.org/netdev/20251119-vsock-interrupted-connect-v2-1-70734cf1233f@rbox.co/
>>
> 
> Thanks!
> 
>> Note a detail though: should signal and timeout happen at the same time,
>> now it's the timeout errno returned.
>>
> 
> Yeah, I thought about that, but I don't see any problems with that.
> I mean, it's something that if it happens, it's still not deterministic,
> so we're not really changing anything.

Mhm, I suppose.

To follow up, should I add a version of syzkaller's lockdep warning repro
to vsock test suite? In theory it could test this fix here as well, but in
practice the race window is small and hitting it (the brute way) takes
prohibitively long.

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Michal Luczaj]

On 11/19/25 20:52, Michal Luczaj wrote:
> ...
> To follow up, should I add a version of syzkaller's lockdep warning repro
> to vsock test suite? In theory it could test this fix here as well, but in
> practice the race window is small and hitting it (the brute way) takes
> prohibitively long.

Replying to self to add more data.

After reverting

f7c877e75352 ("vsock: fix lock inversion in vsock_assign_transport()")
002541ef650b ("vsock: Ignore signal/timeout on connect() if already
established")

adding

--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -2014,6 +2014,7 @@ static void test_stream_transport_change_client(const
struct test_opts *opts)
                        perror("socket");
                        exit(EXIT_FAILURE);
                }
+               enable_so_linger(s, 1);

                ret = connect(s, (struct sockaddr *)&sa, sizeof(sa));
                /* The connect can fail due to signals coming from the

is enough for vsock_test to trigger the lockdep warning syzkaller found.

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Stefano Garzarella]

On Thu, Nov 20, 2025 at 10:12:20PM +0100, Michal Luczaj wrote:
>On 11/19/25 20:52, Michal Luczaj wrote:
>> ...
>> To follow up, should I add a version of syzkaller's lockdep warning repro
>> to vsock test suite? In theory it could test this fix here as well, but in
>> practice the race window is small and hitting it (the brute way) takes
>> prohibitively long.
>
>Replying to self to add more data.
>
>After reverting
>
>f7c877e75352 ("vsock: fix lock inversion in vsock_assign_transport()")
>002541ef650b ("vsock: Ignore signal/timeout on connect() if already
>established")
>
>adding
>
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -2014,6 +2014,7 @@ static void test_stream_transport_change_client(const
>struct test_opts *opts)
>                        perror("socket");
>                        exit(EXIT_FAILURE);
>                }
>+               enable_so_linger(s, 1);
>
>                ret = connect(s, (struct sockaddr *)&sa, sizeof(sa));
>                /* The connect can fail due to signals coming from the
>
>is enough for vsock_test to trigger the lockdep warning syzkaller found.
>

cool, so if it's only that, maybe is worth adding.

Thanks,
Stefano

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Michal Luczaj]

On 11/21/25 10:21, Stefano Garzarella wrote:
> On Thu, Nov 20, 2025 at 10:12:20PM +0100, Michal Luczaj wrote:
>> On 11/19/25 20:52, Michal Luczaj wrote:
>>> ...
>>> To follow up, should I add a version of syzkaller's lockdep warning repro
>>> to vsock test suite? In theory it could test this fix here as well, but in
>>> practice the race window is small and hitting it (the brute way) takes
>>> prohibitively long.
>>
>> Replying to self to add more data.
>>
>> After reverting
>>
>> f7c877e75352 ("vsock: fix lock inversion in vsock_assign_transport()")
>> 002541ef650b ("vsock: Ignore signal/timeout on connect() if already
>> established")
>>
>> adding
>>
>> --- a/tools/testing/vsock/vsock_test.c
>> +++ b/tools/testing/vsock/vsock_test.c
>> @@ -2014,6 +2014,7 @@ static void test_stream_transport_change_client(const
>> struct test_opts *opts)
>>                        perror("socket");
>>                        exit(EXIT_FAILURE);
>>                }
>> +               enable_so_linger(s, 1);
>>
>>                ret = connect(s, (struct sockaddr *)&sa, sizeof(sa));
>>                /* The connect can fail due to signals coming from the
>>
>> is enough for vsock_test to trigger the lockdep warning syzkaller found.
>>
> 
> cool, so if it's only that, maybe is worth adding.

Ok, there it is:
https://lore.kernel.org/netdev/20251123-vsock_test-linger-lockdep-warn-v1-1-4b1edf9d8cdc@rbox.co/

And circling back to [1], let me know if you think it's worth adding to the
suit. I guess it would test the case #2 from [2], but it'd take another 2s
and would require both h2g and non-h2g transports enabled.

[1]:
https://lore.kernel.org/netdev/fjy4jaww6xualdudevfuyoavnrbu45cg4d7erv4rttde363xfc@nahglijbl2eg/
[2]:
https://lore.kernel.org/netdev/20251119-vsock-interrupted-connect-v2-1-70734cf1233f@rbox.co/

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Stefano Garzarella]

On Sun, Nov 23, 2025 at 10:46:22PM +0100, Michal Luczaj wrote:
>On 11/21/25 10:21, Stefano Garzarella wrote:
>> On Thu, Nov 20, 2025 at 10:12:20PM +0100, Michal Luczaj wrote:
>>> On 11/19/25 20:52, Michal Luczaj wrote:
>>>> ...
>>>> To follow up, should I add a version of syzkaller's lockdep warning repro
>>>> to vsock test suite? In theory it could test this fix here as well, but in
>>>> practice the race window is small and hitting it (the brute way) takes
>>>> prohibitively long.
>>>
>>> Replying to self to add more data.
>>>
>>> After reverting
>>>
>>> f7c877e75352 ("vsock: fix lock inversion in vsock_assign_transport()")
>>> 002541ef650b ("vsock: Ignore signal/timeout on connect() if already
>>> established")
>>>
>>> adding
>>>
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -2014,6 +2014,7 @@ static void test_stream_transport_change_client(const
>>> struct test_opts *opts)
>>>                        perror("socket");
>>>                        exit(EXIT_FAILURE);
>>>                }
>>> +               enable_so_linger(s, 1);
>>>
>>>                ret = connect(s, (struct sockaddr *)&sa, sizeof(sa));
>>>                /* The connect can fail due to signals coming from the
>>>
>>> is enough for vsock_test to trigger the lockdep warning syzkaller found.
>>>
>>
>> cool, so if it's only that, maybe is worth adding.
>
>Ok, there it is:
>https://lore.kernel.org/netdev/20251123-vsock_test-linger-lockdep-warn-v1-1-4b1edf9d8cdc@rbox.co/

Great!

>
>And circling back to [1], let me know if you think it's worth adding to the
>suit. I guess it would test the case #2 from [2], but it'd take another 2s

If you think it is better to put them in vsock tests, instead of bpf,
it's fine by me. 2s more is okay IMO.

>and would require both h2g and non-h2g transports enabled.

This should be fine, IIRC we recently added something to check
transports and print warninng or skip tests in that cases.

Thanks,
Stefano

>
>[1]:
>https://lore.kernel.org/netdev/fjy4jaww6xualdudevfuyoavnrbu45cg4d7erv4rttde363xfc@nahglijbl2eg/
>[2]:
>https://lore.kernel.org/netdev/20251119-vsock-interrupted-connect-v2-1-70734cf1233f@rbox.co/
>

Re: [PATCH net] vsock: Ignore signal/timeout on connect() if already established

[Michal Luczaj]

On 11/24/25 12:49, Stefano Garzarella wrote:
> On Sun, Nov 23, 2025 at 10:46:22PM +0100, Michal Luczaj wrote:
>> On 11/21/25 10:21, Stefano Garzarella wrote:
>>> On Thu, Nov 20, 2025 at 10:12:20PM +0100, Michal Luczaj wrote:
>>>> On 11/19/25 20:52, Michal Luczaj wrote:
>>>>> ...
>>>>> To follow up, should I add a version of syzkaller's lockdep warning repro
>>>>> to vsock test suite? In theory it could test this fix here as well, but in
>>>>> practice the race window is small and hitting it (the brute way) takes
>>>>> prohibitively long.
>>>>
>>>> Replying to self to add more data.
>>>>
>>>> After reverting
>>>>
>>>> f7c877e75352 ("vsock: fix lock inversion in vsock_assign_transport()")
>>>> 002541ef650b ("vsock: Ignore signal/timeout on connect() if already
>>>> established")
>>>>
>>>> adding
>>>>
>>>> --- a/tools/testing/vsock/vsock_test.c
>>>> +++ b/tools/testing/vsock/vsock_test.c
>>>> @@ -2014,6 +2014,7 @@ static void test_stream_transport_change_client(const
>>>> struct test_opts *opts)
>>>>                        perror("socket");
>>>>                        exit(EXIT_FAILURE);
>>>>                }
>>>> +               enable_so_linger(s, 1);
>>>>
>>>>                ret = connect(s, (struct sockaddr *)&sa, sizeof(sa));
>>>>                /* The connect can fail due to signals coming from the
>>>>
>>>> is enough for vsock_test to trigger the lockdep warning syzkaller found.
>>>>
>>>
>>> cool, so if it's only that, maybe is worth adding.
>>
>> Ok, there it is:
>> https://lore.kernel.org/netdev/20251123-vsock_test-linger-lockdep-warn-v1-1-4b1edf9d8cdc@rbox.co/
> 
> Great!
> 
>>
>> And circling back to [1], let me know if you think it's worth adding to the
>> suit. I guess it would test the case #2 from [2], but it'd take another 2s
> 
> If you think it is better to put them in vsock tests, instead of bpf,
> it's fine by me. 2s more is okay IMO.

OK, here's a followup:
https://lore.kernel.org/bpf/20260123-selftest-signal-on-connect-v1-0-b0256e7025b6@rbox.co/

[...]

>> [1]:
>> https://lore.kernel.org/netdev/fjy4jaww6xualdudevfuyoavnrbu45cg4d7erv4rttde363xfc@nahglijbl2eg/
>> [2]:
>> https://lore.kernel.org/netdev/20251119-vsock-interrupted-connect-v2-1-70734cf1233f@rbox.co/