| Message ID | 20230510142502.2293109-1-zhuangshengen@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | vsock: bugfix port residue in server | expand |
Hi, thanks for the patch, the change LGTM, but I have the following suggestions: Please avoid "bugfix" in the subject, "fix" should be enough: https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes Anyway, I suggest to change the subject in "vsock: avoid to close connected socket after the timeout" On Wed, May 10, 2023 at 10:25:02PM +0800, Zhuang Shengen wrote: >When client and server establish a connection through vsock, >the client send a request to the server to initiate the connection, >then start a timer to wait for the server's response. When the server's >RESPONSE message arrives, the timer also times out and exits. The >server's RESPONSE message is processed first, and the connection is >established. However, the client's timer also times out, the original >processing logic of the client is to directly set the state of this vsock >to CLOSE and return ETIMEDOUT, User will release the port. It will not What to you mean with "User" here? >notify the server when the port is released, causing the server port remain > Can we remove this blank line? >when client's vsock_connect timeout，it should check sk state is The remote peer can't trust the other peer, indeed it will receive an error after sending the first message and it will remove the connection, right? >ESTABLISHED or not. if sk state is ESTABLISHED, it means the connection >is established, the client should not set the sk state to CLOSE > >Note: I encountered this issue on kernel-4.18, which can be fixed by >this patch. Then I checked the latest code in the community >and found similar issue. > In order to backport it to the stable kernels, we should add a Fixes tag: https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes Thanks, Stefano >Signed-off-by: Zhuang Shengen <zhuangshengen@huawei.com> >--- > net/vmw_vsock/af_vsock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >index 413407bb646c..efb8a0937a13 100644 >--- a/net/vmw_vsock/af_vsock.c >+++ b/net/vmw_vsock/af_vsock.c >@@ -1462,7 +1462,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr, > vsock_transport_cancel_pkt(vsk); > vsock_remove_connected(vsk); > goto out_wait; >- } else if (timeout == 0) { >+ } else if ((sk->sk_state != TCP_ESTABLISHED) && (timeout == 0)) { > err = -ETIMEDOUT; > sk->sk_state = TCP_CLOSE; > sock->state = SS_UNCONNECTED; >-- >2.27.0 >
Hi Stefano: 在 2023/5/10 23:23, Stefano Garzarella 写道: > Hi, > thanks for the patch, the change LGTM, but I have the following > suggestions: > > Please avoid "bugfix" in the subject, "fix" should be enough: > https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes > > > Anyway, I suggest to change the subject in > "vsock: avoid to close connected socket after the timeout" > thanks for your suggestion, I will change the subject > On Wed, May 10, 2023 at 10:25:02PM +0800, Zhuang Shengen wrote: >> When client and server establish a connection through vsock, >> the client send a request to the server to initiate the connection, >> then start a timer to wait for the server's response. When the server's >> RESPONSE message arrives, the timer also times out and exits. The >> server's RESPONSE message is processed first, and the connection is >> established. However, the client's timer also times out, the original >> processing logic of the client is to directly set the state of this >> vsock >> to CLOSE and return ETIMEDOUT, User will release the port. It will not > > What to you mean with "User" here? > The User means Client, I will delete the statement "User will release the por", it indeed difficult to understand >> notify the server when the port is released, causing the server port >> remain >> > > Can we remove this blank line? > OK >> when client's vsock_conqnect timeout，it should check sk state is > > The remote peer can't trust the other peer, indeed it will receive an > error after sending the first message and it will remove the connection, > right? > If this situation happend, the server will response a RST? Anyway the connection will not established before timeout, The sk state wont be ESTABLISHED. >> ESTABLISHED or not. if sk state is ESTABLISHED, it means the connection >> is established, the client should not set the sk state to CLOSE >> >> Note: I encountered this issue on kernel-4.18, which can be fixed by >> this patch. Then I checked the latest code in the community >> and found similar issue. >> > > In order to backport it to the stable kernels, we should add a Fixes tag: > https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes > > OK, I add a Fixes: d021c344051a ("VSOCK: Introduce VM Sockets") in the new patch. I put the new patch with v2 title in the attachment, please check. Thanks, Shengen > Thanks, > Stefano > >> Signed-off-by: Zhuang Shengen <zhuangshengen@huawei.com> >> --- >> net/vmw_vsock/af_vsock.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >> index 413407bb646c..efb8a0937a13 100644 >> --- a/net/vmw_vsock/af_vsock.c >> +++ b/net/vmw_vsock/af_vsock.c >> @@ -1462,7 +1462,7 @@ static int vsock_connect(struct socket *sock, >> struct sockaddr *addr, >> vsock_transport_cancel_pkt(vsk); >> vsock_remove_connected(vsk); >> goto out_wait; >> - } else if (timeout == 0) { >> + } else if ((sk->sk_state != TCP_ESTABLISHED) && (timeout == >> 0)) { >> err = -ETIMEDOUT; >> sk->sk_state = TCP_CLOSE; >> sock->state = SS_UNCONNECTED; >> -- >> 2.27.0 >> > From ba9826c501d45cb4688eb67924ac6dc86c16088e Mon Sep 17 00:00:00 2001 From: Zhuang Shengen <zhuangshengen@huawei.com> Date: Wed, 10 May 2023 21:36:46 +0800 Subject: [PATCH net v2] vsock: avoid to close connected socket after the timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When client and server establish a connection through vsock, the client send a request to the server to initiate the connection, then start a timer to wait for the server's response. When the server's RESPONSE message arrives, the timer also times out and exits. The server's RESPONSE message is processed first, and the connection is established. However, the client's timer also times out, the original processing logic of the client is to directly set the state of this vsock to CLOSE and return ETIMEDOUT. It will not notify the server when the port is released, causing the server port remain. when client's vsock_connect timeout，it should check sk state is ESTABLISHED or not. if sk state is ESTABLISHED, it means the connection is established, the client should not set the sk state to CLOSE Note: I encountered this issue on kernel-4.18, which can be fixed by this patch. Then I checked the latest code in the community and found similar issue. Fixes: d021c344051a ("VSOCK: Introduce VM Sockets") Signed-off-by: Zhuang Shengen <zhuangshengen@huawei.com> --- net/vmw_vsock/af_vsock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index 413407bb646c..efb8a0937a13 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -1462,7 +1462,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr, vsock_transport_cancel_pkt(vsk); vsock_remove_connected(vsk); goto out_wait; - } else if (timeout == 0) { + } else if ((sk->sk_state != TCP_ESTABLISHED) && (timeout == 0)) { err = -ETIMEDOUT; sk->sk_state = TCP_CLOSE; sock->state = SS_UNCONNECTED;
On Thu, May 11, 2023 at 03:03:24PM +0800, Zhuang Shengen wrote: >Hi Stefano: > >在 2023/5/10 23:23, Stefano Garzarella 写道: >>Hi, >>thanks for the patch, the change LGTM, but I have the following >>suggestions: >> >>Please avoid "bugfix" in the subject, "fix" should be enough: >>https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes >> >> >>Anyway, I suggest to change the subject in >>"vsock: avoid to close connected socket after the timeout" >> >thanks for your suggestion, I will change the subject >>On Wed, May 10, 2023 at 10:25:02PM +0800, Zhuang Shengen wrote: >>>When client and server establish a connection through vsock, >>>the client send a request to the server to initiate the connection, >>>then start a timer to wait for the server's response. When the server's >>>RESPONSE message arrives, the timer also times out and exits. The >>>server's RESPONSE message is processed first, and the connection is >>>established. However, the client's timer also times out, the original >>>processing logic of the client is to directly set the state of >>>this vsock >>>to CLOSE and return ETIMEDOUT, User will release the port. It will not >> >>What to you mean with "User" here? >> >The User means Client, I will delete the statement "User will release >the por", it indeed difficult to understand >>>notify the server when the port is released, causing the server >>>port remain >>> >> >>Can we remove this blank line? >> >OK >>>when client's vsock_conqnect timeout，it should check sk state is >> >>The remote peer can't trust the other peer, indeed it will receive an >>error after sending the first message and it will remove the connection, >>right? >> >If this situation happend, the server will response a RST? Anyway the >connection will not established before timeout, The sk state wont be >ESTABLISHED. >>>ESTABLISHED or not. if sk state is ESTABLISHED, it means the connection >>>is established, the client should not set the sk state to CLOSE >>> >>>Note: I encountered this issue on kernel-4.18, which can be fixed by >>>this patch. Then I checked the latest code in the community >>>and found similar issue. >>> >> >>In order to backport it to the stable kernels, we should add a Fixes tag: >>https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes >> >> >OK, I add a Fixes: d021c344051a ("VSOCK: Introduce VM Sockets") in the >new patch. > >I put the new patch with v2 title in the attachment, please check. LGTM (great to have added the net tag!), but please post as plain text like v1. Thanks, Stefano
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index 413407bb646c..efb8a0937a13 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -1462,7 +1462,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr, vsock_transport_cancel_pkt(vsk); vsock_remove_connected(vsk); goto out_wait; - } else if (timeout == 0) { + } else if ((sk->sk_state != TCP_ESTABLISHED) && (timeout == 0)) { err = -ETIMEDOUT; sk->sk_state = TCP_CLOSE; sock->state = SS_UNCONNECTED;
When client and server establish a connection through vsock, the client send a request to the server to initiate the connection, then start a timer to wait for the server's response. When the server's RESPONSE message arrives, the timer also times out and exits. The server's RESPONSE message is processed first, and the connection is established. However, the client's timer also times out, the original processing logic of the client is to directly set the state of this vsock to CLOSE and return ETIMEDOUT, User will release the port. It will not notify the server when the port is released, causing the server port remain when client's vsock_connect timeout，it should check sk state is ESTABLISHED or not. if sk state is ESTABLISHED, it means the connection is established, the client should not set the sk state to CLOSE Note: I encountered this issue on kernel-4.18, which can be fixed by this patch. Then I checked the latest code in the community and found similar issue. Signed-off-by: Zhuang Shengen <zhuangshengen@huawei.com> --- net/vmw_vsock/af_vsock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)