Re: [PATCH net v2 4/5] vsock: Do not reset a TCP_CLOSING socket

Handle the previously overlooked TCP_ESTABLISHED -> TCP_CLOSING
transition (on VIRTIO_VSOCK_OP_RST), which could race with the connect
loop.

Resetting a socket that is still present in connected_table can lead to
memory corruption. The reporter noted lost transports for in-flight skbs,
and I have reproduced crashes caused by re-insertion into connected_table.

 list_add double add: new=, prev=, next=.
 kernel BUG at lib/list_debug.c:35!
 Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
 Workqueue: vsock-loopback vsock_loopback_work
 RIP: 0010:__list_add_valid_or_report+0x11f/0x130
 Call Trace:
  vsock_insert_connected.cold+0xe/0x13
  virtio_transport_recv_pkt+0x10e9/0x1460
  vsock_loopback_work+0x305/0x480
  process_one_work+0xe4c/0x1560
  worker_thread+0x4f1/0xd60
  kthread+0x36e/0x470
  ret_from_fork+0x47b/0x6b0
  ret_from_fork_asm+0x1a/0x30

Drop the redundant err=0 and the inaccurate comment above signal_pending().
This fix is supplementary to commit 002541ef650b ("vsock: Ignore
signal/timeout on connect() if already established"). Details under Link.

Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Reported-by: Hyunwoo Kim <redacted>
Link: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/ (local)
Signed-off-by: Michal Luczaj <redacted>
---
net/vmw_vsock/af_vsock.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 499e902becfa..adf3f018347e 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c

@@ -1712,7 +1712,6 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,

	long timeout;
	DEFINE_WAIT(wait);

-	err = 0;
	sk = sock->sk;
	vsk = vsock_sk(sk);

@@ -1834,23 +1833,22 @@ 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
-		 * release it, that's not connect()'s concern. No need to go
+		/* Connection was 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 = 0;
+			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
		 * 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) || timeout == 0) {
			err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);

@@ -1874,8 +1872,8 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,

		prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
	}

-	err = sock_error(sk);
-	if (err) {