On Fri, 11 Dec 2020 16:24:13 +0100 Stefano Garzarella wrote:
> On Fri, Dec 11, 2020 at 12:32:37PM +0200, Andra Paraschiv wrote:
> >vsock enables communication between virtual machines and the host they are
> >running on. Nested VMs can be setup to use vsock channels, as the multi
> >transport support has been available in the mainline since the v5.5 Linux
> >kernel
> >has been released.
> >
> >Implicitly, if no host->guest vsock transport is loaded, all the vsock
> >packets
> >are forwarded to the host. This behavior can be used to setup communication
> >channels between sibling VMs that are running on the same host. One example
> >can
> >be the vsock channels that can be established within AWS Nitro Enclaves
> >(see Documentation/virt/ne_overview.rst).
> >
> >To be able to explicitly mark a connection as being used for a certain use
> >case,
> >add a flags field in the vsock address data structure. The value of the flags
> >field is taken into consideration when the vsock transport is assigned. This
> >way
> >can distinguish between different use cases, such as nested VMs / local
> >communication and sibling VMs.
> >
> >The flags field can be set in the user space application connect logic. On
> >the
> >listen path, the field can be set in the kernel space logic.
> >
>
> I reviewed all the patches and they are in a good shape!
>
> Maybe the last thing to add is a flags check in the
> vsock_addr_validate(), to avoid that flags that we don't know how to
> handle are specified.
> For example if in the future we add new flags that this version of the
> kernel is not able to satisfy, we should return an error to the
> application.
>
> I mean something like this:
>
> diff --git a/net/vmw_vsock/vsock_addr.c b/net/vmw_vsock/vsock_addr.c
> index 909de26cb0e7..73bb1d2fa526 100644
> --- a/net/vmw_vsock/vsock_addr.c
> +++ b/net/vmw_vsock/vsock_addr.c
> @@ -22,6 +22,8 @@ EXPORT_SYMBOL_GPL(vsock_addr_init);
>
> int vsock_addr_validate(const struct sockaddr_vm *addr)
> {
> + unsigned short svm_valid_flags = VMADDR_FLAG_TO_HOST;
> +
> if (!addr)
> return -EFAULT;
>
> @@ -31,6 +33,9 @@ int vsock_addr_validate(const struct sockaddr_vm *addr)
> if (addr->svm_zero[0] != 0)
> return -EINVAL;
Strictly speaking this check should be superseded by the check below
(AKA removed). We used to check svm_zero[0], with the new field added
this now checks svm_zero[2]. Old applications may have not initialized
svm_zero[2] (we're talking about binary compatibility here, apps built
with old headers).
> + if (addr->svm_flags & ~svm_valid_flags)
> + return -EINVAL;
The flags should also probably be one byte (we can define a "more
flags" flag to unlock further bytes) - otherwise on big endian the
new flag will fall into svm_zero[1] so the v3 improvements are moot
for big endian, right?
> return 0;
> }
> EXPORT_SYMBOL_GPL(vsock_addr_validate);
