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