On 9/24/26 15:15, [email protected] wrote:
>> +static void vsock_unconnected_reset(struct sock *sk)
>> +{
>> + struct vsock_sock *vsk = vsock_sk(sk);
>> +
>> + sock_owned_by_me(sk);
>> +
>> + /*
>> + * Only connected socks may have peer_shutdown or SOCK_DONE set.
>> + *
>> + * Once established (TCP_ESTABLISHED, TCP_CLOSING), a socket can be
>> + * de-assigned only on close(). But we can narrow the check down to
>> + * states we actually expect (TCP_SYN_SENT, TCP_CLOSE).
>> + */
>> + if (WARN_ON_ONCE(vsk->peer_shutdown) ||
>> + WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
>> + WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
>> + sk->sk_state != TCP_CLOSE))
>> + return;
>
> [Severity: High]
> Is the "only connected socks may have peer_shutdown or SOCK_DONE set"
> invariant true for VMCI?
>
> vmci_transport_recv_connecting_client_negotiate() allocates the queue pair
> and subscribes to VMCI_EVENT_QP_PEER_DETACH while the socket is still in
> TCP_SYN_SENT. If the peer then detaches, vmci_transport_handle_detach()
> runs on a never-established socket:
>
> The blocking connect() waiter then wakes on sk_err, err = sock_error(sk)
> becomes -ECONNRESET, and out_reset calls this helper with peer_shutdown ==
> SHUTDOWN_MASK and SOCK_DONE set.
>
> Since WARN_ON_ONCE() evaluates to the condition, the helper splats and
> returns before vsock_transport_cancel_pkt(), vsock_deassign_transport() and
> the TCP_CLOSE/SS_UNCONNECTED restore. Can that leave the socket stuck with
> sk_socket->state == SS_CONNECTING and the transport (plus its module
> reference) pinned, so a later connect() returns -EALREADY for O_NONBLOCK or
> re-hits the same early return, and listen() returns -EINVAL?
>
> Before this patch the same path unconditionally ran:
>
> err = sock_error(sk);
> if (err) {
> sk->sk_state = TCP_CLOSE;
> sock->state = SS_UNCONNECTED;
> }
>
> so the socket stayed retryable. Is the change in behaviour here intended?
True, I'll drop WARN_ON_ONCE(vsk->peer_shutdown).
> On panic_on_warn=1 kernels this is also reachable from an unprivileged
> connect() when the peer VM goes away mid-handshake.
It was meant for debugging and to trip the fuzzers. Would it be better to
use DEBUG_NET_WARN_ON_ONCE?
> [Severity: High]
> Can this de-assign the transport of a socket that did reach
> TCP_ESTABLISHED?
> ...
> A later patch in this series, "vsock: handle socket bricking due to
> transport event", adds an early return on sk_socket->state == SS_CONNECTED
> or SS_DISCONNECTING at the top of this helper, which covers exactly this
> case, so this only affects this commit taken on its own. Would it be
> cleaner to fold that guard in here?
Maybe it would be cleaner, but I think it's easier to review as it is now.
> [Severity: Medium]
> For a non-blocking connect() whose failure is detected asynchronously by
> the transport, which path reaches out_reset?
>
> connect(fd, O_NONBLOCK) assigns a transport, sets sk_state = TCP_SYN_SENT,
> sock->state = SS_CONNECTING and returns -EINPROGRESS. If the peer answers
> with VIRTIO_VSOCK_OP_RST, virtio_transport_recv_connecting() takes its
> destroy label:
>
> net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_connecting() {
> ...
> destroy:
> virtio_transport_reset(vsk, skb);
> sk->sk_state = TCP_CLOSE;
> sk->sk_err = skerr;
> sk_error_report(sk);
> ...
> }
>
> sk_socket->state is left at SS_CONNECTING and the transport stays assigned.
> vmci_transport_recv_connecting_client() has the same pattern.
>
> vsock_connect_timeout() above is gated on sk->sk_state == TCP_SYN_SENT, so
> it does not reset either once the state is TCP_CLOSE. And a retry with
> O_NONBLOCK hits:
>
> case SS_CONNECTING:
> ...
> err = -EALREADY;
> if (flags & O_NONBLOCK)
> goto out;
>
> which returns before out_reset.
>
> Does the socket then keep the transport assignment and the transport module
> reference until close()? The changelog says:
>
> If connection fails (init went wrong, peer misbehaviour, time out,
> signal), transport is de-assigned and socket state is re-initialized.
>
> Should the asynchronous peer-misbehaviour case for a non-blocking connect()
> be covered as well, or should the changelog be narrowed to the synchronous
> paths?
OK, so a _non-blocking_ connect() getting hit with RST before connection
can be established: fd bricked, you can't re-connect() with it. While you
can retry a _blocking_ connect getting hit the same way. And the choice is
to unify the behaviour or document the discrepancy, if I get it right.
