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

[Paraschiv, Andra-Irina]

On 11/12/2020 17:24, Stefano Garzarella wrote:
> Hi Andra,
>
> 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!

Hi Stefano,

Thanks for the overall review and for the reconfirmation of the Rb for 
the vsock address data structure changes.

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

I can add this validation as a new patch in the series, next revision.

Thanks,
Andra

> 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;
>
>     +       if (addr->svm_flags & ~svm_valid_flags)
>     +               return -EINVAL;
>     +
>             return 0;
>      }
>      EXPORT_SYMBOL_GPL(vsock_addr_validate);
>
>
> Thanks,
> Stefano
>
> > Thank you.
> >
> > Andra
> >
> > ---
> >
> > Patch Series Changelog
> >
> > The patch series is built on top of v5.10-rc7.
> >
> > GitHub repo branch for the latest version of the patch series:
> >
> > * https://github.com/andraprs/linux/tree/vsock-flag-sibling-comm-v3
> >
> > v2 -> v3
> >
> > * Rebase on top of v5.10-rc7.
> > * Add "svm_flags" as a new field, not reusing "svm_reserved1".
> > * Update comments to mention when the "VMADDR_FLAG_TO_HOST" flag is 
> > set in the
> >  connect and listen paths.
> > * Update bitwise check logic to not compare result to the flag value.
> > * v2: 
> > https://lore.kernel.org/lkml/20201204170235.84387-1-andraprs@xxxxxxxxxx/
> >
> > v1 -> v2
> >
> > * Update the vsock flag naming to "VMADDR_FLAG_TO_HOST".
> > * Use bitwise operators to setup and check the vsock flag.
> > * Set the vsock flag on the receive path in the vsock transport 
> > assignment
> >  logic.
> > * Merge the checks for the g2h transport assignment in one "if" block.
> > * v1: 
> > https://lore.kernel.org/lkml/20201201152505.19445-1-andraprs@xxxxxxxxxx/
> >
> > ---
> >
> > Andra Paraschiv (4):
> >  vm_sockets: Add flags field in the vsock address data structure
> >  vm_sockets: Add VMADDR_FLAG_TO_HOST vsock flag
> >  af_vsock: Set VMADDR_FLAG_TO_HOST flag on the receive path
> >  af_vsock: Assign the vsock transport considering the vsock address
> >    flags
> >
> > include/uapi/linux/vm_sockets.h | 25 ++++++++++++++++++++++++-
> > net/vmw_vsock/af_vsock.c        | 21 +++++++++++++++++++--
> > 2 files changed, 43 insertions(+), 3 deletions(-)
> >
> > --
> > 2.20.1 (Apple Git-117)
> >
> >
> >
> >
> > 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.




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.