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

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

[Andra Paraschiv]

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 "svm_reserved1" field
has been repurposed to be the flags field. The value of the flags will then be
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.

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

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@amazon.com/

---

Andra Paraschiv (4):
  vm_sockets: Include 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 | 17 ++++++++++++++++-
 net/vmw_vsock/af_vsock.c        | 21 +++++++++++++++++++--
 2 files changed, 35 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.

[PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Andra Paraschiv]

vsock enables communication between virtual machines and the host they
are running on. With the multi transport support (guest->host and
host->guest), nested VMs can also use vsock channels for communication.

In addition to this, by default, all the vsock packets are forwarded to
the host, if no host->guest transport is loaded. This behavior can be
implicitly used for enabling vsock communication between sibling VMs.

Add a flags field in the vsock address data structure that can be used
to explicitly mark the vsock connection as being targeted for a certain
type of communication. This way, can distinguish between different use
cases such as nested VMs and sibling VMs.

Use the already available "svm_reserved1" field and mark it as a flags
field instead. This field can be set when initializing the vsock address
variable used for the connect() call.

Changelog

v1 -> v2

* Update the field name to "svm_flags".
* Split the current patch in 2 patches.

Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
---
 include/uapi/linux/vm_sockets.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
index fd0ed7221645d..46735376a57a8 100644
--- a/include/uapi/linux/vm_sockets.h
+++ b/include/uapi/linux/vm_sockets.h
@@ -145,7 +145,7 @@
 
 struct sockaddr_vm {
 	__kernel_sa_family_t svm_family;
-	unsigned short svm_reserved1;
+	unsigned short svm_flags;
 	unsigned int svm_port;
 	unsigned int svm_cid;
 	unsigned char svm_zero[sizeof(struct sockaddr) -
-- 
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.

[PATCH net-next v2 2/4] vm_sockets: Add VMADDR_FLAG_TO_HOST vsock flag

[Andra Paraschiv]

Add VMADDR_FLAG_TO_HOST vsock flag that is used to setup a vsock
connection where all the packets are forwarded to the host.

Then, using this type of vsock channel, vsock communication between
sibling VMs can be built on top of it.

Changelog

v1 -> v2

* New patch in v2, it was split from the first patch in the series.
* Remove the default value for the vsock flags field.
* Update the naming for the vsock flag to "VMADDR_FLAG_TO_HOST".

Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
---
 include/uapi/linux/vm_sockets.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
index 46735376a57a8..72e1a3d05682d 100644
--- a/include/uapi/linux/vm_sockets.h
+++ b/include/uapi/linux/vm_sockets.h
@@ -114,6 +114,21 @@
 
 #define VMADDR_CID_HOST 2
 
+/* The current default use case for the vsock channel is the following:
+ * local vsock communication between guest and host and nested VMs setup.
+ * In addition to this, implicitly, the vsock packets are forwarded to the host
+ * if no host->guest vsock transport is set.
+ *
+ * Set this flag value in the sockaddr_vm corresponding field if the vsock
+ * packets need to be always forwarded to the host. Using this behavior,
+ * vsock communication between sibling VMs can be setup.
+ *
+ * This way can explicitly distinguish between vsock channels created for
+ * different use cases, such as nested VMs (or local communication between
+ * guest and host) and sibling VMs.
+ */
+#define VMADDR_FLAG_TO_HOST 0x0001
+
 /* Invalid vSockets version. */
 
 #define VM_SOCKETS_INVALID_VERSION -1U
-- 
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.

[PATCH net-next v2 3/4] af_vsock: Set VMADDR_FLAG_TO_HOST flag on the receive path

[Andra Paraschiv]

The vsock flags 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 flags field.

The vsock transport is also assigned on the (listen) receive path. The
flags field needs to be set considering the use case.

Set the value of the vsock flags of the remote address to the one
targeted for packets forwarding to the host, 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.

Changelog

v1 -> v2

* Set the vsock flag on the receive path in the vsock transport
  assignment logic.
* Use bitwise operator for the vsock flag setup.
* Use the updated "VMADDR_FLAG_TO_HOST" flag naming.

Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
---
 net/vmw_vsock/af_vsock.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index d10916ab45267..83d035eab0b05 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -431,6 +431,18 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	unsigned int remote_cid = vsk->remote_addr.svm_cid;
 	int ret;
 
+	/* If the packet is coming with the source and destination CIDs higher
+	 * than VMADDR_CID_HOST, then a vsock channel where all the packets are
+	 * forwarded to the host should be established. Then the host will
+	 * need to forward the packets to the guest.
+	 *
+	 * The flag is set on the (listen) receive path (psk is not NULL). On
+	 * the connect path the flag can be set by the user space application.
+	 */
+	if (psk && vsk->local_addr.svm_cid > VMADDR_CID_HOST &&
+	    vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
+		vsk->remote_addr.svm_flags |= VMADDR_FLAG_TO_HOST;
+
 	switch (sk->sk_type) {
 	case SOCK_DGRAM:
 		new_transport = transport_dgram;
-- 
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.

[PATCH net-next v2 4/4] af_vsock: Assign the vsock transport considering the vsock address flags

[Andra Paraschiv]

The vsock flags field can be 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 flags value (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 VMADDR_FLAG_TO_HOST flag
is set. For the other use cases, the vsock transport assignment logic is
not changed.

Changelog

v1 -> v2

* Use bitwise operator to check the vsock flag.
* Use the updated "VMADDR_FLAG_TO_HOST" flag naming.
* Merge the checks for the g2h transport assignment in one "if" block.

Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
---
 net/vmw_vsock/af_vsock.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 83d035eab0b05..66e643c3b5f85 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -421,7 +421,8 @@ static void vsock_deassign_transport(struct vsock_sock *vsk)
  * 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 or h2g is not loaded or remote flags field
+ *    includes VMADDR_FLAG_TO_HOST flag value, 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)
@@ -429,6 +430,7 @@ 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_flags;
 	int ret;
 
 	/* If the packet is coming with the source and destination CIDs higher
@@ -443,6 +445,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	    vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
 		vsk->remote_addr.svm_flags |= VMADDR_FLAG_TO_HOST;
 
+	remote_flags = vsk->remote_addr.svm_flags;
+
 	switch (sk->sk_type) {
 	case SOCK_DGRAM:
 		new_transport = transport_dgram;
@@ -450,7 +454,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_cid <= VMADDR_CID_HOST || !transport_h2g)
+		else if (remote_cid <= VMADDR_CID_HOST || !transport_h2g ||
+			 (remote_flags & VMADDR_FLAG_TO_HOST) == VMADDR_FLAG_TO_HOST)
 			new_transport = transport_g2h;
 		else
 			new_transport = transport_h2g;
-- 
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.

Re: [PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Stefano Garzarella]

On Fri, Dec 04, 2020 at 07:02:32PM +0200, Andra Paraschiv wrote:
>vsock enables communication between virtual machines and the host they
>are running on. With the multi transport support (guest->host and
>host->guest), nested VMs can also use vsock channels for communication.
>
>In addition to this, by default, all the vsock packets are forwarded to
>the host, if no host->guest transport is loaded. This behavior can be
>implicitly used for enabling vsock communication between sibling VMs.
>
>Add a flags field in the vsock address data structure that can be used
>to explicitly mark the vsock connection as being targeted for a certain
>type of communication. This way, can distinguish between different use
>cases such as nested VMs and sibling VMs.
>
>Use the already available "svm_reserved1" field and mark it as a flags
>field instead. This field can be set when initializing the vsock address
>variable used for the connect() call.
>
>Changelog
>
>v1 -> v2
>
>* Update the field name to "svm_flags".
>* Split the current patch in 2 patches.

Usually the changelog goes after the 3 dashes, but I'm not sure there is 
a strict rule :-)

Anyway the patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>---
> include/uapi/linux/vm_sockets.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
>index fd0ed7221645d..46735376a57a8 100644
>--- a/include/uapi/linux/vm_sockets.h
>+++ b/include/uapi/linux/vm_sockets.h
>@@ -145,7 +145,7 @@
>
> struct sockaddr_vm {
> 	__kernel_sa_family_t svm_family;
>-	unsigned short svm_reserved1;
>+	unsigned short svm_flags;
> 	unsigned int svm_port;
> 	unsigned int svm_cid;
> 	unsigned char svm_zero[sizeof(struct sockaddr) -
>-- 
>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.
>

Re: [PATCH net-next v2 2/4] vm_sockets: Add VMADDR_FLAG_TO_HOST vsock flag

[Stefano Garzarella]

On Fri, Dec 04, 2020 at 07:02:33PM +0200, Andra Paraschiv wrote:
>Add VMADDR_FLAG_TO_HOST vsock flag that is used to setup a vsock
>connection where all the packets are forwarded to the host.
>
>Then, using this type of vsock channel, vsock communication between
>sibling VMs can be built on top of it.
>
>Changelog
>
>v1 -> v2
>
>* New patch in v2, it was split from the first patch in the series.
>* Remove the default value for the vsock flags field.
>* Update the naming for the vsock flag to "VMADDR_FLAG_TO_HOST".
>
>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>---
> include/uapi/linux/vm_sockets.h | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
>diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
>index 46735376a57a8..72e1a3d05682d 100644
>--- a/include/uapi/linux/vm_sockets.h
>+++ b/include/uapi/linux/vm_sockets.h
>@@ -114,6 +114,21 @@
>
> #define VMADDR_CID_HOST 2
>
>+/* The current default use case for the vsock channel is the following:
>+ * local vsock communication between guest and host and nested VMs setup.
>+ * In addition to this, implicitly, the vsock packets are forwarded to the host
>+ * if no host->guest vsock transport is set.
>+ *
>+ * Set this flag value in the sockaddr_vm corresponding field if the vsock
>+ * packets need to be always forwarded to the host. Using this behavior,
>+ * vsock communication between sibling VMs can be setup.

Maybe we can add a sentence saying that this flag is set on the remote 
peer address for an incoming connection when it is routed from the host 
(local CID and remote CID > VMADDR_CID_HOST).

>+ *
>+ * This way can explicitly distinguish between vsock channels created for
>+ * different use cases, such as nested VMs (or local communication between
>+ * guest and host) and sibling VMs.
>+ */
>+#define VMADDR_FLAG_TO_HOST 0x0001
>+
> /* Invalid vSockets version. */
>
> #define VM_SOCKETS_INVALID_VERSION -1U
>-- 
>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.
>

Re: [PATCH net-next v2 3/4] af_vsock: Set VMADDR_FLAG_TO_HOST flag on the receive path

[Stefano Garzarella]

On Fri, Dec 04, 2020 at 07:02:34PM +0200, Andra Paraschiv wrote:
>The vsock flags 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 flags field.
>
>The vsock transport is also assigned on the (listen) receive path. The
>flags field needs to be set considering the use case.
>
>Set the value of the vsock flags of the remote address to the one
>targeted for packets forwarding to the host, 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.
>
>Changelog
>
>v1 -> v2
>
>* Set the vsock flag on the receive path in the vsock transport
>  assignment logic.
>* Use bitwise operator for the vsock flag setup.
>* Use the updated "VMADDR_FLAG_TO_HOST" flag naming.
>
>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>---
> net/vmw_vsock/af_vsock.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index d10916ab45267..83d035eab0b05 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -431,6 +431,18 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> 	unsigned int remote_cid = vsk->remote_addr.svm_cid;
> 	int ret;
>
>+	/* If the packet is coming with the source and destination CIDs higher
>+	 * than VMADDR_CID_HOST, then a vsock channel where all the packets are
>+	 * forwarded to the host should be established. Then the host will
>+	 * need to forward the packets to the guest.
>+	 *
>+	 * The flag is set on the (listen) receive path (psk is not NULL). On
>+	 * the connect path the flag can be set by the user space application.
>+	 */
>+	if (psk && vsk->local_addr.svm_cid > VMADDR_CID_HOST &&
>+	    vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
>+		vsk->remote_addr.svm_flags |= VMADDR_FLAG_TO_HOST;
>+
> 	switch (sk->sk_type) {
> 	case SOCK_DGRAM:
> 		new_transport = transport_dgram;
>-- 
>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.
>

Re: [PATCH net-next v2 4/4] af_vsock: Assign the vsock transport considering the vsock address flags

[Stefano Garzarella]

On Fri, Dec 04, 2020 at 07:02:35PM +0200, Andra Paraschiv wrote:
>The vsock flags field can be 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 flags value (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 VMADDR_FLAG_TO_HOST flag
>is set. For the other use cases, the vsock transport assignment logic is
>not changed.
>
>Changelog
>
>v1 -> v2
>
>* Use bitwise operator to check the vsock flag.
>* Use the updated "VMADDR_FLAG_TO_HOST" flag naming.
>* Merge the checks for the g2h transport assignment in one "if" block.
>
>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>---
> net/vmw_vsock/af_vsock.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 83d035eab0b05..66e643c3b5f85 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -421,7 +421,8 @@ static void vsock_deassign_transport(struct vsock_sock *vsk)
>  * 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 or h2g is not loaded or remote flags field
>+ *    includes VMADDR_FLAG_TO_HOST flag value, 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)
>@@ -429,6 +430,7 @@ 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_flags;
> 	int ret;
>
> 	/* If the packet is coming with the source and destination CIDs higher
>@@ -443,6 +445,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> 	    vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
> 		vsk->remote_addr.svm_flags |= VMADDR_FLAG_TO_HOST;
>
>+	remote_flags = vsk->remote_addr.svm_flags;
>+
> 	switch (sk->sk_type) {
> 	case SOCK_DGRAM:
> 		new_transport = transport_dgram;
>@@ -450,7 +454,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_cid <= VMADDR_CID_HOST || !transport_h2g)
>+		else if (remote_cid <= VMADDR_CID_HOST || !transport_h2g ||
>+			 (remote_flags & VMADDR_FLAG_TO_HOST) == VMADDR_FLAG_TO_HOST)

Maybe "remote_flags & VMADDR_FLAG_TO_HOST" should be enough, but the 
patch is okay:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 			new_transport = transport_g2h;
> 		else
> 			new_transport = transport_h2g;
>-- 
>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.
>

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

[Stefano Garzarella]

Hi Andra,

On Fri, Dec 04, 2020 at 07:02:31PM +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 "svm_reserved1" field
>has been repurposed to be the flags field. The value of the flags will then be
>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 series seems in a good shape, I left some minor comments.
I run my test suite (vsock_test, iperf3, nc) with nested VMs (QEMU/KVM), 
and everything looks good.

Note: I'll be offline today and tomorrow, so I may miss followups.

Thanks,
Stefano

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

[Paraschiv, Andra-Irina]

On 07/12/2020 12:05, Stefano Garzarella wrote:
>
> Hi Andra,
>
> On Fri, Dec 04, 2020 at 07:02:31PM +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 
>> "svm_reserved1" field
>> has been repurposed to be the flags field. The value of the flags 
>> will then be
>> 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 series seems in a good shape, I left some minor comments.
> I run my test suite (vsock_test, iperf3, nc) with nested VMs (QEMU/KVM),
> and everything looks good.

Thanks, Stefano, for review and checking it out for the nested case as well.

I'll send out v3 including the addressed feedback and the Rb tags.

>
> Note: I'll be offline today and tomorrow, so I may miss followups.

Ok, np, thanks for the heads-up.

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.

Re: [PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Paraschiv, Andra-Irina]

On 07/12/2020 11:59, Stefano Garzarella wrote:
>
> On Fri, Dec 04, 2020 at 07:02:32PM +0200, Andra Paraschiv wrote:
>> vsock enables communication between virtual machines and the host they
>> are running on. With the multi transport support (guest->host and
>> host->guest), nested VMs can also use vsock channels for communication.
>>
>> In addition to this, by default, all the vsock packets are forwarded to
>> the host, if no host->guest transport is loaded. This behavior can be
>> implicitly used for enabling vsock communication between sibling VMs.
>>
>> Add a flags field in the vsock address data structure that can be used
>> to explicitly mark the vsock connection as being targeted for a certain
>> type of communication. This way, can distinguish between different use
>> cases such as nested VMs and sibling VMs.
>>
>> Use the already available "svm_reserved1" field and mark it as a flags
>> field instead. This field can be set when initializing the vsock address
>> variable used for the connect() call.
>>
>> Changelog
>>
>> v1 -> v2
>>
>> * Update the field name to "svm_flags".
>> * Split the current patch in 2 patches.
>
> Usually the changelog goes after the 3 dashes, but I'm not sure there is
> a strict rule :-)

Yup, I've seen both ways. I'd rather keep the changelog in the commit 
message, for further reference after merge, in the commits.

>
> Anyway the patch LGTM:
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thank you.

Andra

>
>>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>> ---
>> include/uapi/linux/vm_sockets.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/vm_sockets.h 
>> b/include/uapi/linux/vm_sockets.h
>> index fd0ed7221645d..46735376a57a8 100644
>> --- a/include/uapi/linux/vm_sockets.h
>> +++ b/include/uapi/linux/vm_sockets.h
>> @@ -145,7 +145,7 @@
>>
>> struct sockaddr_vm {
>>       __kernel_sa_family_t svm_family;
>> -      unsigned short svm_reserved1;
>> +      unsigned short svm_flags;
>>       unsigned int svm_port;
>>       unsigned int svm_cid;
>>       unsigned char svm_zero[sizeof(struct sockaddr) -
>> -- 
>> 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.

Re: [PATCH net-next v2 2/4] vm_sockets: Add VMADDR_FLAG_TO_HOST vsock flag

[Paraschiv, Andra-Irina]

On 07/12/2020 11:59, Stefano Garzarella wrote:
>
> On Fri, Dec 04, 2020 at 07:02:33PM +0200, Andra Paraschiv wrote:
>> Add VMADDR_FLAG_TO_HOST vsock flag that is used to setup a vsock
>> connection where all the packets are forwarded to the host.
>>
>> Then, using this type of vsock channel, vsock communication between
>> sibling VMs can be built on top of it.
>>
>> Changelog
>>
>> v1 -> v2
>>
>> * New patch in v2, it was split from the first patch in the series.
>> * Remove the default value for the vsock flags field.
>> * Update the naming for the vsock flag to "VMADDR_FLAG_TO_HOST".
>>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>> ---
>> include/uapi/linux/vm_sockets.h | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/include/uapi/linux/vm_sockets.h 
>> b/include/uapi/linux/vm_sockets.h
>> index 46735376a57a8..72e1a3d05682d 100644
>> --- a/include/uapi/linux/vm_sockets.h
>> +++ b/include/uapi/linux/vm_sockets.h
>> @@ -114,6 +114,21 @@
>>
>> #define VMADDR_CID_HOST 2
>>
>> +/* The current default use case for the vsock channel is the following:
>> + * local vsock communication between guest and host and nested VMs 
>> setup.
>> + * In addition to this, implicitly, the vsock packets are forwarded 
>> to the host
>> + * if no host->guest vsock transport is set.
>> + *
>> + * Set this flag value in the sockaddr_vm corresponding field if the 
>> vsock
>> + * packets need to be always forwarded to the host. Using this 
>> behavior,
>> + * vsock communication between sibling VMs can be setup.
>
> Maybe we can add a sentence saying that this flag is set on the remote
> peer address for an incoming connection when it is routed from the host
> (local CID and remote CID > VMADDR_CID_HOST).

Sure, I can make it more clear when it is set e.g. in user space 
(connect path) and in kernel space (listen path).

Thanks,
Andra

>
>> + *
>> + * This way can explicitly distinguish between vsock channels 
>> created for
>> + * different use cases, such as nested VMs (or local communication 
>> between
>> + * guest and host) and sibling VMs.
>> + */
>> +#define VMADDR_FLAG_TO_HOST 0x0001
>> +
>> /* Invalid vSockets version. */
>>
>> #define VM_SOCKETS_INVALID_VERSION -1U
>> -- 
>> 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.

Re: [PATCH net-next v2 4/4] af_vsock: Assign the vsock transport considering the vsock address flags

[Paraschiv, Andra-Irina]

On 07/12/2020 12:00, Stefano Garzarella wrote:
>
> On Fri, Dec 04, 2020 at 07:02:35PM +0200, Andra Paraschiv wrote:
>> The vsock flags field can be 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 flags value (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 VMADDR_FLAG_TO_HOST flag
>> is set. For the other use cases, the vsock transport assignment logic is
>> not changed.
>>
>> Changelog
>>
>> v1 -> v2
>>
>> * Use bitwise operator to check the vsock flag.
>> * Use the updated "VMADDR_FLAG_TO_HOST" flag naming.
>> * Merge the checks for the g2h transport assignment in one "if" block.
>>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>> ---
>> net/vmw_vsock/af_vsock.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index 83d035eab0b05..66e643c3b5f85 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -421,7 +421,8 @@ static void vsock_deassign_transport(struct 
>> vsock_sock *vsk)
>>  * 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 or h2g is not loaded or remote 
>> flags field
>> + *    includes VMADDR_FLAG_TO_HOST flag value, 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)
>> @@ -429,6 +430,7 @@ 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_flags;
>>       int ret;
>>
>>       /* If the packet is coming with the source and destination CIDs 
>> higher
>> @@ -443,6 +445,8 @@ int vsock_assign_transport(struct vsock_sock 
>> *vsk, struct vsock_sock *psk)
>>           vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
>>               vsk->remote_addr.svm_flags |= VMADDR_FLAG_TO_HOST;
>>
>> +      remote_flags = vsk->remote_addr.svm_flags;
>> +
>>       switch (sk->sk_type) {
>>       case SOCK_DGRAM:
>>               new_transport = transport_dgram;
>> @@ -450,7 +454,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_cid <= VMADDR_CID_HOST || !transport_h2g)
>> +              else if (remote_cid <= VMADDR_CID_HOST || 
>> !transport_h2g ||
>> +                       (remote_flags & VMADDR_FLAG_TO_HOST) == 
>> VMADDR_FLAG_TO_HOST)
>
> Maybe "remote_flags & VMADDR_FLAG_TO_HOST" should be enough, but the
> patch is okay:
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Done, updated to have only the bitwise logic, without the comparison.

Thanks,
Andra

>
>>                       new_transport = transport_g2h;
>>               else
>>                       new_transport = transport_h2g;
>> -- 
>> 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.

Re: [PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Jakub Kicinski]

On Fri, 4 Dec 2020 19:02:32 +0200 Andra Paraschiv wrote:
> diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
> index fd0ed7221645d..46735376a57a8 100644
> --- a/include/uapi/linux/vm_sockets.h
> +++ b/include/uapi/linux/vm_sockets.h
> @@ -145,7 +145,7 @@
>  
>  struct sockaddr_vm {
>  	__kernel_sa_family_t svm_family;
> -	unsigned short svm_reserved1;
> +	unsigned short svm_flags;
>  	unsigned int svm_port;
>  	unsigned int svm_cid;
>  	unsigned char svm_zero[sizeof(struct sockaddr) -

Since this is a uAPI header I gotta ask - are you 100% sure that it's
okay to rename this field?

I didn't grasp from just reading the patches whether this is a uAPI or
just internal kernel flag, seems like the former from the reading of
the comment in patch 2. In which case what guarantees that existing
users don't pass in garbage since the kernel doesn't check it was 0?

Re: [PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Paraschiv, Andra-Irina]

On 07/12/2020 23:29, Jakub Kicinski wrote:
> On Fri, 4 Dec 2020 19:02:32 +0200 Andra Paraschiv wrote:
>> diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
>> index fd0ed7221645d..46735376a57a8 100644
>> --- a/include/uapi/linux/vm_sockets.h
>> +++ b/include/uapi/linux/vm_sockets.h
>> @@ -145,7 +145,7 @@
>>
>>   struct sockaddr_vm {
>>        __kernel_sa_family_t svm_family;
>> -     unsigned short svm_reserved1;
>> +     unsigned short svm_flags;
>>        unsigned int svm_port;
>>        unsigned int svm_cid;
>>        unsigned char svm_zero[sizeof(struct sockaddr) -
> Since this is a uAPI header I gotta ask - are you 100% sure that it's
> okay to rename this field?
>
> I didn't grasp from just reading the patches whether this is a uAPI or
> just internal kernel flag, seems like the former from the reading of
> the comment in patch 2. In which case what guarantees that existing
> users don't pass in garbage since the kernel doesn't check it was 0?

That's always good to double-check the uapi changes don't break / assume 
something, thanks for bringing this up. :)

Sure, let's go through the possible options step by step. Let me know if 
I get anything wrong and if I can help with clarifications.

There is the "svm_reserved1" field that is not used in the kernel 
codebase. It is set to 0 on the receive (listen) path as part of the 
vsock address initialization [1][2]. The "svm_family" and "svm_zero" 
fields are checked as part of the address validation [3].

Now, with the current change to "svm_flags", the flow is the following:

* On the receive (listen) path, the remote address structure is 
initialized as part of the vsock address init logic [2]. Then patch 3/4 
of this series sets the "VMADDR_FLAG_TO_HOST" flag given a set of 
conditions (local and remote CID > VMADDR_CID_HOST).

* On the connect path, the userspace logic can set the "svm_flags" 
field. It can be set to 0 or 1 (VMADDR_FLAG_TO_HOST); or any other value 
greater than 1. If the "VMADDR_FLAG_TO_HOST" flag is set, all the vsock 
packets are then forwarded to the host.

* When the vsock transport is assigned, the "svm_flags" field is 
checked, and if it has the "VMADDR_FLAG_TO_HOST" flag set, it goes on 
with a guest->host transport (patch 4/4 of this series). Otherwise, 
other specific flag value is not currently used.

Given all these points, the question remains what happens if the 
"svm_flags" field is set on the connect path to a value higher than 1 
(maybe a bogus one, not intended so). And it includes the 
"VMADDR_FLAG_TO_HOST" value (the single flag set and specifically used 
for now, but we should also account for any further possible flags). In 
this case, all the vsock packets would be forwarded to the host and 
maybe not intended so, having a bogus value for the flags field. Is this 
possible case what you are referring to?

Thanks,
Andra

[1] https://man7.org/linux/man-pages/man7/vsock.7.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/vmw_vsock/vsock_addr.c#n14
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/vmw_vsock/vsock_addr.c#n23



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.

Re: [PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Jakub Kicinski]

On Tue, 8 Dec 2020 20:23:24 +0200 Paraschiv, Andra-Irina wrote:
> >> --- a/include/uapi/linux/vm_sockets.h
> >> +++ b/include/uapi/linux/vm_sockets.h
> >> @@ -145,7 +145,7 @@
> >>
> >>   struct sockaddr_vm {
> >>        __kernel_sa_family_t svm_family;
> >> -     unsigned short svm_reserved1;
> >> +     unsigned short svm_flags;
> >>        unsigned int svm_port;
> >>        unsigned int svm_cid;
> >>        unsigned char svm_zero[sizeof(struct sockaddr) -  
> > Since this is a uAPI header I gotta ask - are you 100% sure that it's
> > okay to rename this field?
> >
> > I didn't grasp from just reading the patches whether this is a uAPI or
> > just internal kernel flag, seems like the former from the reading of
> > the comment in patch 2. In which case what guarantees that existing
> > users don't pass in garbage since the kernel doesn't check it was 0?  
> 
> That's always good to double-check the uapi changes don't break / assume 
> something, thanks for bringing this up. :)
> 
> Sure, let's go through the possible options step by step. Let me know if 
> I get anything wrong and if I can help with clarifications.
> 
> There is the "svm_reserved1" field that is not used in the kernel 
> codebase. It is set to 0 on the receive (listen) path as part of the 
> vsock address initialization [1][2]. The "svm_family" and "svm_zero" 
> fields are checked as part of the address validation [3].
> 
> Now, with the current change to "svm_flags", the flow is the following:
> 
> * On the receive (listen) path, the remote address structure is 
> initialized as part of the vsock address init logic [2]. Then patch 3/4 
> of this series sets the "VMADDR_FLAG_TO_HOST" flag given a set of 
> conditions (local and remote CID > VMADDR_CID_HOST).
> 
> * On the connect path, the userspace logic can set the "svm_flags" 
> field. It can be set to 0 or 1 (VMADDR_FLAG_TO_HOST); or any other value 
> greater than 1. If the "VMADDR_FLAG_TO_HOST" flag is set, all the vsock 
> packets are then forwarded to the host.
> 
> * When the vsock transport is assigned, the "svm_flags" field is 
> checked, and if it has the "VMADDR_FLAG_TO_HOST" flag set, it goes on 
> with a guest->host transport (patch 4/4 of this series). Otherwise, 
> other specific flag value is not currently used.
> 
> Given all these points, the question remains what happens if the 
> "svm_flags" field is set on the connect path to a value higher than 1 
> (maybe a bogus one, not intended so). And it includes the 
> "VMADDR_FLAG_TO_HOST" value (the single flag set and specifically used 
> for now, but we should also account for any further possible flags). In 
> this case, all the vsock packets would be forwarded to the host and 
> maybe not intended so, having a bogus value for the flags field. Is this 
> possible case what you are referring to?

Correct. What if user basically declared the structure on the stack,
and only initialized the fields the kernel used to check?

This problem needs to be at the very least discussed in the commit
message.

Re: [PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Stefano Garzarella]

On Tue, Dec 08, 2020 at 10:42:22AM -0800, Jakub Kicinski wrote:
>On Tue, 8 Dec 2020 20:23:24 +0200 Paraschiv, Andra-Irina wrote:
>> >> --- a/include/uapi/linux/vm_sockets.h
>> >> +++ b/include/uapi/linux/vm_sockets.h
>> >> @@ -145,7 +145,7 @@
>> >>
>> >>   struct sockaddr_vm {
>> >>        __kernel_sa_family_t svm_family;
>> >> -     unsigned short svm_reserved1;
>> >> +     unsigned short svm_flags;
>> >>        unsigned int svm_port;
>> >>        unsigned int svm_cid;
>> >>        unsigned char svm_zero[sizeof(struct sockaddr) -
>> > Since this is a uAPI header I gotta ask - are you 100% sure that it's
>> > okay to rename this field?
>> >
>> > I didn't grasp from just reading the patches whether this is a uAPI or
>> > just internal kernel flag, seems like the former from the reading of
>> > the comment in patch 2. In which case what guarantees that existing
>> > users don't pass in garbage since the kernel doesn't check it was 0?
>>
>> That's always good to double-check the uapi changes don't break / assume
>> something, thanks for bringing this up. :)
>>
>> Sure, let's go through the possible options step by step. Let me know if
>> I get anything wrong and if I can help with clarifications.
>>
>> There is the "svm_reserved1" field that is not used in the kernel
>> codebase. It is set to 0 on the receive (listen) path as part of the
>> vsock address initialization [1][2]. The "svm_family" and "svm_zero"
>> fields are checked as part of the address validation [3].
>>
>> Now, with the current change to "svm_flags", the flow is the following:
>>
>> * On the receive (listen) path, the remote address structure is
>> initialized as part of the vsock address init logic [2]. Then patch 3/4
>> of this series sets the "VMADDR_FLAG_TO_HOST" flag given a set of
>> conditions (local and remote CID > VMADDR_CID_HOST).
>>
>> * On the connect path, the userspace logic can set the "svm_flags"
>> field. It can be set to 0 or 1 (VMADDR_FLAG_TO_HOST); or any other value
>> greater than 1. If the "VMADDR_FLAG_TO_HOST" flag is set, all the vsock
>> packets are then forwarded to the host.
>>
>> * When the vsock transport is assigned, the "svm_flags" field is
>> checked, and if it has the "VMADDR_FLAG_TO_HOST" flag set, it goes on
>> with a guest->host transport (patch 4/4 of this series). Otherwise,
>> other specific flag value is not currently used.
>>
>> Given all these points, the question remains what happens if the
>> "svm_flags" field is set on the connect path to a value higher than 1
>> (maybe a bogus one, not intended so). And it includes the
>> "VMADDR_FLAG_TO_HOST" value (the single flag set and specifically used
>> for now, but we should also account for any further possible flags). In
>> this case, all the vsock packets would be forwarded to the host and
>> maybe not intended so, having a bogus value for the flags field. Is this
>> possible case what you are referring to?
>
>Correct. What if user basically declared the structure on the stack,
>and only initialized the fields the kernel used to check?
>
>This problem needs to be at the very least discussed in the commit
>message.
>

I agree that could be a problem, but here some considerations:
- I checked some applications (qemu-guest-agent, ncat, iperf-vsock) and 
   all use the same pattern: allocate memory, initialize all the 
   sockaddr_vm to zero (to be sure to initialize the svm_zero), set the 
   cid and port fields.
   So we should be safe, but of course it may not always be true.

- For now the issue could affect only nested VMs. We introduced this 
   support one year ago, so it's something new and maybe we don't cause 
   too many problems.

As an alternative, what about using 1 or 2 bytes from svm_zero[]?
These must be set at zero, even if we only check the first byte in the 
kernel.

Thanks,
Stefano

Re: [PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Paraschiv, Andra-Irina]

On 09/12/2020 12:48, Stefano Garzarella wrote:
>
> On Tue, Dec 08, 2020 at 10:42:22AM -0800, Jakub Kicinski wrote:
>> On Tue, 8 Dec 2020 20:23:24 +0200 Paraschiv, Andra-Irina wrote:
>>> >> --- a/include/uapi/linux/vm_sockets.h
>>> >> +++ b/include/uapi/linux/vm_sockets.h
>>> >> @@ -145,7 +145,7 @@
>>> >>
>>> >>   struct sockaddr_vm {
>>> >>        __kernel_sa_family_t svm_family;
>>> >> -     unsigned short svm_reserved1;
>>> >> +     unsigned short svm_flags;
>>> >>        unsigned int svm_port;
>>> >>        unsigned int svm_cid;
>>> >>        unsigned char svm_zero[sizeof(struct sockaddr) -
>>> > Since this is a uAPI header I gotta ask - are you 100% sure that it's
>>> > okay to rename this field?
>>> >
>>> > I didn't grasp from just reading the patches whether this is a 
>>> uAPI or
>>> > just internal kernel flag, seems like the former from the reading of
>>> > the comment in patch 2. In which case what guarantees that existing
>>> > users don't pass in garbage since the kernel doesn't check it was 0?
>>>
>>> That's always good to double-check the uapi changes don't break / 
>>> assume
>>> something, thanks for bringing this up. :)
>>>
>>> Sure, let's go through the possible options step by step. Let me 
>>> know if
>>> I get anything wrong and if I can help with clarifications.
>>>
>>> There is the "svm_reserved1" field that is not used in the kernel
>>> codebase. It is set to 0 on the receive (listen) path as part of the
>>> vsock address initialization [1][2]. The "svm_family" and "svm_zero"
>>> fields are checked as part of the address validation [3].
>>>
>>> Now, with the current change to "svm_flags", the flow is the following:
>>>
>>> * On the receive (listen) path, the remote address structure is
>>> initialized as part of the vsock address init logic [2]. Then patch 3/4
>>> of this series sets the "VMADDR_FLAG_TO_HOST" flag given a set of
>>> conditions (local and remote CID > VMADDR_CID_HOST).
>>>
>>> * On the connect path, the userspace logic can set the "svm_flags"
>>> field. It can be set to 0 or 1 (VMADDR_FLAG_TO_HOST); or any other 
>>> value
>>> greater than 1. If the "VMADDR_FLAG_TO_HOST" flag is set, all the vsock
>>> packets are then forwarded to the host.
>>>
>>> * When the vsock transport is assigned, the "svm_flags" field is
>>> checked, and if it has the "VMADDR_FLAG_TO_HOST" flag set, it goes on
>>> with a guest->host transport (patch 4/4 of this series). Otherwise,
>>> other specific flag value is not currently used.
>>>
>>> Given all these points, the question remains what happens if the
>>> "svm_flags" field is set on the connect path to a value higher than 1
>>> (maybe a bogus one, not intended so). And it includes the
>>> "VMADDR_FLAG_TO_HOST" value (the single flag set and specifically used
>>> for now, but we should also account for any further possible flags). In
>>> this case, all the vsock packets would be forwarded to the host and
>>> maybe not intended so, having a bogus value for the flags field. Is 
>>> this
>>> possible case what you are referring to?
>>
>> Correct. What if user basically declared the structure on the stack,
>> and only initialized the fields the kernel used to check?
>>
>> This problem needs to be at the very least discussed in the commit
>> message.
>>
>
> I agree that could be a problem, but here some considerations:
> - I checked some applications (qemu-guest-agent, ncat, iperf-vsock) and
>   all use the same pattern: allocate memory, initialize all the
>   sockaddr_vm to zero (to be sure to initialize the svm_zero), set the
>   cid and port fields.
>   So we should be safe, but of course it may not always be true.
>
> - For now the issue could affect only nested VMs. We introduced this
>   support one year ago, so it's something new and maybe we don't cause
>   too many problems.
>
> As an alternative, what about using 1 or 2 bytes from svm_zero[]?
> These must be set at zero, even if we only check the first byte in the
> kernel.

Thanks for the follow-up info.

We can also consider the "svm_zero" option and could use 2 bytes from 
that field for "svm_flags", keeping the same "unsigned short" type.

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.

Re: [PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Jakub Kicinski]

On Wed, 9 Dec 2020 17:17:56 +0200 Paraschiv, Andra-Irina wrote:
> > I agree that could be a problem, but here some considerations:
> > - I checked some applications (qemu-guest-agent, ncat, iperf-vsock) and
> >   all use the same pattern: allocate memory, initialize all the
> >   sockaddr_vm to zero (to be sure to initialize the svm_zero), set the
> >   cid and port fields.
> >   So we should be safe, but of course it may not always be true.
> >
> > - For now the issue could affect only nested VMs. We introduced this
> >   support one year ago, so it's something new and maybe we don't cause
> >   too many problems.
> >
> > As an alternative, what about using 1 or 2 bytes from svm_zero[]?
> > These must be set at zero, even if we only check the first byte in the
> > kernel.  
> 
> Thanks for the follow-up info.
> 
> We can also consider the "svm_zero" option and could use 2 bytes from 
> that field for "svm_flags", keeping the same "unsigned short" type.

Or use svm_zero as a gate for interpreting other fields?
If svm_zero[0]* == something start checking the value of reserved1?
* in practice the name can be unioned to something more palatable ;)

Re: [PATCH net-next v2 1/4] vm_sockets: Include flags field in the vsock address data structure

[Paraschiv, Andra-Irina]

On 09/12/2020 19:30, Jakub Kicinski wrote:
> On Wed, 9 Dec 2020 17:17:56 +0200 Paraschiv, Andra-Irina wrote:
>>> I agree that could be a problem, but here some considerations:
>>> - I checked some applications (qemu-guest-agent, ncat, iperf-vsock) and
>>>    all use the same pattern: allocate memory, initialize all the
>>>    sockaddr_vm to zero (to be sure to initialize the svm_zero), set the
>>>    cid and port fields.
>>>    So we should be safe, but of course it may not always be true.
>>>
>>> - For now the issue could affect only nested VMs. We introduced this
>>>    support one year ago, so it's something new and maybe we don't cause
>>>    too many problems.
>>>
>>> As an alternative, what about using 1 or 2 bytes from svm_zero[]?
>>> These must be set at zero, even if we only check the first byte in the
>>> kernel.
>> Thanks for the follow-up info.
>>
>> We can also consider the "svm_zero" option and could use 2 bytes from
>> that field for "svm_flags", keeping the same "unsigned short" type.
> Or use svm_zero as a gate for interpreting other fields?
> If svm_zero[0]* == something start checking the value of reserved1?
> * in practice the name can be unioned to something more palatable ;)

Thanks for the shared option, that could be one case to reuse the 
reserved field, with a two phase check logic.

I'll give it a try to the option of having a new field "svm_flags" and 
the "svm_zero" updated and then send out a new revision. Just let me 
know if there are other updates needed / questions in the meantime.


struct sockaddr_vm {
     __kernel_sa_family_t svm_family;
     unsigned short svm_reserved1;
     unsigned int svm_port;
     unsigned int svm_cid;
     unsigned short svm_flags;
     unsigned char svm_zero[sizeof(struct sockaddr) -
                    sizeof(sa_family_t) -
                    sizeof(unsigned short) -
                    sizeof(unsigned int) - sizeof(unsigned int) -
sizeof(unsigned short)];
};


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.