2020-12-01 15:29:21

by Paraschiv, Andra-Irina

[permalink] [raw]
Subject: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address

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 flag field in the vsock address data structure. The "svm_reserved1" field
has been repurposed to be the flag field. The value of the flag will then be
taken into consideration when the vsock transport is assigned.

This way can distinguish between nested VMs / local communication and sibling
VMs use cases. And can also setup one or more types of communication at the same
time.

Thank you.

Andra

---

Patch Series Changelog

The patch series is built on top of v5.10-rc6.

GitHub repo branch for the latest version of the patch series:

* https://github.com/andraprs/linux/tree/vsock-flag-sibling-comm-v1

---

Andra Paraschiv (3):
vm_sockets: Include flag field in the vsock address data structure
virtio_transport_common: Set sibling VMs flag on the receive path
af_vsock: Assign the vsock transport considering the vsock address
flag

include/uapi/linux/vm_sockets.h | 18 +++++++++++++++++-
net/vmw_vsock/af_vsock.c | 15 +++++++++++----
net/vmw_vsock/virtio_transport_common.c | 8 ++++++++
3 files changed, 36 insertions(+), 5 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.


2020-12-01 15:29:42

by Paraschiv, Andra-Irina

[permalink] [raw]
Subject: [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag

The vsock flag has been set in the connect and (listen) receive paths.

When the vsock transport is assigned, the remote CID is used to
distinguish between types of connection.

Use the vsock flag (in addition to the CID) from the remote address to
decide which vsock transport to assign. For the sibling VMs use case,
all the vsock packets need to be forwarded to the host, so always assign
the guest->host transport if the vsock flag is set. For the other use
cases, the vsock transport assignment logic is not changed.

Signed-off-by: Andra Paraschiv <[email protected]>
---
net/vmw_vsock/af_vsock.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index d10916ab45267..bafc1cb20abd4 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -419,16 +419,21 @@ static void vsock_deassign_transport(struct vsock_sock *vsk)
* (e.g. during the connect() or when a connection request on a listener
* socket is received).
* The vsk->remote_addr is used to decide which transport to use:
- * - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or VMADDR_CID_HOST if
- * g2h is not loaded, will use local transport;
- * - remote CID <= VMADDR_CID_HOST will use guest->host transport;
- * - remote CID > VMADDR_CID_HOST will use host->guest transport;
+ * - remote flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION, will always
+ * forward the vsock packets to the host and use guest->host transport;
+ * - otherwise, going forward with the remote flag default value:
+ * - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or VMADDR_CID_HOST
+ * if g2h is not loaded, will use local transport;
+ * - remote CID <= VMADDR_CID_HOST or h2g is not loaded, will use
+ * guest->host transport;
+ * - remote CID > VMADDR_CID_HOST will use host->guest transport;
*/
int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
{
const struct vsock_transport *new_transport;
struct sock *sk = sk_vsock(vsk);
unsigned int remote_cid = vsk->remote_addr.svm_cid;
+ unsigned short remote_flag = vsk->remote_addr.svm_flag;
int ret;

switch (sk->sk_type) {
@@ -438,6 +443,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
case SOCK_STREAM:
if (vsock_use_local_transport(remote_cid))
new_transport = transport_local;
+ else if (remote_flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION)
+ new_transport = transport_g2h;
else if (remote_cid <= VMADDR_CID_HOST || !transport_h2g)
new_transport = transport_g2h;
else
--
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.

2020-12-01 15:29:46

by Paraschiv, Andra-Irina

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

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 <[email protected]>
---
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));

+ /* 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.
--
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.

2020-12-01 16:26:14

by Stefano Garzarella

[permalink] [raw]
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 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 <[email protected]>
>---
> 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.
>

2020-12-01 16:27:38

by Stefano Garzarella

[permalink] [raw]
Subject: Re: [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag

On Tue, Dec 01, 2020 at 05:25:05PM +0200, Andra Paraschiv wrote:
>The vsock flag has been set in the connect and (listen) receive paths.
>
>When the vsock transport is assigned, the remote CID is used to
>distinguish between types of connection.
>
>Use the vsock flag (in addition to the CID) from the remote address to
>decide which vsock transport to assign. For the sibling VMs use case,
>all the vsock packets need to be forwarded to the host, so always assign
>the guest->host transport if the vsock flag is set. For the other use
>cases, the vsock transport assignment logic is not changed.
>
>Signed-off-by: Andra Paraschiv <[email protected]>
>---
> net/vmw_vsock/af_vsock.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index d10916ab45267..bafc1cb20abd4 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -419,16 +419,21 @@ static void vsock_deassign_transport(struct vsock_sock *vsk)
> * (e.g. during the connect() or when a connection request on a listener
> * socket is received).
> * The vsk->remote_addr is used to decide which transport to use:
>- * - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or VMADDR_CID_HOST if
>- * g2h is not loaded, will use local transport;
>- * - remote CID <= VMADDR_CID_HOST will use guest->host transport;
>- * - remote CID > VMADDR_CID_HOST will use host->guest transport;
>+ * - remote flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION, will always
>+ * forward the vsock packets to the host and use guest->host transport;
>+ * - otherwise, going forward with the remote flag default value:
>+ * - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or VMADDR_CID_HOST
>+ * if g2h is not loaded, will use local transport;
>+ * - remote CID <= VMADDR_CID_HOST or h2g is not loaded, will use
>+ * guest->host transport;
>+ * - remote CID > VMADDR_CID_HOST will use host->guest transport;
> */
> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> {
> const struct vsock_transport *new_transport;
> struct sock *sk = sk_vsock(vsk);
> unsigned int remote_cid = vsk->remote_addr.svm_cid;
>+ unsigned short remote_flag = vsk->remote_addr.svm_flag;
> int ret;
>
> switch (sk->sk_type) {
>@@ -438,6 +443,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> case SOCK_STREAM:
> if (vsock_use_local_transport(remote_cid))
> new_transport = transport_local;
>+ else if (remote_flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION)

Others flags can be added, so here we should use the bitwise AND
operator to check if this flag is set.

And what about merging with the next if clause?


Thanks,
Stefano

>+ new_transport = transport_g2h;
> else if (remote_cid <= VMADDR_CID_HOST ||
> !transport_h2g)
> new_transport = transport_g2h;
> else
>--
>2.20.1 (Apple Git-117)
>

2020-12-01 16:34:02

by Stefano Garzarella

[permalink] [raw]
Subject: Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address

Hi Andra,

On Tue, Dec 01, 2020 at 05:25:02PM +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 flag field in the vsock address data structure. The "svm_reserved1" field
>has been repurposed to be the flag field. The value of the flag will then be
>taken into consideration when the vsock transport is assigned.
>
>This way can distinguish between nested VMs / local communication and sibling
>VMs use cases. And can also setup one or more types of communication at the same
>time.

Thanks to work on this, I've left you a few comments, but I think this
is the right way to support nested and sibling communication together.

Thank you,
Stefano

2020-12-01 18:35:07

by Paraschiv, Andra-Irina

[permalink] [raw]
Subject: Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address



On 01/12/2020 18:27, Stefano Garzarella wrote:
>
>
> Hi Andra,
>
> On Tue, Dec 01, 2020 at 05:25:02PM +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 flag field in the vsock address data structure. The
>> "svm_reserved1" field
>> has been repurposed to be the flag field. The value of the flag will
>> then be
>> taken into consideration when the vsock transport is assigned.
>>
>> This way can distinguish between nested VMs / local communication and
>> sibling
>> VMs use cases. And can also setup one or more types of communication
>> at the same
>> time.
>
> Thanks to work on this, I've left you a few comments, but I think this
> is the right way to support nested and sibling communication together.

Hi Stefano,

Thanks also for taking time to review and both you and Stefan for
sharing an overview of this proposed option.

I'm going through the comments and will send out the v2 of the patch
series as I have the changes done and validated.

Thanks,
Andra



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.

2020-12-01 19:46:50

by Paraschiv, Andra-Irina

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



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 <[email protected]>
>> ---
>> 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.

2020-12-01 19:53:19

by Paraschiv, Andra-Irina

[permalink] [raw]
Subject: Re: [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag



On 01/12/2020 18:23, Stefano Garzarella wrote:
>
> On Tue, Dec 01, 2020 at 05:25:05PM +0200, Andra Paraschiv wrote:
>> The vsock flag has been set in the connect and (listen) receive paths.
>>
>> When the vsock transport is assigned, the remote CID is used to
>> distinguish between types of connection.
>>
>> Use the vsock flag (in addition to the CID) from the remote address to
>> decide which vsock transport to assign. For the sibling VMs use case,
>> all the vsock packets need to be forwarded to the host, so always assign
>> the guest->host transport if the vsock flag is set. For the other use
>> cases, the vsock transport assignment logic is not changed.
>>
>> Signed-off-by: Andra Paraschiv <[email protected]>
>> ---
>> net/vmw_vsock/af_vsock.c | 15 +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index d10916ab45267..bafc1cb20abd4 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -419,16 +419,21 @@ static void vsock_deassign_transport(struct
>> vsock_sock *vsk)
>>  * (e.g. during the connect() or when a connection request on a listener
>>  * socket is received).
>>  * The vsk->remote_addr is used to decide which transport to use:
>> - *  - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or
>> VMADDR_CID_HOST if
>> - *    g2h is not loaded, will use local transport;
>> - *  - remote CID <= VMADDR_CID_HOST will use guest->host transport;
>> - *  - remote CID > VMADDR_CID_HOST will use host->guest transport;
>> + *  - remote flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION, will always
>> + *    forward the vsock packets to the host and use guest->host
>> transport;
>> + *  - otherwise, going forward with the remote flag default value:
>> + *    - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or
>> VMADDR_CID_HOST
>> + *      if g2h is not loaded, will use local transport;
>> + *    - remote CID <= VMADDR_CID_HOST or h2g is not loaded, will use
>> + *      guest->host transport;
>> + *    - remote CID > VMADDR_CID_HOST will use host->guest transport;
>>  */
>> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock
>> *psk)
>> {
>>       const struct vsock_transport *new_transport;
>>       struct sock *sk = sk_vsock(vsk);
>>       unsigned int remote_cid = vsk->remote_addr.svm_cid;
>> +      unsigned short remote_flag = vsk->remote_addr.svm_flag;
>>       int ret;
>>
>>       switch (sk->sk_type) {
>> @@ -438,6 +443,8 @@ int vsock_assign_transport(struct vsock_sock
>> *vsk, struct vsock_sock *psk)
>>       case SOCK_STREAM:
>>               if (vsock_use_local_transport(remote_cid))
>>                       new_transport = transport_local;
>> +              else if (remote_flag ==
>> VMADDR_FLAG_SIBLING_VMS_COMMUNICATION)
>
> Others flags can be added, so here we should use the bitwise AND
> operator to check if this flag is set.
>
> And what about merging with the next if clause?
>

Indeed, I'll update the codebase to use the bitwise operator. Then I can
also merge all the checks corresponding to the g2h transport in a single
if block.

Thanks,
Andra

>
>> +                      new_transport = transport_g2h;
>>               else if (remote_cid <= VMADDR_CID_HOST ||
>>               !transport_h2g)
>>                       new_transport = transport_g2h;
>>               else
>> --
>> 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.

2020-12-02 08:58:24

by Stefano Garzarella

[permalink] [raw]
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 <[email protected]>
>>>---
>>>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

2020-12-02 13:42:39

by Stefano Garzarella

[permalink] [raw]
Subject: Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address

Hi Andra,

On Tue, Dec 01, 2020 at 05:25:02PM +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 flag field in the vsock address data structure. The "svm_reserved1" field
>has been repurposed to be the flag field. The value of the flag will then be
>taken into consideration when the vsock transport is assigned.
>
>This way can distinguish between nested VMs / local communication and sibling
>VMs use cases. And can also setup one or more types of communication at the same
>time.
>

Another thing worth mentioning is that for now it is not supported in
vhost-vsock, since we are discarding every packet not addressed to the
host.

What we should do would be:
- add a new IOCTL to vhost-vsock to enable sibling communication, by
default I'd like to leave it disabled

- allow sibling forwarding only if both guests have sibling
communication enabled and we should implement some kind of filtering
or network namespace support to allow the communication only between a
subset of VMs


Do you have plans to work on it?

Otherwise I put it in my to-do list and hope I have time to do it (maybe
next month).

Thanks,
Stefano

2020-12-02 16:22:28

by Paraschiv, Andra-Irina

[permalink] [raw]
Subject: Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address



On 02/12/2020 15:37, Stefano Garzarella wrote:
>
> Hi Andra,
>
> On Tue, Dec 01, 2020 at 05:25:02PM +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 flag field in the vsock address data structure. The
>> "svm_reserved1" field
>> has been repurposed to be the flag field. The value of the flag will
>> then be
>> taken into consideration when the vsock transport is assigned.
>>
>> This way can distinguish between nested VMs / local communication and
>> sibling
>> VMs use cases. And can also setup one or more types of communication
>> at the same
>> time.
>>
>
> Another thing worth mentioning is that for now it is not supported in
> vhost-vsock, since we are discarding every packet not addressed to the
> host.

Right, thanks for the follow-up.

>
> What we should do would be:
> - add a new IOCTL to vhost-vsock to enable sibling communication, by
>   default I'd like to leave it disabled
>
> - allow sibling forwarding only if both guests have sibling
>   communication enabled and we should implement some kind of filtering
>   or network namespace support to allow the communication only between a
>   subset of VMs
>
>
> Do you have plans to work on it?

Nope, not yet. But I can take some time in the second part of December /
beginning of January for this. And we can catch up in the meantime if
there is something blocking or more clarifications are needed to make it
work.

Thanks,
Andra

>
>
> Otherwise I put it in my to-do list and hope I have time to do it (maybe
> next month).
>
> Thanks,
> Stefano
>




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.

2020-12-03 08:56:57

by Stefano Garzarella

[permalink] [raw]
Subject: Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address

On Wed, Dec 02, 2020 at 06:18:15PM +0200, Paraschiv, Andra-Irina wrote:
>
>
>On 02/12/2020 15:37, Stefano Garzarella wrote:
>>
>>Hi Andra,
>>
>>On Tue, Dec 01, 2020 at 05:25:02PM +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 flag field in the vsock address data structure. The
>>>"svm_reserved1" field
>>>has been repurposed to be the flag field. The value of the flag
>>>will then be
>>>taken into consideration when the vsock transport is assigned.
>>>
>>>This way can distinguish between nested VMs / local communication
>>>and sibling
>>>VMs use cases. And can also setup one or more types of
>>>communication at the same
>>>time.
>>>
>>
>>Another thing worth mentioning is that for now it is not supported in
>>vhost-vsock, since we are discarding every packet not addressed to the
>>host.
>
>Right, thanks for the follow-up.
>
>>
>>What we should do would be:
>>- add a new IOCTL to vhost-vsock to enable sibling communication, by
>>? default I'd like to leave it disabled
>>
>>- allow sibling forwarding only if both guests have sibling
>>? communication enabled and we should implement some kind of filtering
>>? or network namespace support to allow the communication only between a
>>? subset of VMs
>>
>>
>>Do you have plans to work on it?
>
>Nope, not yet. But I can take some time in the second part of December
>/ beginning of January for this. And we can catch up in the meantime
>if there is something blocking or more clarifications are needed to
>make it work.
>

Good, it will be great!

Thanks,
Stefano