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/[email protected]/
Note a detail though: should signal and timeout happen at the same time,
now it's the timeout errno returned.
