lkml.org 
[lkml]   [2020]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next v3 0/4] vsock: Add flags field in the vsock address
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.

Thanks,
Stefano

>
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(vsock_addr_validate);
>

\
 
 \ /
  Last update: 2020-12-14 09:17    [W:0.083 / U:0.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site