Re: [PATCH net v3 7/8] vsock: handle listen() racing connect()

[netdev-bot+sashiko@kernel.org]

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

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

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 commit message states the goal is to 'let connect() time
  out, but don't reset the socket', implying the pre-patch code resets a…
- [Low] The patch fixes a user-visible, unprivileged-reachable defect
  that exists in released kernels but carries no `Fixes:` tag and no…

--

vsock: handle listen() racing connect()

This adds an early return to vsock_unconnected_reset() so that a socket
which a racing listen() has moved to TCP_LISTEN is left alone. A blocking
connect() that was sleeping in schedule_timeout() then returns its error
without demoting the listener.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5a2d7e10ecb88..56c5959424564 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1663,6 +1663,13 @@ static void vsock_unconnected_reset(struct sock *sk)
>  
>  	sock_owned_by_me(sk);
>  
> +	/*
> +	 * listen() can race connect() on schedule_timeout().
> +	 * Don't reset TCP_LISTEN.
> +	 */
> +	if (sk->sk_state == TCP_LISTEN)
> +		return;
> +

[Severity: Medium]
The changelog says the intent is to "let connect() time out, but don't
reset the socket", which reads as though the code just before this patch
resets a TCP_LISTEN socket.

Is that still true with the immediate parent ("vsock: handle socket
bricking due to transport event") applied?  In that tree the race
described in the changelog reaches the checks already present in
vsock_unconnected_reset():

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;
	...
}

With sk_state == TCP_LISTEN and sk->sk_socket->state == SS_UNCONNECTED,
the SS_CONNECTED/SS_DISCONNECTING guard does not fire, peer_shutdown is
zero and SOCK_DONE is unset, so control reaches the third WARN_ON_ONCE
and returns without resetting anything.

That makes the pre-patch symptom at this point of the series a
userspace-triggerable splat from vsock_connect()'s out_reset: path rather
than a lost listener, and a splat is fatal with panic_on_warn.  The
window covers the commits from "vsock: do not preserve transport
assignment" through "vsock: handle socket bricking due to transport
event", so a bisect or a partial backport can land on it.

Would it make sense to squash this TCP_LISTEN check into the commit that
introduced that WARN_ON_ONCE, or otherwise mention the warning in the
changelog?

[Severity: Low]
Should this patch carry a Fixes: tag?  The sibling fixes in the same
series carry Fixes: d021c344051a ("VSOCK: Introduce VM Sockets"), while
this one has only a Signed-off-by.

Released kernels appear to hit the same race without this guard.  At the
pre-series baseline, vsock_connect() writes the state back unconditionally
with no sk_state test:

net/vmw_vsock/af_vsock.c:vsock_connect() {
	...
	err = sock_error(sk);
	if (err) {
		sk->sk_state = TCP_CLOSE;
		sock->state = SS_UNCONNECTED;
	}
	...
}

and vsock_listen() only requires SS_UNCONNECTED plus a bound address
before flipping the state:

net/vmw_vsock/af_vsock.c:vsock_listen() {
	...
	if (sock->state != SS_UNCONNECTED) {
		err = -EINVAL;
		goto out;
	}
	...
	if (!vsock_addr_bound(&vsk->local_addr)) {
		err = -EINVAL;
		goto out;
	}

	sk->sk_max_ack_backlog = backlog;
	sk->sk_state = TCP_LISTEN;
	...
}

So on a stable tree, two threads sharing one fd (non-blocking connect()
arming vsock_connect_timeout(), blocking connect() sleeping in
schedule_timeout(), then listen()) can end up with the listener demoted
to TCP_CLOSE/SS_UNCONNECTED and accept() returning -EINVAL.  Without a
Fixes: or Cc: stable trailer, is there anything to signal that to
backporters?

>  	/*
>  	 * Transport events are serviced by resetting each connected socket to
>  	 * TCP_CLOSE. The reset is done under vsock_table_lock but without

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