diff mbox series

[SRU,1/1,aws/groovy] vsock: forward all packets to the host when no H2G is registered

Message ID 20201201165459.9564-2-kamal@canonical.com
State New
Headers show
Series vsock fix for nitro_enclaves | expand

Commit Message

Kamal Mostafa Dec. 1, 2020, 4:54 p.m. UTC
From: Stefano Garzarella <sgarzare@redhat.com>

BugLink: https://bugs.launchpad.net/bugs/1903087

Before commit c0cfa2d8a788 ("vsock: add multi-transports support"),
if a G2H transport was loaded (e.g. virtio transport), every packets
was forwarded to the host, regardless of the destination CID.
The H2G transports implemented until then (vhost-vsock, VMCI) always
responded with an error, if the destination CID was not
VMADDR_CID_HOST.

From that commit, we are using the remote CID to decide which
transport to use, so packets with remote CID > VMADDR_CID_HOST(2)
are sent only through H2G transport. If no H2G is available, packets
are discarded directly in the guest.

Some use cases (e.g. Nitro Enclaves [1]) rely on the old behaviour
to implement sibling VMs communication, so we restore the old
behavior when no H2G is registered.
It will be up to the host to discard packets if the destination is
not the right one. As it was already implemented before adding
multi-transport support.

Tested with nested QEMU/KVM by me and Nitro Enclaves by Andra.

[1] Documentation/virt/ne_overview.rst

Cc: Jorgen Hansen <jhansen@vmware.com>
Cc: Dexuan Cui <decui@microsoft.com>
Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Reported-by: Andra Paraschiv <andraprs@amazon.com>
Tested-by: Andra Paraschiv <andraprs@amazon.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Link: https://lore.kernel.org/r/20201112133837.34183-1-sgarzare@redhat.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
(cherry picked from commit 65b422d9b61ba12c08150784e8012fa1892ad03e)
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
---
 net/vmw_vsock/af_vsock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrea Righi Dec. 1, 2020, 5:10 p.m. UTC | #1
On Tue, Dec 01, 2020 at 08:54:59AM -0800, Kamal Mostafa wrote:
> From: Stefano Garzarella <sgarzare@redhat.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1903087
> 
> Before commit c0cfa2d8a788 ("vsock: add multi-transports support"),
> if a G2H transport was loaded (e.g. virtio transport), every packets
> was forwarded to the host, regardless of the destination CID.
> The H2G transports implemented until then (vhost-vsock, VMCI) always
> responded with an error, if the destination CID was not
> VMADDR_CID_HOST.
> 
> From that commit, we are using the remote CID to decide which
> transport to use, so packets with remote CID > VMADDR_CID_HOST(2)
> are sent only through H2G transport. If no H2G is available, packets
> are discarded directly in the guest.
> 
> Some use cases (e.g. Nitro Enclaves [1]) rely on the old behaviour
> to implement sibling VMs communication, so we restore the old
> behavior when no H2G is registered.
> It will be up to the host to discard packets if the destination is
> not the right one. As it was already implemented before adding
> multi-transport support.
> 
> Tested with nested QEMU/KVM by me and Nitro Enclaves by Andra.
> 
> [1] Documentation/virt/ne_overview.rst
> 
> Cc: Jorgen Hansen <jhansen@vmware.com>
> Cc: Dexuan Cui <decui@microsoft.com>
> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
> Reported-by: Andra Paraschiv <andraprs@amazon.com>
> Tested-by: Andra Paraschiv <andraprs@amazon.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> Link: https://lore.kernel.org/r/20201112133837.34183-1-sgarzare@redhat.com
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> (cherry picked from commit 65b422d9b61ba12c08150784e8012fa1892ad03e)
> Signed-off-by: Kamal Mostafa <kamal@canonical.com>

Clean upstream cherry pick. Looks good to me, thanks!

Acked-by: Andrea Righi <andrea.righi@canonical.com>
Ian May Dec. 2, 2020, 4:37 p.m. UTC | #2
Applied to groovy/linux

Thanks,
Ian

On 2020-12-01 08:54:59 , Kamal Mostafa wrote:
> From: Stefano Garzarella <sgarzare@redhat.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1903087
> 
> Before commit c0cfa2d8a788 ("vsock: add multi-transports support"),
> if a G2H transport was loaded (e.g. virtio transport), every packets
> was forwarded to the host, regardless of the destination CID.
> The H2G transports implemented until then (vhost-vsock, VMCI) always
> responded with an error, if the destination CID was not
> VMADDR_CID_HOST.
> 
> From that commit, we are using the remote CID to decide which
> transport to use, so packets with remote CID > VMADDR_CID_HOST(2)
> are sent only through H2G transport. If no H2G is available, packets
> are discarded directly in the guest.
> 
> Some use cases (e.g. Nitro Enclaves [1]) rely on the old behaviour
> to implement sibling VMs communication, so we restore the old
> behavior when no H2G is registered.
> It will be up to the host to discard packets if the destination is
> not the right one. As it was already implemented before adding
> multi-transport support.
> 
> Tested with nested QEMU/KVM by me and Nitro Enclaves by Andra.
> 
> [1] Documentation/virt/ne_overview.rst
> 
> Cc: Jorgen Hansen <jhansen@vmware.com>
> Cc: Dexuan Cui <decui@microsoft.com>
> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
> Reported-by: Andra Paraschiv <andraprs@amazon.com>
> Tested-by: Andra Paraschiv <andraprs@amazon.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> Link: https://lore.kernel.org/r/20201112133837.34183-1-sgarzare@redhat.com
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> (cherry picked from commit 65b422d9b61ba12c08150784e8012fa1892ad03e)
> Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> ---
>  net/vmw_vsock/af_vsock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 6cd0df1c5caf..5fa4b1ac3c96 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -438,7 +438,7 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>  	case SOCK_STREAM:
>  		if (vsock_use_local_transport(remote_cid))
>  			new_transport = transport_local;
> -		else if (remote_cid <= VMADDR_CID_HOST)
> +		else if (remote_cid <= VMADDR_CID_HOST || !transport_h2g)
>  			new_transport = transport_g2h;
>  		else
>  			new_transport = transport_h2g;
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff mbox series

Patch

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 6cd0df1c5caf..5fa4b1ac3c96 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -438,7 +438,7 @@  int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	case SOCK_STREAM:
 		if (vsock_use_local_transport(remote_cid))
 			new_transport = transport_local;
-		else if (remote_cid <= VMADDR_CID_HOST)
+		else if (remote_cid <= VMADDR_CID_HOST || !transport_h2g)
 			new_transport = transport_g2h;
 		else
 			new_transport = transport_h2g;