On 9/16/26 14:30, Stefano Garzarella wrote:
>> @@ -1834,23 +1833,22 @@ static int vsock_connect(struct socket *sock, struct
>> sockaddr_unsized *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
>> + /* Connection was 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 (sk->sk_state == TCP_ESTABLISHED ||
>> + sk->sk_state == TCP_CLOSING) {
>> + err = 0;
>> + goto out_wait;
>> + }
>>
>> /* 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.
>
> Should we keep "We keep the binding and the transport assigned." or
> maybe add on the other phrase, "User space may want to retry, so we keep
> ..."
Right, I'll extend the first paragraph.
>> */
>> if (signal_pending(current) || timeout == 0) {
>> err = timeout == 0 ? -ETIMEDOUT :
>> sock_intr_errno(timeout);
>> @@ -1874,8 +1872,8 @@ static int vsock_connect(struct socket *sock, struct
>> sockaddr_unsized *addr,
>> prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
>> }
>>
>> - err = sock_error(sk);
>> - if (err) {
>
> Should we add a comment here explaining why we are doing this?
OK, I'll expand on this. With `if (sk->sk_state == TCP_ESTABLISHED) break`,
there's another bug here.
>> + if (sk->sk_state != TCP_ESTABLISHED && sk->sk_state != TCP_CLOSING) {
>> + err = sock_error(sk);
>> sk->sk_state = TCP_CLOSE;
>> sock->state = SS_UNCONNECTED;
>> }
We might exit the loop and get here on:
1. vsock_connect_timeout() timing out; setting sk_err, TCP_CLOSE,
SS_UNCONNECTED
2. virtio_transport_recv_connecting() failing to establish connection;
setting sk_err, TCP_CLOSE, and leaving SS_CONNECTING
And to be precise:
3. established connection getting torn by a transport event, locklessly
setting sk_err, TCP_CLOSE, and leaving SS_CONNECTED
I sure hope I'm not missing something.
So the code above is taking a bit of a defensive approach, but I suppose it
could be simplified to an if()-less
err = sock_error(sk);
sock->state = SS_UNCONNECTED;
thanks,
Michal
