* [PATCH] net: virtio_vsock: Fix race condition between bind and listen @ 2020-02-13 9:16 Boeuf, Sebastien 2020-02-13 9:41 ` Stefano Garzarella 0 siblings, 1 reply; 13+ messages in thread From: Boeuf, Sebastien @ 2020-02-13 9:16 UTC (permalink / raw) To: netdev; +Cc: stefanha, sgarzare, davem From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition between bind and listen 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 will effectively never recover. 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. Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> --- net/vmw_vsock/virtio_transport_common.c | 1 + 1 file changed, 1 insertion(+) diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index e5ea29c6bca7..909334d58328 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -1143,6 +1143,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; } -- 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. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 9:16 [PATCH] net: virtio_vsock: Fix race condition between bind and listen Boeuf, Sebastien @ 2020-02-13 9:41 ` Stefano Garzarella 2020-02-13 9:51 ` Boeuf, Sebastien 0 siblings, 1 reply; 13+ messages in thread From: Stefano Garzarella @ 2020-02-13 9:41 UTC (permalink / raw) To: Boeuf, Sebastien; +Cc: netdev, stefanha, davem Hi Sebastien, On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien wrote: > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition between bind and listen > > 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 will > effectively never recover. Do you have a test case? In the host, the af_vsock.c:vsock_stream_connect() set a timeout, so if the host try to connect before the guest starts listening, the connect() should return ETIMEDOUT if the guest does not answer anything. Anyway, maybe the patch make sense anyway, changing a bit the description (if the host connect() receive the ETIMEDOUT). I'm just concerned that this code is common between guest and host. If a malicious guest starts sending us wrong requests, we spend time sending a reset packet. But we already do that if we can't find the bound socket, so it might make sense. Thanks, Stefano > > 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. > > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> > --- > net/vmw_vsock/virtio_transport_common.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c > index e5ea29c6bca7..909334d58328 100644 > --- a/net/vmw_vsock/virtio_transport_common.c > +++ b/net/vmw_vsock/virtio_transport_common.c > @@ -1143,6 +1143,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; > } > -- > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 9:41 ` Stefano Garzarella @ 2020-02-13 9:51 ` Boeuf, Sebastien 2020-02-13 10:22 ` Stefano Garzarella 0 siblings, 1 reply; 13+ messages in thread From: Boeuf, Sebastien @ 2020-02-13 9:51 UTC (permalink / raw) To: sgarzare; +Cc: stefanha, netdev, davem Hi Stefano, On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > Hi Sebastien, > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien wrote: > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition between bind > > and listen > > > > 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 will > > effectively never recover. > > Do you have a test case? Yes I do. This has been a bug we've been investigating on Kata Containers for quite some time now. This was happening when using Kata along with Cloud-Hypervisor (which rely on the hybrid vsock implementation from Firecracker). The thing is, this bug is very hard to reproduce and was happening for Kata because of the connection strategy. The kata-runtime tries to connect a million times after it started the VM, just hoping the kata-agent will start to listen from the guest side at some point. > > In the host, the af_vsock.c:vsock_stream_connect() set a timeout, so > if > the host try to connect before the guest starts listening, the > connect() > should return ETIMEDOUT if the guest does not answer anything. > > Anyway, maybe the patch make sense anyway, changing a bit the > description > (if the host connect() receive the ETIMEDOUT). > I'm just concerned that this code is common between guest and host. > If a > malicious guest starts sending us wrong requests, we spend time > sending > a reset packet. But we already do that if we can't find the bound > socket, > so it might make sense. Yes I don't think this is gonna cause more trouble, but at least we cannot end up in this weird situation I described. I was just not sure if the function we should use to do the reset should be virtio_transport_reset_no_sock() or virtio_transport_reset() since at this point the socket is already bound. Thanks, Sebastien > > Thanks, > Stefano > > > 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. > > > > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com> > > --- > > net/vmw_vsock/virtio_transport_common.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/net/vmw_vsock/virtio_transport_common.c > > b/net/vmw_vsock/virtio_transport_common.c > > index e5ea29c6bca7..909334d58328 100644 > > --- a/net/vmw_vsock/virtio_transport_common.c > > +++ b/net/vmw_vsock/virtio_transport_common.c > > @@ -1143,6 +1143,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; > > } > > -- > > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 9:51 ` Boeuf, Sebastien @ 2020-02-13 10:22 ` Stefano Garzarella 2020-02-13 10:44 ` Boeuf, Sebastien 0 siblings, 1 reply; 13+ messages in thread From: Stefano Garzarella @ 2020-02-13 10:22 UTC (permalink / raw) To: Boeuf, Sebastien; +Cc: stefanha, netdev, davem On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien wrote: > Hi Stefano, > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > > Hi Sebastien, > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien wrote: > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition between bind > > > and listen > > > > > > 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 will > > > effectively never recover. > > > > Do you have a test case? > > Yes I do. This has been a bug we've been investigating on Kata > Containers for quite some time now. This was happening when using Kata > along with Cloud-Hypervisor (which rely on the hybrid vsock > implementation from Firecracker). The thing is, this bug is very hard > to reproduce and was happening for Kata because of the connection > strategy. The kata-runtime tries to connect a million times after it > started the VM, just hoping the kata-agent will start to listen from > the guest side at some point. Maybe is related to something else. I tried the following which should be your case simplified (IIUC): guest$ python import socket s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) s.bind((socket.VMADDR_CID_ANY, 1234)) host$ python import socket s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) s.connect((3, 1234)) Traceback (most recent call last): File "<stdin>", line 1, in <module> TimeoutError: [Errno 110] Connection timed out > > > > > In the host, the af_vsock.c:vsock_stream_connect() set a timeout, so > > if > > the host try to connect before the guest starts listening, the > > connect() > > should return ETIMEDOUT if the guest does not answer anything. > > > > Anyway, maybe the patch make sense anyway, changing a bit the > > description > > (if the host connect() receive the ETIMEDOUT). > > I'm just concerned that this code is common between guest and host. > > If a > > malicious guest starts sending us wrong requests, we spend time > > sending > > a reset packet. But we already do that if we can't find the bound > > socket, > > so it might make sense. > > Yes I don't think this is gonna cause more trouble, but at least we > cannot end up in this weird situation I described. Okay, but in the host, we can't trust the guest to answer, we should handle this case properly. > > I was just not sure if the function we should use to do the reset > should be virtio_transport_reset_no_sock() or virtio_transport_reset() > since at this point the socket is already bound. I think you can safely use virtio_transport_reset() in this case. Maybe we can add a test case to the vsock test suite (tools/testing/vsock) to stress the connect. Thanks, Stefano ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 10:22 ` Stefano Garzarella @ 2020-02-13 10:44 ` Boeuf, Sebastien 2020-02-13 11:02 ` Stefano Garzarella 2020-02-13 11:39 ` Stefan Hajnoczi 0 siblings, 2 replies; 13+ messages in thread From: Boeuf, Sebastien @ 2020-02-13 10:44 UTC (permalink / raw) To: sgarzare; +Cc: stefanha, netdev, davem On Thu, 2020-02-13 at 11:22 +0100, Stefano Garzarella wrote: > On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien wrote: > > Hi Stefano, > > > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > > > Hi Sebastien, > > > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien wrote: > > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition between > > > > bind > > > > and listen > > > > > > > > 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 > > > > will > > > > effectively never recover. > > > > > > Do you have a test case? > > > > Yes I do. This has been a bug we've been investigating on Kata > > Containers for quite some time now. This was happening when using > > Kata > > along with Cloud-Hypervisor (which rely on the hybrid vsock > > implementation from Firecracker). The thing is, this bug is very > > hard > > to reproduce and was happening for Kata because of the connection > > strategy. The kata-runtime tries to connect a million times after > > it > > started the VM, just hoping the kata-agent will start to listen > > from > > the guest side at some point. > > Maybe is related to something else. I tried the following which > should be > your case simplified (IIUC): > > guest$ python > import socket > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > s.bind((socket.VMADDR_CID_ANY, 1234)) > > host$ python > import socket > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > s.connect((3, 1234)) > > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > TimeoutError: [Errno 110] Connection timed out Yes this is exactly the simplified case. But that's the point, I don't think the timeout is the best way to go here. Because this means that when we run into this case, the host side will wait for quite some time before retrying, which can cause a very long delay before the communication with the guest is established. By simply answering the host with a RST packet, we inform it that nobody's listening on the guest side yet, therefore the host side will close and try again. > > > > In the host, the af_vsock.c:vsock_stream_connect() set a timeout, > > > so > > > if > > > the host try to connect before the guest starts listening, the > > > connect() > > > should return ETIMEDOUT if the guest does not answer anything. > > > > > > Anyway, maybe the patch make sense anyway, changing a bit the > > > description > > > (if the host connect() receive the ETIMEDOUT). > > > I'm just concerned that this code is common between guest and > > > host. > > > If a > > > malicious guest starts sending us wrong requests, we spend time > > > sending > > > a reset packet. But we already do that if we can't find the bound > > > socket, > > > so it might make sense. > > > > Yes I don't think this is gonna cause more trouble, but at least we > > cannot end up in this weird situation I described. > > Okay, but in the host, we can't trust the guest to answer, we should > handle this case properly. Well I cannot agree more with the "we cannot trust the guest" philosophy, but in this case the worst thing that can happen is the guest shooting himself in the foot because it would simply prevent the connection from happening. And I agree setting up a timeout from the host side is still a good idea for preventing from such DOS attack. But as I mentioned above, in the normal use case, this allows for better responsiveness when it comes to establish the connection as fast as possible. > > > I was just not sure if the function we should use to do the reset > > should be virtio_transport_reset_no_sock() or > > virtio_transport_reset() > > since at this point the socket is already bound. > > I think you can safely use virtio_transport_reset() in this case. I've just tried it and unfortunately it doesn't work. I think that's because the connection has not been properly established yet, so we cannot consider being in this case. Using virtio_transport_reset_no_sock() seems like the less intrusive function here. > > Maybe we can add a test case to the vsock test suite > (tools/testing/vsock) > to stress the connect. Oh yes that's a good idea :) Thanks, Sebastien > > Thanks, > Stefano > --------------------------------------------------------------------- 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 10:44 ` Boeuf, Sebastien @ 2020-02-13 11:02 ` Stefano Garzarella 2020-02-13 11:13 ` Boeuf, Sebastien 2020-02-13 11:39 ` Stefan Hajnoczi 1 sibling, 1 reply; 13+ messages in thread From: Stefano Garzarella @ 2020-02-13 11:02 UTC (permalink / raw) To: Boeuf, Sebastien; +Cc: stefanha, netdev, davem On Thu, Feb 13, 2020 at 10:44:18AM +0000, Boeuf, Sebastien wrote: > On Thu, 2020-02-13 at 11:22 +0100, Stefano Garzarella wrote: > > On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien wrote: > > > Hi Stefano, > > > > > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > > > > Hi Sebastien, > > > > > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien wrote: > > > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition between > > > > > bind > > > > > and listen > > > > > > > > > > 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 > > > > > will > > > > > effectively never recover. > > > > > > > > Do you have a test case? > > > > > > Yes I do. This has been a bug we've been investigating on Kata > > > Containers for quite some time now. This was happening when using > > > Kata > > > along with Cloud-Hypervisor (which rely on the hybrid vsock > > > implementation from Firecracker). The thing is, this bug is very > > > hard > > > to reproduce and was happening for Kata because of the connection > > > strategy. The kata-runtime tries to connect a million times after > > > it > > > started the VM, just hoping the kata-agent will start to listen > > > from > > > the guest side at some point. > > > > Maybe is related to something else. I tried the following which > > should be > > your case simplified (IIUC): > > > > guest$ python > > import socket > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > s.bind((socket.VMADDR_CID_ANY, 1234)) > > > > host$ python > > import socket > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > s.connect((3, 1234)) > > > > Traceback (most recent call last): > > File "<stdin>", line 1, in <module> > > TimeoutError: [Errno 110] Connection timed out > > Yes this is exactly the simplified case. But that's the point, I don't > think the timeout is the best way to go here. Because this means that > when we run into this case, the host side will wait for quite some time > before retrying, which can cause a very long delay before the > communication with the guest is established. By simply answering the > host with a RST packet, we inform it that nobody's listening on the > guest side yet, therefore the host side will close and try again. Yes, make sense. I just wanted to point out that the host shouldn't get stuck if the guest doesn't respond. So it's weird that this happens and it might be related to some other problem. > > > > > > > In the host, the af_vsock.c:vsock_stream_connect() set a timeout, > > > > so > > > > if > > > > the host try to connect before the guest starts listening, the > > > > connect() > > > > should return ETIMEDOUT if the guest does not answer anything. > > > > > > > > Anyway, maybe the patch make sense anyway, changing a bit the > > > > description > > > > (if the host connect() receive the ETIMEDOUT). > > > > I'm just concerned that this code is common between guest and > > > > host. > > > > If a > > > > malicious guest starts sending us wrong requests, we spend time > > > > sending > > > > a reset packet. But we already do that if we can't find the bound > > > > socket, > > > > so it might make sense. > > > > > > Yes I don't think this is gonna cause more trouble, but at least we > > > cannot end up in this weird situation I described. > > > > Okay, but in the host, we can't trust the guest to answer, we should > > handle this case properly. > > Well I cannot agree more with the "we cannot trust the guest" > philosophy, but in this case the worst thing that can happen is the > guest shooting himself in the foot because it would simply prevent the > connection from happening. > > And I agree setting up a timeout from the host side is still a good > idea for preventing from such DOS attack. > > But as I mentioned above, in the normal use case, this allows for > better responsiveness when it comes to establish the connection as fast > as possible. Sure, maybe you can rewrite a bit the commit (title and body) to explain this. > > > > > > I was just not sure if the function we should use to do the reset > > > should be virtio_transport_reset_no_sock() or > > > virtio_transport_reset() > > > since at this point the socket is already bound. > > > > I think you can safely use virtio_transport_reset() in this case. > > I've just tried it and unfortunately it doesn't work. I think that's > because the connection has not been properly established yet, so we > cannot consider being in this case. > Using virtio_transport_reset_no_sock() seems like the less intrusive > function here. Oh sorry, I also put a comment on virtio_transport_reset() to say to use it only on connected sockets and not listeners. In this case it's a listener, sorry for the wrong suggestion. Thanks, Stefano ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 11:02 ` Stefano Garzarella @ 2020-02-13 11:13 ` Boeuf, Sebastien 2020-02-13 11:22 ` Stefano Garzarella 0 siblings, 1 reply; 13+ messages in thread From: Boeuf, Sebastien @ 2020-02-13 11:13 UTC (permalink / raw) To: sgarzare; +Cc: stefanha, netdev, davem On Thu, 2020-02-13 at 12:02 +0100, Stefano Garzarella wrote: > On Thu, Feb 13, 2020 at 10:44:18AM +0000, Boeuf, Sebastien wrote: > > On Thu, 2020-02-13 at 11:22 +0100, Stefano Garzarella wrote: > > > On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien wrote: > > > > Hi Stefano, > > > > > > > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > > > > > Hi Sebastien, > > > > > > > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien > > > > > wrote: > > > > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition > > > > > > between > > > > > > bind > > > > > > and listen > > > > > > > > > > > > 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 > > > > > > will > > > > > > effectively never recover. > > > > > > > > > > Do you have a test case? > > > > > > > > Yes I do. This has been a bug we've been investigating on Kata > > > > Containers for quite some time now. This was happening when > > > > using > > > > Kata > > > > along with Cloud-Hypervisor (which rely on the hybrid vsock > > > > implementation from Firecracker). The thing is, this bug is > > > > very > > > > hard > > > > to reproduce and was happening for Kata because of the > > > > connection > > > > strategy. The kata-runtime tries to connect a million times > > > > after > > > > it > > > > started the VM, just hoping the kata-agent will start to listen > > > > from > > > > the guest side at some point. > > > > > > Maybe is related to something else. I tried the following which > > > should be > > > your case simplified (IIUC): > > > > > > guest$ python > > > import socket > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > s.bind((socket.VMADDR_CID_ANY, 1234)) > > > > > > host$ python > > > import socket > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > s.connect((3, 1234)) > > > > > > Traceback (most recent call last): > > > File "<stdin>", line 1, in <module> > > > TimeoutError: [Errno 110] Connection timed out > > > > Yes this is exactly the simplified case. But that's the point, I > > don't > > think the timeout is the best way to go here. Because this means > > that > > when we run into this case, the host side will wait for quite some > > time > > before retrying, which can cause a very long delay before the > > communication with the guest is established. By simply answering > > the > > host with a RST packet, we inform it that nobody's listening on the > > guest side yet, therefore the host side will close and try again. > > Yes, make sense. > > I just wanted to point out that the host shouldn't get stuck if the > guest doesn't respond. So it's weird that this happens and it might > be > related to some other problem. Yes that's because we're using the hybrid vsock implementation from Firecracker. So basically they proxy a UNIX socket connection into a VSOCK socket. And the timeout is not implemented. I'll point them out that's something it'd be nice to have along with this fix in the guest. > > > > > > In the host, the af_vsock.c:vsock_stream_connect() set a > > > > > timeout, > > > > > so > > > > > if > > > > > the host try to connect before the guest starts listening, > > > > > the > > > > > connect() > > > > > should return ETIMEDOUT if the guest does not answer > > > > > anything. > > > > > > > > > > Anyway, maybe the patch make sense anyway, changing a bit the > > > > > description > > > > > (if the host connect() receive the ETIMEDOUT). > > > > > I'm just concerned that this code is common between guest and > > > > > host. > > > > > If a > > > > > malicious guest starts sending us wrong requests, we spend > > > > > time > > > > > sending > > > > > a reset packet. But we already do that if we can't find the > > > > > bound > > > > > socket, > > > > > so it might make sense. > > > > > > > > Yes I don't think this is gonna cause more trouble, but at > > > > least we > > > > cannot end up in this weird situation I described. > > > > > > Okay, but in the host, we can't trust the guest to answer, we > > > should > > > handle this case properly. > > > > Well I cannot agree more with the "we cannot trust the guest" > > philosophy, but in this case the worst thing that can happen is the > > guest shooting himself in the foot because it would simply prevent > > the > > connection from happening. > > > > And I agree setting up a timeout from the host side is still a good > > idea for preventing from such DOS attack. > > > > But as I mentioned above, in the normal use case, this allows for > > better responsiveness when it comes to establish the connection as > > fast > > as possible. > > Sure, maybe you can rewrite a bit the commit (title and body) to > explain > this. Of course, let me work on this. How am I supposed to resend the patch? Should I send a new email with v2 in the title? > > > > > I was just not sure if the function we should use to do the > > > > reset > > > > should be virtio_transport_reset_no_sock() or > > > > virtio_transport_reset() > > > > since at this point the socket is already bound. > > > > > > I think you can safely use virtio_transport_reset() in this case. > > > > I've just tried it and unfortunately it doesn't work. I think > > that's > > because the connection has not been properly established yet, so we > > cannot consider being in this case. > > Using virtio_transport_reset_no_sock() seems like the less > > intrusive > > function here. > > Oh sorry, I also put a comment on virtio_transport_reset() to say to > use it > only on connected sockets and not listeners. > In this case it's a listener, sorry for the wrong suggestion. Hehe no worries, at least we're on the same page :) Thanks, Sebastien > > Thanks, > Stefano > --------------------------------------------------------------------- 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 11:13 ` Boeuf, Sebastien @ 2020-02-13 11:22 ` Stefano Garzarella 0 siblings, 0 replies; 13+ messages in thread From: Stefano Garzarella @ 2020-02-13 11:22 UTC (permalink / raw) To: Boeuf, Sebastien; +Cc: stefanha, netdev, davem On Thu, Feb 13, 2020 at 11:13:55AM +0000, Boeuf, Sebastien wrote: > On Thu, 2020-02-13 at 12:02 +0100, Stefano Garzarella wrote: > > On Thu, Feb 13, 2020 at 10:44:18AM +0000, Boeuf, Sebastien wrote: > > > On Thu, 2020-02-13 at 11:22 +0100, Stefano Garzarella wrote: > > > > On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien wrote: > > > > > Hi Stefano, > > > > > > > > > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > > > > > > Hi Sebastien, > > > > > > > > > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien > > > > > > wrote: > > > > > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition > > > > > > > between > > > > > > > bind > > > > > > > and listen > > > > > > > > > > > > > > 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 > > > > > > > will > > > > > > > effectively never recover. > > > > > > > > > > > > Do you have a test case? > > > > > > > > > > Yes I do. This has been a bug we've been investigating on Kata > > > > > Containers for quite some time now. This was happening when > > > > > using > > > > > Kata > > > > > along with Cloud-Hypervisor (which rely on the hybrid vsock > > > > > implementation from Firecracker). The thing is, this bug is > > > > > very > > > > > hard > > > > > to reproduce and was happening for Kata because of the > > > > > connection > > > > > strategy. The kata-runtime tries to connect a million times > > > > > after > > > > > it > > > > > started the VM, just hoping the kata-agent will start to listen > > > > > from > > > > > the guest side at some point. > > > > > > > > Maybe is related to something else. I tried the following which > > > > should be > > > > your case simplified (IIUC): > > > > > > > > guest$ python > > > > import socket > > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > > s.bind((socket.VMADDR_CID_ANY, 1234)) > > > > > > > > host$ python > > > > import socket > > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > > s.connect((3, 1234)) > > > > > > > > Traceback (most recent call last): > > > > File "<stdin>", line 1, in <module> > > > > TimeoutError: [Errno 110] Connection timed out > > > > > > Yes this is exactly the simplified case. But that's the point, I > > > don't > > > think the timeout is the best way to go here. Because this means > > > that > > > when we run into this case, the host side will wait for quite some > > > time > > > before retrying, which can cause a very long delay before the > > > communication with the guest is established. By simply answering > > > the > > > host with a RST packet, we inform it that nobody's listening on the > > > guest side yet, therefore the host side will close and try again. > > > > Yes, make sense. > > > > I just wanted to point out that the host shouldn't get stuck if the > > guest doesn't respond. So it's weird that this happens and it might > > be > > related to some other problem. > > Yes that's because we're using the hybrid vsock implementation from > Firecracker. So basically they proxy a UNIX socket connection into a > VSOCK socket. And the timeout is not implemented. I'll point them out > that's something it'd be nice to have along with this fix in the guest. > Okay, now I get it. > > > > > > > > In the host, the af_vsock.c:vsock_stream_connect() set a > > > > > > timeout, > > > > > > so > > > > > > if > > > > > > the host try to connect before the guest starts listening, > > > > > > the > > > > > > connect() > > > > > > should return ETIMEDOUT if the guest does not answer > > > > > > anything. > > > > > > > > > > > > Anyway, maybe the patch make sense anyway, changing a bit the > > > > > > description > > > > > > (if the host connect() receive the ETIMEDOUT). > > > > > > I'm just concerned that this code is common between guest and > > > > > > host. > > > > > > If a > > > > > > malicious guest starts sending us wrong requests, we spend > > > > > > time > > > > > > sending > > > > > > a reset packet. But we already do that if we can't find the > > > > > > bound > > > > > > socket, > > > > > > so it might make sense. > > > > > > > > > > Yes I don't think this is gonna cause more trouble, but at > > > > > least we > > > > > cannot end up in this weird situation I described. > > > > > > > > Okay, but in the host, we can't trust the guest to answer, we > > > > should > > > > handle this case properly. > > > > > > Well I cannot agree more with the "we cannot trust the guest" > > > philosophy, but in this case the worst thing that can happen is the > > > guest shooting himself in the foot because it would simply prevent > > > the > > > connection from happening. > > > > > > And I agree setting up a timeout from the host side is still a good > > > idea for preventing from such DOS attack. > > > > > > But as I mentioned above, in the normal use case, this allows for > > > better responsiveness when it comes to establish the connection as > > > fast > > > as possible. > > > > Sure, maybe you can rewrite a bit the commit (title and body) to > > explain > > this. > > Of course, let me work on this. > How am I supposed to resend the patch? Should I send a new email with > v2 in the title? > Yes, v2 in the []. Something like this: [PATCH net v2] vsock/virtio: ... > > > > > > > I was just not sure if the function we should use to do the > > > > > reset > > > > > should be virtio_transport_reset_no_sock() or > > > > > virtio_transport_reset() > > > > > since at this point the socket is already bound. > > > > > > > > I think you can safely use virtio_transport_reset() in this case. > > > > > > I've just tried it and unfortunately it doesn't work. I think > > > that's > > > because the connection has not been properly established yet, so we > > > cannot consider being in this case. > > > Using virtio_transport_reset_no_sock() seems like the less > > > intrusive > > > function here. > > > > Oh sorry, I also put a comment on virtio_transport_reset() to say to > > use it > > only on connected sockets and not listeners. > > In this case it's a listener, sorry for the wrong suggestion. > > Hehe no worries, at least we're on the same page :) > :-) Thanks, Stefano ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 10:44 ` Boeuf, Sebastien 2020-02-13 11:02 ` Stefano Garzarella @ 2020-02-13 11:39 ` Stefan Hajnoczi 2020-02-13 13:04 ` Stefano Garzarella 1 sibling, 1 reply; 13+ messages in thread From: Stefan Hajnoczi @ 2020-02-13 11:39 UTC (permalink / raw) To: Boeuf, Sebastien; +Cc: sgarzare, netdev, davem [-- Attachment #1: Type: text/plain, Size: 3390 bytes --] On Thu, Feb 13, 2020 at 10:44:18AM +0000, Boeuf, Sebastien wrote: > On Thu, 2020-02-13 at 11:22 +0100, Stefano Garzarella wrote: > > On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien wrote: > > > Hi Stefano, > > > > > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > > > > Hi Sebastien, > > > > > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien wrote: > > > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition between > > > > > bind > > > > > and listen > > > > > > > > > > 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 > > > > > will > > > > > effectively never recover. > > > > > > > > Do you have a test case? > > > > > > Yes I do. This has been a bug we've been investigating on Kata > > > Containers for quite some time now. This was happening when using > > > Kata > > > along with Cloud-Hypervisor (which rely on the hybrid vsock > > > implementation from Firecracker). The thing is, this bug is very > > > hard > > > to reproduce and was happening for Kata because of the connection > > > strategy. The kata-runtime tries to connect a million times after > > > it > > > started the VM, just hoping the kata-agent will start to listen > > > from > > > the guest side at some point. > > > > Maybe is related to something else. I tried the following which > > should be > > your case simplified (IIUC): > > > > guest$ python > > import socket > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > s.bind((socket.VMADDR_CID_ANY, 1234)) > > > > host$ python > > import socket > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > s.connect((3, 1234)) > > > > Traceback (most recent call last): > > File "<stdin>", line 1, in <module> > > TimeoutError: [Errno 110] Connection timed out > > Yes this is exactly the simplified case. But that's the point, I don't > think the timeout is the best way to go here. Because this means that > when we run into this case, the host side will wait for quite some time > before retrying, which can cause a very long delay before the > communication with the guest is established. By simply answering the > host with a RST packet, we inform it that nobody's listening on the > guest side yet, therefore the host side will close and try again. My expectation is that TCP/IP will produce ECONNREFUSED in this case but I haven't checked. Timing out is weird behavior. In any case, the reference for virtio-vsock semantics is: 1. How does VMCI (VMware) vsock behave? We strive to be compatible with the VMCI transport. 2. If there is no clear VMCI behavior, then we look at TCP/IP because those semantics are expected by most applications. This bug needs a test case in tools/testings/vsock/ and that test case will run against VMCI, virtio-vsock, and Hyper-V. Doing that will answer the question of how VMCI handles this case. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 11:39 ` Stefan Hajnoczi @ 2020-02-13 13:04 ` Stefano Garzarella 2020-02-13 16:51 ` Boeuf, Sebastien 0 siblings, 1 reply; 13+ messages in thread From: Stefano Garzarella @ 2020-02-13 13:04 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Boeuf, Sebastien, netdev, davem On Thu, Feb 13, 2020 at 11:39:49AM +0000, Stefan Hajnoczi wrote: > On Thu, Feb 13, 2020 at 10:44:18AM +0000, Boeuf, Sebastien wrote: > > On Thu, 2020-02-13 at 11:22 +0100, Stefano Garzarella wrote: > > > On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien wrote: > > > > Hi Stefano, > > > > > > > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > > > > > Hi Sebastien, > > > > > > > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien wrote: > > > > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition between > > > > > > bind > > > > > > and listen > > > > > > > > > > > > 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 > > > > > > will > > > > > > effectively never recover. > > > > > > > > > > Do you have a test case? > > > > > > > > Yes I do. This has been a bug we've been investigating on Kata > > > > Containers for quite some time now. This was happening when using > > > > Kata > > > > along with Cloud-Hypervisor (which rely on the hybrid vsock > > > > implementation from Firecracker). The thing is, this bug is very > > > > hard > > > > to reproduce and was happening for Kata because of the connection > > > > strategy. The kata-runtime tries to connect a million times after > > > > it > > > > started the VM, just hoping the kata-agent will start to listen > > > > from > > > > the guest side at some point. > > > > > > Maybe is related to something else. I tried the following which > > > should be > > > your case simplified (IIUC): > > > > > > guest$ python > > > import socket > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > s.bind((socket.VMADDR_CID_ANY, 1234)) > > > > > > host$ python > > > import socket > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > s.connect((3, 1234)) > > > > > > Traceback (most recent call last): > > > File "<stdin>", line 1, in <module> > > > TimeoutError: [Errno 110] Connection timed out > > > > Yes this is exactly the simplified case. But that's the point, I don't > > think the timeout is the best way to go here. Because this means that > > when we run into this case, the host side will wait for quite some time > > before retrying, which can cause a very long delay before the > > communication with the guest is established. By simply answering the > > host with a RST packet, we inform it that nobody's listening on the > > guest side yet, therefore the host side will close and try again. > > My expectation is that TCP/IP will produce ECONNREFUSED in this case but > I haven't checked. Timing out is weird behavior. I just tried and yes, TCP/IP produces ECONNREFUSED. The same error returned when no one's bound to the port. Instead virtio-vsock returns ECONNRESET in the last case. I'm not sure it's correct (looking at the man page connect(2) it would seem not), but if I understood correctly VMCI returns the same ECONNRESET in this case. > > In any case, the reference for virtio-vsock semantics is: > 1. How does VMCI (VMware) vsock behave? We strive to be compatible with > the VMCI transport. Looking at the code, it looks like VMCI returns ECONNRESET in this case, so this patch should be okay. (I haven't tried it) > 2. If there is no clear VMCI behavior, then we look at TCP/IP because > those semantics are expected by most applications. > > This bug needs a test case in tools/testings/vsock/ and that test case > will run against VMCI, virtio-vsock, and Hyper-V. Doing that will > answer the question of how VMCI handles this case. I agree. Thanks, Stefano ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 13:04 ` Stefano Garzarella @ 2020-02-13 16:51 ` Boeuf, Sebastien 2020-02-13 17:14 ` Stefano Garzarella 0 siblings, 1 reply; 13+ messages in thread From: Boeuf, Sebastien @ 2020-02-13 16:51 UTC (permalink / raw) To: sgarzare, stefanha; +Cc: netdev, davem On Thu, 2020-02-13 at 14:04 +0100, Stefano Garzarella wrote: > On Thu, Feb 13, 2020 at 11:39:49AM +0000, Stefan Hajnoczi wrote: > > On Thu, Feb 13, 2020 at 10:44:18AM +0000, Boeuf, Sebastien wrote: > > > On Thu, 2020-02-13 at 11:22 +0100, Stefano Garzarella wrote: > > > > On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien > > > > wrote: > > > > > Hi Stefano, > > > > > > > > > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > > > > > > Hi Sebastien, > > > > > > > > > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien > > > > > > wrote: > > > > > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition > > > > > > > between > > > > > > > bind > > > > > > > and listen > > > > > > > > > > > > > > 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 > > > > > > > will > > > > > > > effectively never recover. > > > > > > > > > > > > Do you have a test case? > > > > > > > > > > Yes I do. This has been a bug we've been investigating on > > > > > Kata > > > > > Containers for quite some time now. This was happening when > > > > > using > > > > > Kata > > > > > along with Cloud-Hypervisor (which rely on the hybrid vsock > > > > > implementation from Firecracker). The thing is, this bug is > > > > > very > > > > > hard > > > > > to reproduce and was happening for Kata because of the > > > > > connection > > > > > strategy. The kata-runtime tries to connect a million times > > > > > after > > > > > it > > > > > started the VM, just hoping the kata-agent will start to > > > > > listen > > > > > from > > > > > the guest side at some point. > > > > > > > > Maybe is related to something else. I tried the following which > > > > should be > > > > your case simplified (IIUC): > > > > > > > > guest$ python > > > > import socket > > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > > s.bind((socket.VMADDR_CID_ANY, 1234)) > > > > > > > > host$ python > > > > import socket > > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > > s.connect((3, 1234)) > > > > > > > > Traceback (most recent call last): > > > > File "<stdin>", line 1, in <module> > > > > TimeoutError: [Errno 110] Connection timed out > > > > > > Yes this is exactly the simplified case. But that's the point, I > > > don't > > > think the timeout is the best way to go here. Because this means > > > that > > > when we run into this case, the host side will wait for quite > > > some time > > > before retrying, which can cause a very long delay before the > > > communication with the guest is established. By simply answering > > > the > > > host with a RST packet, we inform it that nobody's listening on > > > the > > > guest side yet, therefore the host side will close and try again. > > > > My expectation is that TCP/IP will produce ECONNREFUSED in this > > case but > > I haven't checked. Timing out is weird behavior. > > I just tried and yes, TCP/IP produces ECONNREFUSED. The same error > returned > when no one's bound to the port. > > Instead virtio-vsock returns ECONNRESET in the last case. > I'm not sure it's correct (looking at the man page connect(2) it > would > seem not), but if I understood correctly VMCI returns the same > ECONNRESET in this case. > > > In any case, the reference for virtio-vsock semantics is: > > 1. How does VMCI (VMware) vsock behave? We strive to be compatible > > with > > the VMCI transport. > > Looking at the code, it looks like VMCI returns ECONNRESET in this > case, > so this patch should be okay. (I haven't tried it) > > > 2. If there is no clear VMCI behavior, then we look at TCP/IP > > because > > those semantics are expected by most applications. > > > > This bug needs a test case in tools/testings/vsock/ and that test > > case > > will run against VMCI, virtio-vsock, and Hyper-V. Doing that will > > answer the question of how VMCI handles this case. > > I agree. I'm trying to write the test but I'm kinda stuck as I have no way to ensure the proper timing between the test on the server(guest) and the test on from the client(host). Basically I was thinking about creating a new test reusing test_stream_connection_reset() from vsock_test.c. The reused function would be used for the run_client callback, while I would define a new function for the run_server. I wanted to basically do a bind only, and don't go up to the listen/accept calls. Problem is, the server won't block after the bind is done, and I don't know how much the test should wait between the bind and the end. I would like to show that even when the server reaches the point where the socket is bound, the connect will still return with ECONNRESET. The same test without the patch would simply hang till we hit the timeout on the client side. Does that make sense? And how much time should I wait for after the bind on the server side? Thanks, Sebastien > > Thanks, > Stefano > --------------------------------------------------------------------- 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 16:51 ` Boeuf, Sebastien @ 2020-02-13 17:14 ` Stefano Garzarella 2020-02-13 17:18 ` Boeuf, Sebastien 0 siblings, 1 reply; 13+ messages in thread From: Stefano Garzarella @ 2020-02-13 17:14 UTC (permalink / raw) To: Boeuf, Sebastien; +Cc: stefanha, netdev, davem On Thu, Feb 13, 2020 at 5:51 PM Boeuf, Sebastien <sebastien.boeuf@intel.com> wrote: > > On Thu, 2020-02-13 at 14:04 +0100, Stefano Garzarella wrote: > > On Thu, Feb 13, 2020 at 11:39:49AM +0000, Stefan Hajnoczi wrote: > > > On Thu, Feb 13, 2020 at 10:44:18AM +0000, Boeuf, Sebastien wrote: > > > > On Thu, 2020-02-13 at 11:22 +0100, Stefano Garzarella wrote: > > > > > On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien > > > > > wrote: > > > > > > Hi Stefano, > > > > > > > > > > > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella wrote: > > > > > > > Hi Sebastien, > > > > > > > > > > > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, Sebastien > > > > > > > wrote: > > > > > > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race condition > > > > > > > > between > > > > > > > > bind > > > > > > > > and listen > > > > > > > > > > > > > > > > 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 > > > > > > > > will > > > > > > > > effectively never recover. > > > > > > > > > > > > > > Do you have a test case? > > > > > > > > > > > > Yes I do. This has been a bug we've been investigating on > > > > > > Kata > > > > > > Containers for quite some time now. This was happening when > > > > > > using > > > > > > Kata > > > > > > along with Cloud-Hypervisor (which rely on the hybrid vsock > > > > > > implementation from Firecracker). The thing is, this bug is > > > > > > very > > > > > > hard > > > > > > to reproduce and was happening for Kata because of the > > > > > > connection > > > > > > strategy. The kata-runtime tries to connect a million times > > > > > > after > > > > > > it > > > > > > started the VM, just hoping the kata-agent will start to > > > > > > listen > > > > > > from > > > > > > the guest side at some point. > > > > > > > > > > Maybe is related to something else. I tried the following which > > > > > should be > > > > > your case simplified (IIUC): > > > > > > > > > > guest$ python > > > > > import socket > > > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > > > s.bind((socket.VMADDR_CID_ANY, 1234)) > > > > > > > > > > host$ python > > > > > import socket > > > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > > > s.connect((3, 1234)) > > > > > > > > > > Traceback (most recent call last): > > > > > File "<stdin>", line 1, in <module> > > > > > TimeoutError: [Errno 110] Connection timed out > > > > > > > > Yes this is exactly the simplified case. But that's the point, I > > > > don't > > > > think the timeout is the best way to go here. Because this means > > > > that > > > > when we run into this case, the host side will wait for quite > > > > some time > > > > before retrying, which can cause a very long delay before the > > > > communication with the guest is established. By simply answering > > > > the > > > > host with a RST packet, we inform it that nobody's listening on > > > > the > > > > guest side yet, therefore the host side will close and try again. > > > > > > My expectation is that TCP/IP will produce ECONNREFUSED in this > > > case but > > > I haven't checked. Timing out is weird behavior. > > > > I just tried and yes, TCP/IP produces ECONNREFUSED. The same error > > returned > > when no one's bound to the port. > > > > Instead virtio-vsock returns ECONNRESET in the last case. > > I'm not sure it's correct (looking at the man page connect(2) it > > would > > seem not), but if I understood correctly VMCI returns the same > > ECONNRESET in this case. > > > > > In any case, the reference for virtio-vsock semantics is: > > > 1. How does VMCI (VMware) vsock behave? We strive to be compatible > > > with > > > the VMCI transport. > > > > Looking at the code, it looks like VMCI returns ECONNRESET in this > > case, > > so this patch should be okay. (I haven't tried it) > > > > > 2. If there is no clear VMCI behavior, then we look at TCP/IP > > > because > > > those semantics are expected by most applications. > > > > > > This bug needs a test case in tools/testings/vsock/ and that test > > > case > > > will run against VMCI, virtio-vsock, and Hyper-V. Doing that will > > > answer the question of how VMCI handles this case. > > > > I agree. > > I'm trying to write the test but I'm kinda stuck as I have no way to Great! > ensure the proper timing between the test on the server(guest) and the > test on from the client(host). > > Basically I was thinking about creating a new test reusing > test_stream_connection_reset() from vsock_test.c. The reused function > would be used for the run_client callback, while I would define a new > function for the run_server. I wanted to basically do a bind only, and > don't go up to the listen/accept calls. > Problem is, the server won't block after the bind is done, and I don't > know how much the test should wait between the bind and the end. > I would like to show that even when the server reaches the point where > the socket is bound, the connect will still return with ECONNRESET. The > same test without the patch would simply hang till we hit the timeout > on the client side. > Does that make sense? And how much time should I wait for after the > bind on the server side? You can use control_writeln() and control_expectln() from control.c to syncronize server and client. The control path uses a TCP/IP socket. You can do something like this: server client s = socket(); s = socket(); bind(s, ...); control_writeln("BIND"); control_expectln("BIND"); connect(s, ...); # check ret and errno control_writeln("DONE"); control_expectln("DONE"); close(s); close(s); Cheers, Stefano ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] net: virtio_vsock: Fix race condition between bind and listen 2020-02-13 17:14 ` Stefano Garzarella @ 2020-02-13 17:18 ` Boeuf, Sebastien 0 siblings, 0 replies; 13+ messages in thread From: Boeuf, Sebastien @ 2020-02-13 17:18 UTC (permalink / raw) To: sgarzare; +Cc: stefanha, netdev, davem On Thu, 2020-02-13 at 18:14 +0100, Stefano Garzarella wrote: > On Thu, Feb 13, 2020 at 5:51 PM Boeuf, Sebastien < > sebastien.boeuf@intel.com> wrote: > > On Thu, 2020-02-13 at 14:04 +0100, Stefano Garzarella wrote: > > > On Thu, Feb 13, 2020 at 11:39:49AM +0000, Stefan Hajnoczi wrote: > > > > On Thu, Feb 13, 2020 at 10:44:18AM +0000, Boeuf, Sebastien > > > > wrote: > > > > > On Thu, 2020-02-13 at 11:22 +0100, Stefano Garzarella wrote: > > > > > > On Thu, Feb 13, 2020 at 09:51:36AM +0000, Boeuf, Sebastien > > > > > > wrote: > > > > > > > Hi Stefano, > > > > > > > > > > > > > > On Thu, 2020-02-13 at 10:41 +0100, Stefano Garzarella > > > > > > > wrote: > > > > > > > > Hi Sebastien, > > > > > > > > > > > > > > > > On Thu, Feb 13, 2020 at 09:16:11AM +0000, Boeuf, > > > > > > > > Sebastien > > > > > > > > wrote: > > > > > > > > > From 2f1276d02f5a12d85aec5adc11dfe1eab7e160d6 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: Fix race > > > > > > > > > condition > > > > > > > > > between > > > > > > > > > bind > > > > > > > > > and listen > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > will > > > > > > > > > effectively never recover. > > > > > > > > > > > > > > > > Do you have a test case? > > > > > > > > > > > > > > Yes I do. This has been a bug we've been investigating on > > > > > > > Kata > > > > > > > Containers for quite some time now. This was happening > > > > > > > when > > > > > > > using > > > > > > > Kata > > > > > > > along with Cloud-Hypervisor (which rely on the hybrid > > > > > > > vsock > > > > > > > implementation from Firecracker). The thing is, this bug > > > > > > > is > > > > > > > very > > > > > > > hard > > > > > > > to reproduce and was happening for Kata because of the > > > > > > > connection > > > > > > > strategy. The kata-runtime tries to connect a million > > > > > > > times > > > > > > > after > > > > > > > it > > > > > > > started the VM, just hoping the kata-agent will start to > > > > > > > listen > > > > > > > from > > > > > > > the guest side at some point. > > > > > > > > > > > > Maybe is related to something else. I tried the following > > > > > > which > > > > > > should be > > > > > > your case simplified (IIUC): > > > > > > > > > > > > guest$ python > > > > > > import socket > > > > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > > > > s.bind((socket.VMADDR_CID_ANY, 1234)) > > > > > > > > > > > > host$ python > > > > > > import socket > > > > > > s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM) > > > > > > s.connect((3, 1234)) > > > > > > > > > > > > Traceback (most recent call last): > > > > > > File "<stdin>", line 1, in <module> > > > > > > TimeoutError: [Errno 110] Connection timed out > > > > > > > > > > Yes this is exactly the simplified case. But that's the > > > > > point, I > > > > > don't > > > > > think the timeout is the best way to go here. Because this > > > > > means > > > > > that > > > > > when we run into this case, the host side will wait for quite > > > > > some time > > > > > before retrying, which can cause a very long delay before the > > > > > communication with the guest is established. By simply > > > > > answering > > > > > the > > > > > host with a RST packet, we inform it that nobody's listening > > > > > on > > > > > the > > > > > guest side yet, therefore the host side will close and try > > > > > again. > > > > > > > > My expectation is that TCP/IP will produce ECONNREFUSED in this > > > > case but > > > > I haven't checked. Timing out is weird behavior. > > > > > > I just tried and yes, TCP/IP produces ECONNREFUSED. The same > > > error > > > returned > > > when no one's bound to the port. > > > > > > Instead virtio-vsock returns ECONNRESET in the last case. > > > I'm not sure it's correct (looking at the man page connect(2) it > > > would > > > seem not), but if I understood correctly VMCI returns the same > > > ECONNRESET in this case. > > > > > > > In any case, the reference for virtio-vsock semantics is: > > > > 1. How does VMCI (VMware) vsock behave? We strive to be > > > > compatible > > > > with > > > > the VMCI transport. > > > > > > Looking at the code, it looks like VMCI returns ECONNRESET in > > > this > > > case, > > > so this patch should be okay. (I haven't tried it) > > > > > > > 2. If there is no clear VMCI behavior, then we look at TCP/IP > > > > because > > > > those semantics are expected by most applications. > > > > > > > > This bug needs a test case in tools/testings/vsock/ and that > > > > test > > > > case > > > > will run against VMCI, virtio-vsock, and Hyper-V. Doing that > > > > will > > > > answer the question of how VMCI handles this case. > > > > > > I agree. > > > > I'm trying to write the test but I'm kinda stuck as I have no way > > to > > Great! > > > ensure the proper timing between the test on the server(guest) and > > the > > test on from the client(host). > > > > Basically I was thinking about creating a new test reusing > > test_stream_connection_reset() from vsock_test.c. The reused > > function > > would be used for the run_client callback, while I would define a > > new > > function for the run_server. I wanted to basically do a bind only, > > and > > don't go up to the listen/accept calls. > > Problem is, the server won't block after the bind is done, and I > > don't > > know how much the test should wait between the bind and the end. > > I would like to show that even when the server reaches the point > > where > > the socket is bound, the connect will still return with ECONNRESET. > > The > > same test without the patch would simply hang till we hit the > > timeout > > on the client side. > > Does that make sense? And how much time should I wait for after the > > bind on the server side? > > You can use control_writeln() and control_expectln() from control.c > to > syncronize server and client. The control path uses a TCP/IP socket. Oh that's cool! I completely missed this feature :) Let me resume my work then! > > You can do something like this: > > server client > > s = socket(); s = socket(); > bind(s, ...); > control_writeln("BIND"); > control_expectln("BIND"); > connect(s, ...); > # check ret and errno > control_writeln("DONE"); > control_expectln("DONE"); > close(s); close(s); > > Cheers, > Stefano > --------------------------------------------------------------------- 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-02-13 17:19 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-13 9:16 [PATCH] net: virtio_vsock: Fix race condition between bind and listen Boeuf, Sebastien 2020-02-13 9:41 ` Stefano Garzarella 2020-02-13 9:51 ` Boeuf, Sebastien 2020-02-13 10:22 ` Stefano Garzarella 2020-02-13 10:44 ` Boeuf, Sebastien 2020-02-13 11:02 ` Stefano Garzarella 2020-02-13 11:13 ` Boeuf, Sebastien 2020-02-13 11:22 ` Stefano Garzarella 2020-02-13 11:39 ` Stefan Hajnoczi 2020-02-13 13:04 ` Stefano Garzarella 2020-02-13 16:51 ` Boeuf, Sebastien 2020-02-13 17:14 ` Stefano Garzarella 2020-02-13 17:18 ` Boeuf, Sebastien
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.