Re: [PATCH net v2] net: virtio_vsock: Enhance connection semantics

Stefano Garzarella
2020-02-14T09:17:15Z

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@intel.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@intel.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.