Thread (5 messages) flat view 5 messages, 2 authors, 2021-05-17

Re: [RFC] vsock: notify server to shutdown when client has pending signal

From: Stefano Garzarella <sgarzare@redhat.com>
Date: 2021-05-13 09:41:54
Also in: lkml

Hi,
thanks for this patch, comments below...

On Tue, May 11, 2021 at 05:41:27PM +0800, Longpeng(Mike) wrote:
quoted hunk ↗ jump to hunk
The client's sk_state will be set to TCP_ESTABLISHED if the
server replay the client's connect request.
However, if the client has pending signal, its sk_state will
be set to TCP_CLOSE without notify the server, so the server
will hold the corrupt connection.

           client                        server

1. sk_state=TCP_SYN_SENT         |
2. call ->connect()              |
3. wait reply                    |
                                | 4. sk_state=TCP_ESTABLISHED
                                | 5. insert to connected list
                                | 6. reply to the client
7. sk_state=TCP_ESTABLISHED      |
8. insert to connected list      |
9. *signal pending* <--------------------- the user kill client
10. sk_state=TCP_CLOSE           |
client is exiting...             |
11. call ->release()             |
    virtio_transport_close
     if (!(sk->sk_state == TCP_ESTABLISHED ||
      sk->sk_state == TCP_CLOSING))
	return true; <------------- return at here
As a result, the server cannot notice the connection is corrupt.
So the client should notify the peer in this case.

Cc: David S. Miller <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Stefano Garzarella <sgarzare@redhat.com>
Cc: Jorgen Hansen <redacted>
Cc: Norbert Slusarek <redacted>
Cc: Andra Paraschiv <redacted>
Cc: Colin Ian King <redacted>
Cc: David Brazdil <redacted>
Cc: Alexander Popov <redacted>
Signed-off-by: lixianming <redacted>
Signed-off-by: Longpeng(Mike) <redacted>
---
net/vmw_vsock/af_vsock.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 92a72f0..d5df908 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1368,6 +1368,7 @@ static int vsock_stream_connect(struct socket *sock, struct sockaddr *addr,
		lock_sock(sk);

		if (signal_pending(current)) {
+			vsock_send_shutdown(sk, SHUTDOWN_MASK);
I see the issue, but I'm not sure is okay to send the shutdown in any 
case, think about the server didn't setup the connection.

Maybe is better to set TCP_CLOSING if the socket state was 
TCP_ESTABLISHED, so the shutdown will be handled by the 
transport->release() as usual.

What do you think?

Anyway, also without the patch, the server should receive a RST if it 
sends any data to the client, but of course, is better to let it know 
the socket is closed in advance.

Thanks,
Stefano
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help