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?
