Re: [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path

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

On 01/12/2020 18:22, Stefano Garzarella wrote:
>
> On Tue, Dec 01, 2020 at 05:25:04PM +0200, Andra Paraschiv wrote:
>> The vsock flag can be set during the connect() setup logic, when
>> initializing the vsock address data structure variable. Then the vsock
>> transport is assigned, also considering this flag.
>>
>> The vsock transport is also assigned on the (listen) receive path. The
>> flag needs to be set considering the use case.
>>
>> Set the vsock flag of the remote address to the one targeted for sibling
>> VMs communication if the following conditions are met:
>>
>> * The source CID of the packet is higher than VMADDR_CID_HOST.
>> * The destination CID of the packet is higher than VMADDR_CID_HOST.
>>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>> ---
>> net/vmw_vsock/virtio_transport_common.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c 
>> b/net/vmw_vsock/virtio_transport_common.c
>> index 5956939eebb78..871c84e0916b1 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -1062,6 +1062,14 @@ virtio_transport_recv_listen(struct sock *sk, 
>> struct virtio_vsock_pkt *pkt,
>>       vsock_addr_init(&vchild->remote_addr, 
>> le64_to_cpu(pkt->hdr.src_cid),
>>                       le32_to_cpu(pkt->hdr.src_port));
>>
>
> Maybe is better to create an helper function that other transports can
> use for the same purpose or we can put this code in the
> vsock_assign_transport() and set this flag only when the 'psk' argument
> is not NULL (this is the case when it's called by the transports when we
> receive a new connection request and 'psk' is the listener socket).
>
> The second way should allow us to support all the transports without
> touching them.

Ack, I was wondering about the other transports such as vmci or hyperv.

I can move the logic below in the codebase that assigns the transport, 
after checking 'psk'.

>
>> +      /* If the packet is coming with the source and destination 
>> CIDs higher
>> +       * than VMADDR_CID_HOST, then a vsock channel should be 
>> established for
>> +       * sibling VMs communication.
>> +       */
>> +      if (vchild->local_addr.svm_cid > VMADDR_CID_HOST &&
>> +          vchild->remote_addr.svm_cid > VMADDR_CID_HOST)
>> +              vchild->remote_addr.svm_flag = 
>> VMADDR_FLAG_SIBLING_VMS_COMMUNICATION;
>
> svm_flag is always initialized to 0 in vsock_addr_init(), so this
> assignment is the first one and it's okay, but to avoid future issues
> I'd use |= here to set the flag.

Fair point. I was thinking more towards exclusive flags values 
(purposes), but that's fine with the bitwise operator if we would get a 
set of flag values together. I will also update the field name to 
'svm_flags', let me know if we should keep the previous one or there is 
a better option.

Thanks,
Andra

>
>> +
>>       ret = vsock_assign_transport(vchild, vsk);
>>       /* Transport assigned (looking at remote_addr) must be the same
>>        * where we received the request.
>> -- 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.