Date:   Fri, 14 Feb 2020 10:17:04 +0100
From:   Stefano Garzarella <sgarzare@...hat.com>
To:     "Boeuf, Sebastien" <sebastien.boeuf@...el.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "stefanha@...hat.com" <stefanha@...hat.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH net v2] net: virtio_vsock: Enhance connection semantics

Hi Sebastien,
the patch and the test look good to me!
I tested with virtio and VMCI transports and your test works great, thanks!

I suggest split the patch in two, one for the fix and one for the test.

Are you using git format-patch / git send-email?
I fought a little bit to apply it because of CRLF ;-)

Thanks,
Stefano

On Thu, Feb 13, 2020 at 06:31:50PM +0000, Boeuf, Sebastien wrote:
> From d59100b8ab91289d9eac80d0e2a7ae9b0e19fe9c Mon Sep 17 00:00:00 2001
> From: Sebastien Boeuf <sebastien.boeuf@...el.com>
> Date: Thu, 13 Feb 2020 08:50:38 +0100
> Subject: [PATCH] net: virtio_vsock: Enhance connection semantics
> 
> Whenever the vsock backend on the host sends a packet through the RX
> queue, it expects an answer on the TX queue. Unfortunately, there is one
> case where the host side will hang waiting for the answer and might
> effectively never recover if no timeout mechanism was implemented.
> 
> This issue happens when the guest side starts binding to the socket,
> which insert a new bound socket into the list of already bound sockets.
> At this time, we expect the guest to also start listening, which will
> trigger the sk_state to move from TCP_CLOSE to TCP_LISTEN. The problem
> occurs if the host side queued a RX packet and triggered an interrupt
> right between the end of the binding process and the beginning of the
> listening process. In this specific case, the function processing the
> packet virtio_transport_recv_pkt() will find a bound socket, which means
> it will hit the switch statement checking for the sk_state, but the
> state won't be changed into TCP_LISTEN yet, which leads the code to pick
> the default statement. This default statement will only free the buffer,
> while it should also respond to the host side, by sending a packet on
> its TX queue.
> 
> In order to simply fix this unfortunate chain of events, it is important
> that in case the default statement is entered, and because at this stage
> we know the host side is waiting for an answer, we must send back a
> packet containing the operation VIRTIO_VSOCK_OP_RST.
> 
> One could say that a proper timeout mechanism on the host side will be
> enough to avoid the backend to hang. But the point of this patch is to
> ensure the normal use case will be provided with proper responsiveness
> when it comes to establishing the connection.
> 
> Signed-off-by: Sebastien Boeuf <sebastien.boeuf@...el.com>
> ---
>  net/vmw_vsock/virtio_transport_common.c |  1 +
>  tools/testing/vsock/vsock_test.c        | 77 +++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index d9f0c9c5425a..2f696124bab6 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -1153,6 +1153,7 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
>  		virtio_transport_free_pkt(pkt);
>  		break;
>  	default:
> +		(void)virtio_transport_reset_no_sock(t, pkt);
>  		virtio_transport_free_pkt(pkt);
>  		break;
>  	}
> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
> index 1d8b93f1af31..5a4fb80fa832 100644
> --- a/tools/testing/vsock/vsock_test.c
> +++ b/tools/testing/vsock/vsock_test.c
> @@ -55,6 +55,78 @@ static void test_stream_connection_reset(const struct test_opts *opts)
>  	close(fd);
>  }
>  
> +static void test_stream_bind_only_client(const struct test_opts *opts)
> +{
> +	union {
> +		struct sockaddr sa;
> +		struct sockaddr_vm svm;
> +	} addr = {
> +		.svm = {
> +			.svm_family = AF_VSOCK,
> +			.svm_port = 1234,
> +			.svm_cid = opts->peer_cid,
> +		},
> +	};
> +	int ret;
> +	int fd;
> +
> +	/* Wait for the server to be ready */
> +	control_expectln("BIND");
> +
> +	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
> +
> +	timeout_begin(TIMEOUT);
> +	do {
> +		ret = connect(fd, &addr.sa, sizeof(addr.svm));
> +		timeout_check("connect");
> +	} while (ret < 0 && errno == EINTR);
> +	timeout_end();
> +
> +	if (ret != -1) {
> +		fprintf(stderr, "expected connect(2) failure, got %d\n", ret);
> +		exit(EXIT_FAILURE);
> +	}
> +	if (errno != ECONNRESET) {
> +		fprintf(stderr, "unexpected connect(2) errno %d\n", errno);
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	/* Notify the server that the client has finished */
> +	control_writeln("DONE");
> +
> +	close(fd);
> +}
> +
> +static void test_stream_bind_only_server(const struct test_opts *opts)
> +{
> +	union {
> +		struct sockaddr sa;
> +		struct sockaddr_vm svm;
> +	} addr = {
> +		.svm = {
> +			.svm_family = AF_VSOCK,
> +			.svm_port = 1234,
> +			.svm_cid = VMADDR_CID_ANY,
> +		},
> +	};
> +	int fd;
> +
> +	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
> +
> +	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
> +		perror("bind");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	/* Notify the client that the server is ready */
> +	control_writeln("BIND");
> +
> +	/* Wait for the client to finish */
> +	control_expectln("DONE");
> +
> +	close(fd);
> +}
> +
>  static void test_stream_client_close_client(const struct test_opts *opts)
>  {
>  	int fd;
> @@ -212,6 +284,11 @@ static struct test_case test_cases[] = {
>  		.name = "SOCK_STREAM connection reset",
>  		.run_client = test_stream_connection_reset,
>  	},
> +	{
> +		.name = "SOCK_STREAM bind only",
> +		.run_client = test_stream_bind_only_client,
> +		.run_server = test_stream_bind_only_server,
> +	},
>  	{
>  		.name = "SOCK_STREAM client close",
>  		.run_client = test_stream_client_close_client,
> -- 
> 2.20.1
> 
> ---------------------------------------------------------------------
> Intel Corporation SAS (French simplified joint stock company)
> Registered headquarters: "Les Montalets"- 2, rue de Paris, 
> 92196 Meudon Cedex, France
> Registration Number:  302 456 199 R.C.S. NANTERRE
> Capital: 4,572,000 Euros
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

Powered by blists - more mailing lists