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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201202085345.jfzxuxbeoics6f2a@steredhat>
Date:   Wed, 2 Dec 2020 09:53:45 +0100
From:   Stefano Garzarella <sgarzare@...hat.com>
To:     "Paraschiv, Andra-Irina" <andraprs@...zon.com>
Cc:     netdev <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        David Duncan <davdunc@...zon.com>,
        Dexuan Cui <decui@...rosoft.com>,
        Alexander Graf <graf@...zon.de>,
        Jorgen Hansen <jhansen@...are.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: Re: [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs
 flag on the receive path

On Tue, Dec 01, 2020 at 09:01:05PM +0200, Paraschiv, Andra-Irina wrote:
>
>
>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@...zon.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.

Yeah, maybe in the future we will add some new flags and we'll only need 
to add them without touching this code.

Agree with the new 'svm_flags' field name.

Thanks,
Stefano