Re: [PATCH net-next v3 0/4] vsock: Add flags field in the vsock address

["Paraschiv, Andra-Irina" <andraprs@amazon.com>]

On 14/12/2020 10:13, Stefano Garzarella wrote:
>
> On Sat, Dec 12, 2020 at 09:16:08AM -0800, Jakub Kicinski wrote:
>> 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?
>
> 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/

Thank you both.

Andra

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