| Message ID | 20210511094127.724-1-longpeng2@huawei.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] vsock: notify server to shutdown when client has pending signal | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hi, thanks for this patch, comments below... On Tue, May 11, 2021 at 05:41:27PM +0800, Longpeng(Mike) wrote: >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 <jhansen@vmware.com> >Cc: Norbert Slusarek <nslusarek@gmx.net> >Cc: Andra Paraschiv <andraprs@amazon.com> >Cc: Colin Ian King <colin.king@canonical.com> >Cc: David Brazdil <dbrazdil@google.com> >Cc: Alexander Popov <alex.popov@linux.com> >Signed-off-by: lixianming <lixianming5@huawei.com> >Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> >--- > 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
Hi 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.) > <longpeng2@huawei.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gonglei (Arei) > <arei.gonglei@huawei.com>; Subo (Subo, Cloud Infrastructure Service Product > Dept.) <subo7@huawei.com>; David S . Miller <davem@davemloft.net>; Jakub > Kicinski <kuba@kernel.org>; Jorgen Hansen <jhansen@vmware.com>; Norbert > Slusarek <nslusarek@gmx.net>; Andra Paraschiv <andraprs@amazon.com>; > Colin Ian King <colin.king@canonical.com>; David Brazdil > <dbrazdil@google.com>; Alexander Popov <alex.popov@linux.com>; > lixianming (E) <lixianming5@huawei.com> > 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: > >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 <jhansen@vmware.com> > >Cc: Norbert Slusarek <nslusarek@gmx.net> > >Cc: Andra Paraschiv <andraprs@amazon.com> > >Cc: Colin Ian King <colin.king@canonical.com> > >Cc: David Brazdil <dbrazdil@google.com> > >Cc: Alexander Popov <alex.popov@linux.com> > >Signed-off-by: lixianming <lixianming5@huawei.com> > >Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > >--- > > 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? > 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. > Yes, agree. > Thanks, > Stefano
Hi Stefano, > -----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) > <arei.gonglei@huawei.com>; Subo (Subo, Cloud Infrastructure Service Product > Dept.) <subo7@huawei.com>; David S . Miller <davem@davemloft.net>; Jakub > Kicinski <kuba@kernel.org>; Jorgen Hansen <jhansen@vmware.com>; Norbert > Slusarek <nslusarek@gmx.net>; Andra Paraschiv <andraprs@amazon.com>; > Colin Ian King <colin.king@canonical.com>; David Brazdil > <dbrazdil@google.com>; Alexander Popov <alex.popov@linux.com>; > lixianming (E) <lixianming5@huawei.com> > Subject: RE: [RFC] vsock: notify server to shutdown when client has pending > signal > > Hi 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.) > > <longpeng2@huawei.com> > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gonglei > > (Arei) <arei.gonglei@huawei.com>; Subo (Subo, Cloud Infrastructure > > Service Product > > Dept.) <subo7@huawei.com>; David S . Miller <davem@davemloft.net>; > > Jakub Kicinski <kuba@kernel.org>; Jorgen Hansen <jhansen@vmware.com>; > > Norbert Slusarek <nslusarek@gmx.net>; Andra Paraschiv > > <andraprs@amazon.com>; Colin Ian King <colin.king@canonical.com>; > > David Brazdil <dbrazdil@google.com>; Alexander Popov > > <alex.popov@linux.com>; lixianming (E) <lixianming5@huawei.com> > > 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: > > >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 <jhansen@vmware.com> > > >Cc: Norbert Slusarek <nslusarek@gmx.net> > > >Cc: Andra Paraschiv <andraprs@amazon.com> > > >Cc: Colin Ian King <colin.king@canonical.com> > > >Cc: David Brazdil <dbrazdil@google.com> > > >Cc: Alexander Popov <alex.popov@linux.com> > > >Signed-off-by: lixianming <lixianming5@huawei.com> > > >Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > > >--- > > > 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? > > > > 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 ? > > 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. > > > > Yes, agree. > > > Thanks, > > Stefano
On Mon, May 17, 2021 at 02:18:51AM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: >Hi Stefano, > >> -----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) >> <arei.gonglei@huawei.com>; Subo (Subo, Cloud Infrastructure Service Product >> Dept.) <subo7@huawei.com>; David S . Miller <davem@davemloft.net>; Jakub >> Kicinski <kuba@kernel.org>; Jorgen Hansen <jhansen@vmware.com>; Norbert >> Slusarek <nslusarek@gmx.net>; Andra Paraschiv <andraprs@amazon.com>; >> Colin Ian King <colin.king@canonical.com>; David Brazdil >> <dbrazdil@google.com>; Alexander Popov <alex.popov@linux.com>; >> lixianming (E) <lixianming5@huawei.com> >> Subject: RE: [RFC] vsock: notify server to shutdown when client has pending >> signal >> >> Hi 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.) >> > <longpeng2@huawei.com> >> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gonglei >> > (Arei) <arei.gonglei@huawei.com>; Subo (Subo, Cloud Infrastructure >> > Service Product >> > Dept.) <subo7@huawei.com>; David S . Miller <davem@davemloft.net>; >> > Jakub Kicinski <kuba@kernel.org>; Jorgen Hansen <jhansen@vmware.com>; >> > Norbert Slusarek <nslusarek@gmx.net>; Andra Paraschiv >> > <andraprs@amazon.com>; Colin Ian King <colin.king@canonical.com>; >> > David Brazdil <dbrazdil@google.com>; Alexander Popov >> > <alex.popov@linux.com>; lixianming (E) <lixianming5@huawei.com> >> > 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: >> > >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 <jhansen@vmware.com> >> > >Cc: Norbert Slusarek <nslusarek@gmx.net> >> > >Cc: Andra Paraschiv <andraprs@amazon.com> >> > >Cc: Colin Ian King <colin.king@canonical.com> >> > >Cc: David Brazdil <dbrazdil@google.com> >> > >Cc: Alexander Popov <alex.popov@linux.com> >> > >Signed-off-by: lixianming <lixianming5@huawei.com> >> > >Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> >> > >--- >> > > 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? >> > >> >> 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
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); err = sock_intr_errno(timeout); sk->sk_state = TCP_CLOSE; sock->state = SS_UNCONNECTED;
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 <jhansen@vmware.com> Cc: Norbert Slusarek <nslusarek@gmx.net> Cc: Andra Paraschiv <andraprs@amazon.com> Cc: Colin Ian King <colin.king@canonical.com> Cc: David Brazdil <dbrazdil@google.com> Cc: Alexander Popov <alex.popov@linux.com> Signed-off-by: lixianming <lixianming5@huawei.com> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> --- net/vmw_vsock/af_vsock.c | 1 + 1 file changed, 1 insertion(+)