| Message ID | 20201201152505.19445-3-andraprs@amazon.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock: Add flag field in the vsock address | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 85 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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. >+ /* 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. Thanks, Stefano >+ > 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. >
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.
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@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. 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
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)); + /* 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; + ret = vsock_assign_transport(vchild, vsk); /* Transport assigned (looking at remote_addr) must be the same * where we received the request.
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(+)