Re: [PATCH net v3 4/8] vsock: drop the re-assignment logic

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The invariant this patch asserts ("On a failed connect(),
  transport is de-assigned", enforced by the new…
- [Low] Two in-tree comments still justify the SOCK_DEAD gating in
  vsock_remove_sock() by the "transport reassignment" case, which…
- [Low] vsock_assign_transport() gains a hard precondition (the socket
  must have neither ->transport nor ->trans set, otherwise WARN +…

--

vsock: drop the re-assignment logic

This removes the transport re-assignment path from vsock_assign_transport()
and replaces it with an assertion that the socket has no transport and no
private transport state yet.  The rationale given is that a failed
connect() de-assigns the transport and a connected socket keeps its
transport until close().

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 20181ddde114f..5cecd1c9c6e50 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -604,6 +604,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>  	__u8 remote_flags;
>  	int ret;
>  
> +	if (WARN_ON_ONCE(vsk->transport || vsk->trans))
> +		return -EINVAL;
> +

[Severity: Medium]
Is the invariant behind this assertion always true?  The commit message
says "On a failed connect(), transport is de-assigned", but
vsock_unconnected_reset() can return early without de-assigning:

net/vmw_vsock/af_vsock.c:vsock_unconnected_reset() {
	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;
	...
	vsock_deassign_transport(vsk);
	...
}

With the VMCI transport, can a peer detach during the handshake produce
exactly that state?  vmci_transport_recv_connecting_client_negotiate()
subscribes the detach callback while sk_state is still TCP_SYN_SENT, and
then:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
	sock_set_flag(sk, SOCK_DONE);

	/* On a detach the peer will not be sending or receiving
	 * anymore.
	 */
	WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
	...
		if (sk->sk_state == TCP_SYN_SENT) {
	...
			sk->sk_state = TCP_CLOSE;
			sk->sk_err = ECONNRESET;
}

The blocking connect() then wakes on sk_err and takes:

net/vmw_vsock/af_vsock.c:vsock_connect() {
out_reset:
	if (err)
		vsock_unconnected_reset(sk);
}

so WARN_ON_ONCE(vsk->peer_shutdown) fires and the transport is never
de-assigned, leaving sock->state at SS_CONNECTING until close().  A later
vsock_assign_transport() on that socket would then hit the new assertion
added here.

At this commit there also seems to be a second, transport-independent way
in: a peer RESPONSE followed by RST leaves the socket in TCP_CLOSING with
SOCK_DONE and peer_shutdown set while connect() sleeps, and since the wait
loop here only breaks on TCP_ESTABLISHED, a signal or the connect timeout
routes to out_reset with err != 0 and trips the same warnings.  That second
path looks like it is addressed later in the series by "vsock: treat
TCP_CLOSING as once-established" and the SS_CONNECTED guard from "vsock:
handle socket bricking due to transport event", but the VMCI detach case
appears to survive to the end of the series.

Would it make sense to make vsock_unconnected_reset() de-assign the
transport unconditionally, or to normalize SOCK_DONE/peer_shutdown there
before the checks?

[Severity: Low]
Could the new precondition be documented?  vsock_assign_transport() is
EXPORT_SYMBOL_GPL'd and called by the transports
(virtio_transport_common.c, vmci_transport.c, hyperv_transport.c), but the
descriptive block above the function still only covers transport selection
and the -ENODEV case:

/* Assign a transport to a socket and call the .init transport callback.
 *
 * Note: for connection oriented socket this must be called when vsk->remote_addr
 * is set ...
 *  - anything else goes to h2g or returns -ENODEV if no h2g is available
 */

and the declaration in include/net/af_vsock.h carries no documentation.  A
note that the function is one-shot per socket and now returns -EINVAL if
vsk->transport or vsk->trans is already set would help future callers.

[ ... ]

> @@ -668,26 +666,6 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>  	 */
>  	mutex_unlock(&vsock_register_mutex);
>  
> -	if (vsk->transport) {
> -		/* transport->release() must be called with sock lock acquired.
> -		 * This path can only be taken during vsock_connect(), where we
> -		 * have already held the sock lock. In the other cases, this
> -		 * function is called on a new socket which is not assigned to
> -		 * any transport.
> -		 */
> -		vsk->transport->release(vsk);
> -		vsock_deassign_transport(vsk);
> -
> -		/* transport's release() and destruct() can touch some socket
> -		 * state, since we are reassigning the socket to a new transport
> -		 * during vsock_connect(), let's reset these fields to have a
> -		 * clean state.
> -		 */
> -		sock_reset_flag(sk, SOCK_DONE);
> -		sk->sk_state = TCP_CLOSE;
> -		WRITE_ONCE(vsk->peer_shutdown, 0);
> -	}
> -

[Severity: Low]
With this block gone, two comments elsewhere in the file still explain a
load-bearing check in terms of reassignment.  Should they be updated?

net/vmw_vsock/af_vsock.c:vsock_remove_sock() {
	/* Transport reassignment must not remove the binding. */
	if (sock_flag(sk_vsock(vsk), SOCK_DEAD))
		vsock_remove_bound(vsk);
}

net/vmw_vsock/af_vsock.c:__vsock_release() {
	/* Indicate to vsock_remove_sock() that the socket is being released and
	 * can be removed from the bound_table. Unlike transport reassignment
	 * case, where the socket must remain bound despite vsock_remove_sock()
	 * being called from the transport release() callback.
	 */
	sock_set_flag(sk, SOCK_DEAD);
}

The SOCK_DEAD test itself still looks necessary, but for other callers:
virtio_transport_recv_connected() and virtio_transport_remove_sock() on the
RST path, and hvs_do_close_lock_held() from hvs_close_connection(), all run
on live sockets without SOCK_DEAD set.  The reassignment reason the
comments name no longer exists after this patch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-vsock-connect-reset-closing-v3-0-78907b8200d4%40rbox.co