On 14/12/2020 10:13, Stefano Garzarella wrote:

On Sat, Dec 12, 2020 at 09:16:08AM -0800, Jakub Kicinski wrote:

quoted

On Fri, 11 Dec 2020 16:24:13 +0100 Stefano Garzarella wrote:

quoted

On Fri, Dec 11, 2020 at 12:32:37PM +0200, Andra Paraschiv wrote:

quoted

vsock enables communication between virtual machines and the host 

they are

quoted

running on. Nested VMs can be setup to use vsock channels, as the 

multi

quoted

transport support has been available in the mainline since the v5.5 

Linux kernel

quoted

has been released.

Implicitly, if no host->guest vsock transport is loaded, all the 

vsock packets

quoted

are forwarded to the host. This behavior can be used to setup 

communication

quoted

channels between sibling VMs that are running on the same host. One 

example can

quoted

be the vsock channels that can be established within AWS Nitro 

Enclaves

quoted

(see Documentation/virt/ne_overview.rst).

To be able to explicitly mark a connection as being used for a 

certain use case,

quoted

add a flags field in the vsock address data structure. The value of 

the flags

quoted

field is taken into consideration when the vsock transport is 

assigned. This way

quoted

can distinguish between different use cases, such as nested VMs / 

local

quoted

communication and sibling VMs.

The flags field can be set in the user space application connect 

logic. On the

quoted

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).

quoted

     +       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?

Right, I assumed the entire svm_zero[] was zeroed out, but we can't be
sure.

So, I agree to change the svm_flags to 1 byte (__u8), and remove the
superseded check that you pointed out.
With these changes we should be fully binary compatibility.

Here we go, sent out v4:

https://lore.kernel.org/lkml/20201214161122.37717-1-andraprs@amazon.com/ (local)

Thank you both.

Andra

quoted

quoted

             return 0;
      }
      EXPORT_SYMBOL_GPL(vsock_addr_validate);

Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.