netdev - Re: [PATCH net v2] vsock: avoid to close connected socket after the timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <atd47kt3wyhw2mhrzob4fxzlvmw37xviehn7agao3srni2jmts@gxgzvqrb7mc7>
Date: Thu, 11 May 2023 15:00:20 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Zhuang Shengen <zhuangshengen@...wei.com>
Cc: virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, arei.gonglei@...wei.com, 
	longpeng2@...wei.com, jianjay.zhou@...wei.com
Subject: Re: [PATCH net v2] vsock: avoid to close connected socket after the
 timeout

On Thu, May 11, 2023 at 07:34:30PM +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. 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@...wei.com>
>---
> net/vmw_vsock/af_vsock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

The patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@...hat.com>

I noticed that the net maintainers are not in cc, if this patch isn't
queued in the next days, please resend it by cc'ing all the
maintainers [1]:

$ ./scripts/get_maintainer.pl \
   20230510_zhuangshengen_vsock_bugfix_port_residue_in_server.mbx
Stefano Garzarella <sgarzare@...hat.com> (maintainer:VM SOCKETS (AF_VSOCK))
"David S. Miller" <davem@...emloft.net> (maintainer:NETWORKING [GENERAL])
Eric Dumazet <edumazet@...gle.com> (maintainer:NETWORKING [GENERAL])
Jakub Kicinski <kuba@...nel.org> (maintainer:NETWORKING [GENERAL])
Paolo Abeni <pabeni@...hat.com> (maintainer:NETWORKING [GENERAL])
virtualization@...ts.linux-foundation.org (open list:VM SOCKETS (AF_VSOCK))
netdev@...r.kernel.org (open list:VM SOCKETS (AF_VSOCK))
linux-kernel@...r.kernel.org (open list)

Thanks,
Stefano

[1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#select-the-recipients-for-your-patch

>
>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
>