On 8/12/26 22:13, Hyunwoo Kim wrote:
> commit 002541ef650b ("vsock: Ignore signal/timeout on connect() if
> already established") stopped connect() from resetting an established
> socket. The check only looks at whether sk_state is TCP_ESTABLISHED at
> that moment, and the state can change while connect() sleeps.
I guess this makes my fix incomplete. "Fixes: 002541ef650b"?
> A peer RST moves the socket to TCP_CLOSING, and it is not removed from
> vsock_connected_table on that path. connect() then wakes up, fails the
> check, and resets a socket that had actually connected to TCP_CLOSE and
> SS_UNCONNECTED.
Thanks for the details. Do I get it right: connect() misses the fact that
socket might have already transitioned TCP_ESTABLISHED -> TCP_CLOSING
during schedule_timeout()?
How about:
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 622dbd046799..39c42ef016c3 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1807,15 +1807,18 @@ 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
+ /* Connection (has been) 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 = -sk->sk_err;
+ 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
?
> The socket can now be connected again while it is still on the table.
> Reconnecting to an address served by a different transport makes
> vsock_assign_transport() drop the transport from a live socket and free
> vsk->trans, even if skbs it already sent are still in flight.
> Reconnecting to the same address inserts a node that is already on the
> table, provided shutdown() has cleared SOCK_DONE in between.
>
> sock->state cannot be used for the check either. shutdown() overwrites
> SS_CONNECTED with SS_DISCONNECTING.
>
> Record on the socket that the connection completed, and check that
> instead. The sk_err path after the loop does the same reset, so guard it
> as well.
