net/vmw_vsock/af_vsock.c | 48 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-)
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>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
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?
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
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.
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
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.
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.
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
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/
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/
>
© 2016 - 2025 Red Hat, Inc.