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

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

[Boeuf, Sebastien]

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.

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

[Stefano Garzarella]

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.

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

[Boeuf, Sebastien]

On Fri, 2020-02-14 at 10:17 +0100, Stefano Garzarella wrote:
> 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'm glad everything worked out well so quickly :)

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

Ok will do!

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

Well I have been using evolution mail client, but I'm gonna use the
proper way with git send-email.

One question though, if I split this into 2 patches, I need to write a
cover letter?

Thanks,
Sebastien
> 
> 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.
---------------------------------------------------------------------
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.

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

[Stefano Garzarella]

On Fri, Feb 14, 2020 at 10:23:44AM +0000, Boeuf, Sebastien wrote:
> On Fri, 2020-02-14 at 10:17 +0100, Stefano Garzarella wrote:
> > 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'm glad everything worked out well so quickly :)

:)

> 
> > 
> > I suggest split the patch in two, one for the fix and one for the
> > test.
> 
> Ok will do!
> 
> > 
> > Are you using git format-patch / git send-email?
> > I fought a little bit to apply it because of CRLF ;-)
> 
> Well I have been using evolution mail client, but I'm gonna use the
> proper way with git send-email.

A very useful tool is the Stefan's git-publish:
https://github.com/stefanha/git-publish

> 
> One question though, if I split this into 2 patches, I need to write a
> cover letter?

Yes, it is better.

Just a brief description of the series and its patches.

Thanks,
Stefano