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(+)
Hi,
thanks for this patch, comments below...
On Tue, May 11, 2021 at 05:41:27PM +0800, Longpeng(Mike) wrote:
quoted 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(+)
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
-----Original Message-----
From: Stefano Garzarella [mailto:sgarzare@redhat.com]
Sent: Thursday, May 13, 2021 5:42 PM
To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
[off-list ref]
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gonglei (Arei)
[off-list ref]; Subo (Subo, Cloud Infrastructure Service Product
Dept.) [off-list ref]; David S . Miller [off-list ref]; Jakub
Kicinski [off-list ref]; Jorgen Hansen [off-list ref]; Norbert
Slusarek [off-list ref]; Andra Paraschiv [off-list ref];
Colin Ian King [off-list ref]; David Brazdil
[off-list ref]; Alexander Popov [off-list ref];
lixianming (E) [off-list ref]
Subject: Re: [RFC] vsock: notify server to shutdown when client has pending
signal
Hi,
thanks for this patch, comments below...
On Tue, May 11, 2021 at 05:41:27PM +0800, Longpeng(Mike) wrote:
quoted
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
@@ -1368,6 +1368,7 @@ static int vsock_stream_connect(struct socket *sock,
struct sockaddr *addr,
quoted
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?
Your method looks more gracefully, we'll try it and get back to you, thanks.
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.
-----Original Message-----
From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
[mailto:longpeng2@huawei.com]
Sent: Thursday, May 13, 2021 6:36 PM
To: Stefano Garzarella <sgarzare@redhat.com>
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gonglei (Arei)
[off-list ref]; Subo (Subo, Cloud Infrastructure Service Product
Dept.) [off-list ref]; David S . Miller [off-list ref]; Jakub
Kicinski [off-list ref]; Jorgen Hansen [off-list ref]; Norbert
Slusarek [off-list ref]; Andra Paraschiv [off-list ref];
Colin Ian King [off-list ref]; David Brazdil
[off-list ref]; Alexander Popov [off-list ref];
lixianming (E) [off-list ref]
Subject: RE: [RFC] vsock: notify server to shutdown when client has pending
signal
Hi Stefano,
quoted
-----Original Message-----
From: Stefano Garzarella [mailto:sgarzare@redhat.com]
Sent: Thursday, May 13, 2021 5:42 PM
To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
[off-list ref]
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gonglei
(Arei) [off-list ref]; Subo (Subo, Cloud Infrastructure
Service Product
Dept.) [off-list ref]; David S . Miller [off-list ref];
Jakub Kicinski [off-list ref]; Jorgen Hansen [off-list ref];
Norbert Slusarek [off-list ref]; Andra Paraschiv
[off-list ref]; Colin Ian King [off-list ref];
David Brazdil [off-list ref]; Alexander Popov
[off-list ref]; lixianming (E) [off-list ref]
Subject: Re: [RFC] vsock: notify server to shutdown when client has
pending signal
Hi,
thanks for this patch, comments below...
On Tue, May 11, 2021 at 05:41:27PM +0800, Longpeng(Mike) wrote:
quoted
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(+)
@@ -1368,6 +1368,7 @@ static int vsock_stream_connect(struct socket
*sock,
struct sockaddr *addr,
quoted
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?
Your method looks more gracefully, we'll try it and get back to you, thanks.
As your suggestion, the following code can solve the problem:
if (signal_pending(current)) {
err = sock_intr_errno(timeout);
- sk->sk_state = TCP_CLOSE;
+ sk->sk_state = TCP_CLOSING;
sock->state = SS_UNCONNECTED;
vsock_transport_cancel_pkt(vsk);
goto out_wait;
This will send shutdown to the server even if the connection is not established, but
I don't see any side effects yet, right ?
The problem is also in the timeout case, we should fix it together ?
quoted
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.
On Mon, May 17, 2021 at 02:18:51AM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
Hi Stefano,
quoted
-----Original Message-----
From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
[mailto:longpeng2@huawei.com]
Sent: Thursday, May 13, 2021 6:36 PM
To: Stefano Garzarella <sgarzare@redhat.com>
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gonglei (Arei)
[off-list ref]; Subo (Subo, Cloud Infrastructure Service Product
Dept.) [off-list ref]; David S . Miller [off-list ref]; Jakub
Kicinski [off-list ref]; Jorgen Hansen [off-list ref]; Norbert
Slusarek [off-list ref]; Andra Paraschiv [off-list ref];
Colin Ian King [off-list ref]; David Brazdil
[off-list ref]; Alexander Popov [off-list ref];
lixianming (E) [off-list ref]
Subject: RE: [RFC] vsock: notify server to shutdown when client has pending
signal
Hi Stefano,
quoted
-----Original Message-----
From: Stefano Garzarella [mailto:sgarzare@redhat.com]
Sent: Thursday, May 13, 2021 5:42 PM
To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
[off-list ref]
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gonglei
(Arei) [off-list ref]; Subo (Subo, Cloud Infrastructure
Service Product
Dept.) [off-list ref]; David S . Miller [off-list ref];
Jakub Kicinski [off-list ref]; Jorgen Hansen [off-list ref];
Norbert Slusarek [off-list ref]; Andra Paraschiv
[off-list ref]; Colin Ian King [off-list ref];
David Brazdil [off-list ref]; Alexander Popov
[off-list ref]; lixianming (E) [off-list ref]
Subject: Re: [RFC] vsock: notify server to shutdown when client has
pending signal
Hi,
thanks for this patch, comments below...
On Tue, May 11, 2021 at 05:41:27PM +0800, Longpeng(Mike) wrote:
quoted
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(+)
@@ -1368,6 +1368,7 @@ static int vsock_stream_connect(struct socket
*sock,
struct sockaddr *addr,
quoted
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?
Your method looks more gracefully, we'll try it and get back to you,
thanks.
As your suggestion, the following code can solve the problem:
if (signal_pending(current)) {
err = sock_intr_errno(timeout);
- sk->sk_state = TCP_CLOSE;
+ sk->sk_state = TCP_CLOSING;
sock->state = SS_UNCONNECTED;
vsock_transport_cancel_pkt(vsk);
goto out_wait;
This will send shutdown to the server even if the connection is not established, but
I don't see any side effects yet, right ?
Should we set TCP_CLOSING only if sk_state was TCP_ESTABLISHED?
The problem is also in the timeout case, we should fix it together ?
I'm not sure, if we reach the timeout, it should mean that the other
peer never answered, so why take care to notify it?
Thanks,
Stefano