Messages in this thread | | | Date | Mon, 14 Dec 2020 09:13:12 +0100 | From | Stefano Garzarella <> | Subject | Re: [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); >
| |