2019-11-19 11:03:13

by Stefano Garzarella

Subject: [PATCH net-next 0/6] vsock: add local transport support

This series introduces a new transport (vsock_loopback) to handle
local communication.
This could be useful to test vsock core itself and to allow developers
to test their applications without launching a VM.

Before this series, vmci and virtio transports allowed this behavior,
but only in the guest.
We are moving the loopback handling in a new transport, because it
might be useful to provide this feature also in the host or when
no H2G/G2H transports (hyperv, virtio, vmci) are loaded.

The user can use the loopback with the new VMADDR_CID_LOCAL (that
replaces VMADDR_CID_RESERVED) in any condition.
Otherwise, if the G2H transport is loaded, it can also use the guest
local CID as previously supported by vmci and virtio transports.
If G2H transport is not loaded, the user can also use VMADDR_CID_HOST
for local communication.

Patch 1 is a cleanup to build virtio_transport_common without virtio
Patch 2 adds the new VMADDR_CID_LOCAL, replacing VMADDR_CID_RESERVED
Patch 3 adds a new feature flag to register a loopback transport
Patch 4 adds the new vsock_loopback transport based on the loopback
implementation of virtio_transport
Patch 5 implements the logic to use the local transport for loopback
communication
Patch 6 removes the loopback from virtio_transport

@Jorgen: Do you think it might be a problem to replace
VMADDR_CID_RESERVED with VMADDR_CID_LOCAL?

Thanks,
Stefano

Stefano Garzarella (6):
vsock/virtio_transport_common: remove unused virtio header includes
vsock: add VMADDR_CID_LOCAL definition
vsock: add local transport support in the vsock core
vsock: add vsock_loopback transport
vsock: use local transport when it is loaded
vsock/virtio: remove loopback handling

MAINTAINERS | 1 +
include/net/af_vsock.h | 2 +
include/uapi/linux/vm_sockets.h | 8 +-
net/vmw_vsock/Kconfig | 12 ++
net/vmw_vsock/Makefile | 1 +
net/vmw_vsock/af_vsock.c | 49 +++++-
net/vmw_vsock/virtio_transport.c | 61 +------
net/vmw_vsock/virtio_transport_common.c | 3 -
net/vmw_vsock/vmci_transport.c | 2 +-
net/vmw_vsock/vsock_loopback.c | 217 ++++++++++++++++++++++++
10 files changed, 283 insertions(+), 73 deletions(-)
create mode 100644 net/vmw_vsock/vsock_loopback.c

--
2.21.0

2019-11-19 11:03:15

by Stefano Garzarella

Subject: [PATCH net-next 1/6] vsock/virtio_transport_common: remove unused virtio header includes

We can remove virtio header includes, because virtio_transport_common
doesn't use virtio API, but provides common functions to interface
virtio/vhost transports with the af_vsock core, and to handle
the protocol.

Signed-off-by: Stefano Garzarella <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index e5ea29c6bca7..0e20b0f6eb65 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -11,9 +11,6 @@
#include <linux/sched/signal.h>
#include <linux/ctype.h>
#include <linux/list.h>
-#include <linux/virtio.h>
-#include <linux/virtio_ids.h>
-#include <linux/virtio_config.h>
#include <linux/virtio_vsock.h>
#include <uapi/linux/vsockmon.h>

--
2.21.0

2019-11-19 11:03:28

by Stefano Garzarella

Subject: [PATCH net-next 4/6] vsock: add vsock_loopback transport

This patch adds a new vsock_loopback transport to handle local
communication.
This transport is based on the loopback implementation of
virtio_transport, so it uses the virtio_transport_common APIs
to interface with the vsock core.

Signed-off-by: Stefano Garzarella <[email protected]>
---
MAINTAINERS | 1 +
net/vmw_vsock/Kconfig | 12 ++
net/vmw_vsock/Makefile | 1 +
net/vmw_vsock/vsock_loopback.c | 217 +++++++++++++++++++++++++++++++++
4 files changed, 231 insertions(+)
create mode 100644 net/vmw_vsock/vsock_loopback.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 760049454a23..c2a3dc3113ba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17239,6 +17239,7 @@ F: net/vmw_vsock/diag.c
F: net/vmw_vsock/af_vsock_tap.c
F: net/vmw_vsock/virtio_transport_common.c
F: net/vmw_vsock/virtio_transport.c
+F: net/vmw_vsock/vsock_loopback.c
F: drivers/net/vsockmon.c
F: drivers/vhost/vsock.c
F: tools/testing/vsock/
diff --git a/net/vmw_vsock/Kconfig b/net/vmw_vsock/Kconfig
index 8abcb815af2d..56356d2980c8 100644
--- a/net/vmw_vsock/Kconfig
+++ b/net/vmw_vsock/Kconfig
@@ -26,6 +26,18 @@ config VSOCKETS_DIAG

Enable this module so userspace applications can query open sockets.

+config VSOCKETS_LOOPBACK
+ tristate "Virtual Sockets loopback transport"
+ depends on VSOCKETS
+ default y
+ select VIRTIO_VSOCKETS_COMMON
+ help
+ This module implements a loopback transport for Virtual Sockets,
+ using vmw_vsock_virtio_transport_common.
+
+ To compile this driver as a module, choose M here: the module
+ will be called vsock_loopback. If unsure, say N.
+
config VMWARE_VMCI_VSOCKETS
tristate "VMware VMCI transport for Virtual Sockets"
depends on VSOCKETS && VMWARE_VMCI
diff --git a/net/vmw_vsock/Makefile b/net/vmw_vsock/Makefile
index 7c6f9a0b67b0..6a943ec95c4a 100644
--- a/net/vmw_vsock/Makefile
+++ b/net/vmw_vsock/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_VMWARE_VMCI_VSOCKETS) += vmw_vsock_vmci_transport.o
obj-$(CONFIG_VIRTIO_VSOCKETS) += vmw_vsock_virtio_transport.o
obj-$(CONFIG_VIRTIO_VSOCKETS_COMMON) += vmw_vsock_virtio_transport_common.o
obj-$(CONFIG_HYPERV_VSOCKETS) += hv_sock.o
+obj-$(CONFIG_VSOCKETS_LOOPBACK) += vsock_loopback.o

vsock-y += af_vsock.o af_vsock_tap.o vsock_addr.o

diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
new file mode 100644
index 000000000000..3d1c1a88305f
--- /dev/null
+++ b/net/vmw_vsock/vsock_loopback.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * loopback transport for vsock using virtio_transport_common APIs
+ *
+ * Copyright (C) 2013-2019 Red Hat, Inc.
+ * Author: Asias He <[email protected]>
+ * Stefan Hajnoczi <[email protected]>
+ * Stefano Garzarella <[email protected]>
+ *
+ */
+#include <linux/spinlock.h>
+#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/virtio_vsock.h>
+
+static struct workqueue_struct *vsock_loopback_workqueue;
+static struct vsock_loopback *the_vsock_loopback;
+
+struct vsock_loopback {
+ spinlock_t loopback_list_lock; /* protects loopback_list */
+ struct list_head loopback_list;
+ struct work_struct loopback_work;
+};
+
+static u32 vsock_loopback_get_local_cid(void)
+{
+ return VMADDR_CID_LOCAL;
+}
+
+static int vsock_loopback_send_pkt(struct virtio_vsock_pkt *pkt)
+{
+ struct vsock_loopback *vsock;
+ int len = pkt->len;
+
+ rcu_read_lock();
+ vsock = rcu_dereference(the_vsock_loopback);
+ if (!vsock) {
+ virtio_transport_free_pkt(pkt);
+ len = -ENODEV;
+ goto out_rcu;
+ }
+
+ spin_lock_bh(&vsock->loopback_list_lock);
+ list_add_tail(&pkt->list, &vsock->loopback_list);
+ spin_unlock_bh(&vsock->loopback_list_lock);
+
+ queue_work(vsock_loopback_workqueue, &vsock->loopback_work);
+
+out_rcu:
+ rcu_read_unlock();
+ return len;
+}
+
+static int vsock_loopback_cancel_pkt(struct vsock_sock *vsk)
+{
+ struct vsock_loopback *vsock;
+ struct virtio_vsock_pkt *pkt, *n;
+ int ret;
+ LIST_HEAD(freeme);
+
+ rcu_read_lock();
+ vsock = rcu_dereference(the_vsock_loopback);
+ if (!vsock) {
+ ret = -ENODEV;
+ goto out_rcu;
+ }
+
+ spin_lock_bh(&vsock->loopback_list_lock);
+ list_for_each_entry_safe(pkt, n, &vsock->loopback_list, list) {
+ if (pkt->vsk != vsk)
+ continue;
+ list_move(&pkt->list, &freeme);
+ }
+ spin_unlock_bh(&vsock->loopback_list_lock);
+
+ list_for_each_entry_safe(pkt, n, &freeme, list) {
+ list_del(&pkt->list);
+ virtio_transport_free_pkt(pkt);
+ }
+
+ ret = 0;
+
+out_rcu:
+ rcu_read_unlock();
+ return ret;
+}
+
+static struct virtio_transport loopback_transport = {
+ .transport = {
+ .module = THIS_MODULE,
+
+ .get_local_cid = vsock_loopback_get_local_cid,
+
+ .init = virtio_transport_do_socket_init,
+ .destruct = virtio_transport_destruct,
+ .release = virtio_transport_release,
+ .connect = virtio_transport_connect,
+ .shutdown = virtio_transport_shutdown,
+ .cancel_pkt = vsock_loopback_cancel_pkt,
+
+ .dgram_bind = virtio_transport_dgram_bind,
+ .dgram_dequeue = virtio_transport_dgram_dequeue,
+ .dgram_enqueue = virtio_transport_dgram_enqueue,
+ .dgram_allow = virtio_transport_dgram_allow,
+
+ .stream_dequeue = virtio_transport_stream_dequeue,
+ .stream_enqueue = virtio_transport_stream_enqueue,
+ .stream_has_data = virtio_transport_stream_has_data,
+ .stream_has_space = virtio_transport_stream_has_space,
+ .stream_rcvhiwat = virtio_transport_stream_rcvhiwat,
+ .stream_is_active = virtio_transport_stream_is_active,
+ .stream_allow = virtio_transport_stream_allow,
+
+ .notify_poll_in = virtio_transport_notify_poll_in,
+ .notify_poll_out = virtio_transport_notify_poll_out,
+ .notify_recv_init = virtio_transport_notify_recv_init,
+ .notify_recv_pre_block = virtio_transport_notify_recv_pre_block,
+ .notify_recv_pre_dequeue = virtio_transport_notify_recv_pre_dequeue,
+ .notify_recv_post_dequeue = virtio_transport_notify_recv_post_dequeue,
+ .notify_send_init = virtio_transport_notify_send_init,
+ .notify_send_pre_block = virtio_transport_notify_send_pre_block,
+ .notify_send_pre_enqueue = virtio_transport_notify_send_pre_enqueue,
+ .notify_send_post_enqueue = virtio_transport_notify_send_post_enqueue,
+ .notify_buffer_size = virtio_transport_notify_buffer_size,
+ },
+
+ .send_pkt = vsock_loopback_send_pkt,
+};
+
+static void vsock_loopback_work(struct work_struct *work)
+{
+ struct vsock_loopback *vsock =
+ container_of(work, struct vsock_loopback, loopback_work);
+ LIST_HEAD(pkts);
+
+ spin_lock_bh(&vsock->loopback_list_lock);
+ list_splice_init(&vsock->loopback_list, &pkts);
+ spin_unlock_bh(&vsock->loopback_list_lock);
+
+ while (!list_empty(&pkts)) {
+ struct virtio_vsock_pkt *pkt;
+
+ pkt = list_first_entry(&pkts, struct virtio_vsock_pkt, list);
+ list_del_init(&pkt->list);
+
+ virtio_transport_deliver_tap_pkt(pkt);
+ virtio_transport_recv_pkt(&loopback_transport, pkt);
+ }
+}
+
+static int __init vsock_loopback_init(void)
+{
+ struct vsock_loopback *vsock = NULL;
+ int ret;
+
+ vsock_loopback_workqueue = alloc_workqueue("vsock-loopback", 0, 0);
+ if (!vsock_loopback_workqueue)
+ return -ENOMEM;
+
+ vsock = kzalloc(sizeof(*vsock), GFP_KERNEL);
+ if (!vsock) {
+ ret = -ENOMEM;
+ goto out_wq;
+ }
+
+ spin_lock_init(&vsock->loopback_list_lock);
+ INIT_LIST_HEAD(&vsock->loopback_list);
+ INIT_WORK(&vsock->loopback_work, vsock_loopback_work);
+
+ rcu_assign_pointer(the_vsock_loopback, vsock);
+
+ ret = vsock_core_register(&loopback_transport.transport,
+ VSOCK_TRANSPORT_F_LOCAL);
+ if (ret)
+ goto out_free;
+
+ return 0;
+
+out_free:
+ rcu_assign_pointer(the_vsock_loopback, NULL);
+ kfree(vsock);
+out_wq:
+ destroy_workqueue(vsock_loopback_workqueue);
+ return ret;
+}
+
+static void __exit vsock_loopback_exit(void)
+{
+ struct vsock_loopback *vsock = the_vsock_loopback;
+ struct virtio_vsock_pkt *pkt;
+
+ vsock_core_unregister(&loopback_transport.transport);
+
+ rcu_assign_pointer(the_vsock_loopback, NULL);
+ synchronize_rcu();
+
+ spin_lock_bh(&vsock->loopback_list_lock);
+ while (!list_empty(&vsock->loopback_list)) {
+ pkt = list_first_entry(&vsock->loopback_list,
+ struct virtio_vsock_pkt, list);
+ list_del(&pkt->list);
+ virtio_transport_free_pkt(pkt);
+ }
+ spin_unlock_bh(&vsock->loopback_list_lock);
+
+ flush_work(&vsock->loopback_work);
+
+ kfree(vsock);
+ destroy_workqueue(vsock_loopback_workqueue);
+}
+
+module_init(vsock_loopback_init);
+module_exit(vsock_loopback_exit);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Stefano Garzarella <[email protected]>");
+MODULE_DESCRIPTION("loopback transport for vsock");
+MODULE_ALIAS_NETPROTO(PF_VSOCK);
--
2.21.0

2019-11-19 11:03:35

by Stefano Garzarella

Subject: [PATCH net-next 6/6] vsock/virtio: remove loopback handling

We can remove the loopback handling from virtio_transport,
because now the vsock core is able to handle local communication
using the new vsock_loopback device.

Signed-off-by: Stefano Garzarella <[email protected]>
---
net/vmw_vsock/virtio_transport.c | 61 ++------------------------------
1 file changed, 2 insertions(+), 59 deletions(-)

diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index 1458c5c8b64d..dfbaf6bd8b1c 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -44,10 +44,6 @@ struct virtio_vsock {
spinlock_t send_pkt_list_lock;
struct list_head send_pkt_list;

- struct work_struct loopback_work;
- spinlock_t loopback_list_lock; /* protects loopback_list */
- struct list_head loopback_list;
-
atomic_t queued_replies;

/* The following fields are protected by rx_lock. vqs[VSOCK_VQ_RX]
@@ -86,20 +82,6 @@ static u32 virtio_transport_get_local_cid(void)
return ret;
}

-static int virtio_transport_send_pkt_loopback(struct virtio_vsock *vsock,
- struct virtio_vsock_pkt *pkt)
-{
- int len = pkt->len;
-
- spin_lock_bh(&vsock->loopback_list_lock);
- list_add_tail(&pkt->list, &vsock->loopback_list);
- spin_unlock_bh(&vsock->loopback_list_lock);
-
- queue_work(virtio_vsock_workqueue, &vsock->loopback_work);
-
- return len;
-}
-
static void
virtio_transport_send_pkt_work(struct work_struct *work)
{
@@ -194,7 +176,8 @@ virtio_transport_send_pkt(struct virtio_vsock_pkt *pkt)
}

if (le64_to_cpu(pkt->hdr.dst_cid) == vsock->guest_cid) {
- len = virtio_transport_send_pkt_loopback(vsock, pkt);
+ virtio_transport_free_pkt(pkt);
+ len = -ENODEV;
goto out_rcu;
}

@@ -502,33 +485,6 @@ static struct virtio_transport virtio_transport = {
.send_pkt = virtio_transport_send_pkt,
};

-static void virtio_transport_loopback_work(struct work_struct *work)
-{
- struct virtio_vsock *vsock =
- container_of(work, struct virtio_vsock, loopback_work);
- LIST_HEAD(pkts);
-
- spin_lock_bh(&vsock->loopback_list_lock);
- list_splice_init(&vsock->loopback_list, &pkts);
- spin_unlock_bh(&vsock->loopback_list_lock);
-
- mutex_lock(&vsock->rx_lock);
-
- if (!vsock->rx_run)
- goto out;
-
- while (!list_empty(&pkts)) {
- struct virtio_vsock_pkt *pkt;
-
- pkt = list_first_entry(&pkts, struct virtio_vsock_pkt, list);
- list_del_init(&pkt->list);
-
- virtio_transport_recv_pkt(&virtio_transport, pkt);
- }
-out:
- mutex_unlock(&vsock->rx_lock);
-}
-
static void virtio_transport_rx_work(struct work_struct *work)
{
struct virtio_vsock *vsock =
@@ -633,13 +589,10 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
mutex_init(&vsock->event_lock);
spin_lock_init(&vsock->send_pkt_list_lock);
INIT_LIST_HEAD(&vsock->send_pkt_list);
- spin_lock_init(&vsock->loopback_list_lock);
- INIT_LIST_HEAD(&vsock->loopback_list);
INIT_WORK(&vsock->rx_work, virtio_transport_rx_work);
INIT_WORK(&vsock->tx_work, virtio_transport_tx_work);
INIT_WORK(&vsock->event_work, virtio_transport_event_work);
INIT_WORK(&vsock->send_pkt_work, virtio_transport_send_pkt_work);
- INIT_WORK(&vsock->loopback_work, virtio_transport_loopback_work);

mutex_lock(&vsock->tx_lock);
vsock->tx_run = true;
@@ -720,22 +673,12 @@ static void virtio_vsock_remove(struct virtio_device *vdev)
}
spin_unlock_bh(&vsock->send_pkt_list_lock);

- spin_lock_bh(&vsock->loopback_list_lock);
- while (!list_empty(&vsock->loopback_list)) {
- pkt = list_first_entry(&vsock->loopback_list,
- struct virtio_vsock_pkt, list);
- list_del(&pkt->list);
- virtio_transport_free_pkt(pkt);
- }
- spin_unlock_bh(&vsock->loopback_list_lock);
-
/* Delete virtqueues and flush outstanding callbacks if any */
vdev->config->del_vqs(vdev);

/* Other works can be queued before 'config->del_vqs()', so we flush
* all works before to free the vsock object to avoid use after free.
*/
- flush_work(&vsock->loopback_work);
flush_work(&vsock->rx_work);
flush_work(&vsock->tx_work);
flush_work(&vsock->event_work);
--
2.21.0

2019-11-19 11:04:36

by Stefano Garzarella

Subject: [PATCH net-next 3/6] vsock: add local transport support in the vsock core

This patch allows to register a transport able to handle
local communication (loopback).

Signed-off-by: Stefano Garzarella <[email protected]>
---
include/net/af_vsock.h | 2 ++
net/vmw_vsock/af_vsock.c | 17 ++++++++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index 4206dc6d813f..b1c717286993 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -98,6 +98,8 @@ struct vsock_transport_send_notify_data {
#define VSOCK_TRANSPORT_F_G2H 0x00000002
/* Transport provides DGRAM communication */
#define VSOCK_TRANSPORT_F_DGRAM 0x00000004
+/* Transport provides local (loopback) communication */
+#define VSOCK_TRANSPORT_F_LOCAL 0x00000008

struct vsock_transport {
struct module *module;
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index cc8659838bf2..c9e5bad59dc1 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -136,6 +136,8 @@ static const struct vsock_transport *transport_h2g;
static const struct vsock_transport *transport_g2h;
/* Transport used for DGRAM communication */
static const struct vsock_transport *transport_dgram;
+/* Transport used for local communication */
+static const struct vsock_transport *transport_local;
static DEFINE_MUTEX(vsock_register_mutex);

/**** UTILS ****/
@@ -2130,7 +2132,7 @@ EXPORT_SYMBOL_GPL(vsock_core_get_transport);

int vsock_core_register(const struct vsock_transport *t, int features)
{
- const struct vsock_transport *t_h2g, *t_g2h, *t_dgram;
+ const struct vsock_transport *t_h2g, *t_g2h, *t_dgram, *t_local;
int err = mutex_lock_interruptible(&vsock_register_mutex);

if (err)
@@ -2139,6 +2141,7 @@ int vsock_core_register(const struct vsock_transport *t, int features)
t_h2g = transport_h2g;
t_g2h = transport_g2h;
t_dgram = transport_dgram;
+ t_local = transport_local;

if (features & VSOCK_TRANSPORT_F_H2G) {
if (t_h2g) {
@@ -2164,9 +2167,18 @@ int vsock_core_register(const struct vsock_transport *t, int features)
t_dgram = t;
}

+ if (features & VSOCK_TRANSPORT_F_LOCAL) {
+ if (t_local) {
+ err = -EBUSY;
+ goto err_busy;
+ }
+ t_local = t;
+ }
+
transport_h2g = t_h2g;
transport_g2h = t_g2h;
transport_dgram = t_dgram;
+ transport_local = t_local;

err_busy:
mutex_unlock(&vsock_register_mutex);
@@ -2187,6 +2199,9 @@ void vsock_core_unregister(const struct vsock_transport *t)
if (transport_dgram == t)
transport_dgram = NULL;

+ if (transport_local == t)
+ transport_local = NULL;
+
mutex_unlock(&vsock_register_mutex);
}
EXPORT_SYMBOL_GPL(vsock_core_unregister);
--
2.21.0

2019-11-19 11:04:50

by Stefano Garzarella

Subject: [PATCH net-next 2/6] vsock: add VMADDR_CID_LOCAL definition

The VMADDR_CID_RESERVED (1) was used by VMCI, but now it is not
used anymore, so we can reuse it for local communication
(loopback) adding the new well-know CID: VMADDR_CID_LOCAL.

Cc: Jorgen Hansen <[email protected]>
Signed-off-by: Stefano Garzarella <[email protected]>
---
include/uapi/linux/vm_sockets.h | 8 +++++---
net/vmw_vsock/vmci_transport.c | 2 +-
2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
index 68d57c5e99bc..fd0ed7221645 100644
--- a/include/uapi/linux/vm_sockets.h
+++ b/include/uapi/linux/vm_sockets.h
@@ -99,11 +99,13 @@

#define VMADDR_CID_HYPERVISOR 0

-/* This CID is specific to VMCI and can be considered reserved (even VMCI
- * doesn't use it anymore, it's a legacy value from an older release).
+/* Use this as the destination CID in an address when referring to the
+ * local communication (loopback).
+ * (This was VMADDR_CID_RESERVED, but even VMCI doesn't use it anymore,
+ * it was a legacy value from an older release).
*/

-#define VMADDR_CID_RESERVED 1
+#define VMADDR_CID_LOCAL 1

/* Use this as the destination CID in an address when referring to the host
* (any process other than the hypervisor). VMCI relies on it being 2, but
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 644d32e43d23..4b8b1150a738 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -648,7 +648,7 @@ static int vmci_transport_recv_dgram_cb(void *data, struct vmci_datagram *dg)
static bool vmci_transport_stream_allow(u32 cid, u32 port)
{
static const u32 non_socket_contexts[] = {
- VMADDR_CID_RESERVED,
+ VMADDR_CID_LOCAL,
};
int i;

--
2.21.0

2019-11-19 11:06:17

by Stefano Garzarella

Subject: [PATCH net-next 5/6] vsock: use local transport when it is loaded

Now that we have a transport that can handle the local communication,
we can use it when it is loaded.

A socket will use the local transport (loopback) when the remote
CID is:
- equal to VMADDR_CID_LOCAL
- or equal to transport_g2h->get_local_cid(), if transport_g2h
is loaded (this allows us to keep the same behavior implemented
by virtio and vmci transports)
- or equal to VMADDR_CID_HOST, if transport_g2h is not loaded

Signed-off-by: Stefano Garzarella <[email protected]>
---
net/vmw_vsock/af_vsock.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index c9e5bad59dc1..40bbb2a17e3d 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -388,6 +388,21 @@ void vsock_enqueue_accept(struct sock *listener, struct sock *connected)
}
EXPORT_SYMBOL_GPL(vsock_enqueue_accept);

+static bool vsock_use_local_transport(unsigned int remote_cid)
+{
+ if (!transport_local)
+ return false;
+
+ if (remote_cid == VMADDR_CID_LOCAL)
+ return true;
+
+ if (transport_g2h) {
+ return remote_cid == transport_g2h->get_local_cid();
+ } else {
+ return remote_cid == VMADDR_CID_HOST;
+ }
+}
+
static void vsock_deassign_transport(struct vsock_sock *vsk)
{
if (!vsk->transport)
@@ -404,9 +419,10 @@ 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_HOST will use guest->host transport;
- * - remote CID == local_cid (guest->host transport) will use guest->host
- * transport for loopback (host->guest transports don't support loopback);
+ * - 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 VMADDR_CID_HYPERVISOR, 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)
@@ -420,9 +436,10 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
new_transport = transport_dgram;
break;
case SOCK_STREAM:
- if (remote_cid <= VMADDR_CID_HOST ||
- (transport_g2h &&
- remote_cid == transport_g2h->get_local_cid()))
+ if (vsock_use_local_transport(remote_cid))
+ new_transport = transport_local;
+ else if (remote_cid == VMADDR_CID_HOST ||
+ remote_cid == VMADDR_CID_HYPERVISOR)
new_transport = transport_g2h;
else
new_transport = transport_h2g;
@@ -459,6 +476,9 @@ bool vsock_find_cid(unsigned int cid)
if (transport_h2g && cid == VMADDR_CID_HOST)
return true;

+ if (transport_local && cid == VMADDR_CID_LOCAL)
+ return true;
+
return false;
}
EXPORT_SYMBOL_GPL(vsock_find_cid);
--
2.21.0

2019-11-20 01:16:28

by David Miller

Subject: Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport

From: Stefano Garzarella <[email protected]>
Date: Tue, 19 Nov 2019 12:01:19 +0100

> +static int vsock_loopback_cancel_pkt(struct vsock_sock *vsk)
> +{
> + struct vsock_loopback *vsock;
> + struct virtio_vsock_pkt *pkt, *n;
> + int ret;
> + LIST_HEAD(freeme);

Reverse christmas tree ordering of local variables here please.

2019-11-20 09:04:13

by Stefano Garzarella

Subject: Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport

On Tue, Nov 19, 2019 at 05:15:01PM -0800, David Miller wrote:
> From: Stefano Garzarella <[email protected]>
> Date: Tue, 19 Nov 2019 12:01:19 +0100
>
> > +static int vsock_loopback_cancel_pkt(struct vsock_sock *vsk)
> > +{
> > + struct vsock_loopback *vsock;
> > + struct virtio_vsock_pkt *pkt, *n;
> > + int ret;
> > + LIST_HEAD(freeme);
>
> Reverse christmas tree ordering of local variables here please.
>

Sure, I'll fix in the v2.

Thanks,
Stefano

2019-11-21 09:36:33

by Stefan Hajnoczi

Subject: Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport

On Tue, Nov 19, 2019 at 12:01:19PM +0100, Stefano Garzarella wrote:

Ideas for long-term changes below.

Reviewed-by: Stefan Hajnoczi <[email protected]>

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 760049454a23..c2a3dc3113ba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17239,6 +17239,7 @@ F: net/vmw_vsock/diag.c
> F: net/vmw_vsock/af_vsock_tap.c
> F: net/vmw_vsock/virtio_transport_common.c
> F: net/vmw_vsock/virtio_transport.c
> +F: net/vmw_vsock/vsock_loopback.c
> F: drivers/net/vsockmon.c
> F: drivers/vhost/vsock.c
> F: tools/testing/vsock/

At this point you are most active in virtio-vsock and I am reviewing
patches on a best-effort basis. Feel free to add yourself as
maintainer.

> diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
> new file mode 100644
> index 000000000000..3d1c1a88305f
> --- /dev/null
> +++ b/net/vmw_vsock/vsock_loopback.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * loopback transport for vsock using virtio_transport_common APIs
> + *
> + * Copyright (C) 2013-2019 Red Hat, Inc.
> + * Author: Asias He <[email protected]>
> + * Stefan Hajnoczi <[email protected]>
> + * Stefano Garzarella <[email protected]>
> + *
> + */
> +#include <linux/spinlock.h>
> +#include <linux/module.h>
> +#include <linux/list.h>
> +#include <linux/virtio_vsock.h>

Is it time to rename the generic functionality in
virtio_transport_common.c? This doesn't have anything to do with virtio
:).

> +
> +static struct workqueue_struct *vsock_loopback_workqueue;
> +static struct vsock_loopback *the_vsock_loopback;

the_vsock_loopback could be a static global variable (not a pointer) and
vsock_loopback_workqueue could also be included in the struct.

The RCU pointer is really a way to synchronize vsock_loopback_send_pkt()
and vsock_loopback_cancel_pkt() with module exit. There is no other
reason for using a pointer.

It's cleaner to implement the synchronization once in af_vsock.c (or
virtio_transport_common.c) instead of making each transport do it.
Maybe try_module_get() and related APIs provide the necessary semantics
so that core vsock code can hold the transport module while it's being
used to send/cancel a packet.

> +MODULE_ALIAS_NETPROTO(PF_VSOCK);

Why does this module define the alias for PF_VSOCK? Doesn't another
module already define this alias?

Attachments:

2019-11-21 09:49:10

by Stefan Hajnoczi

Subject: Re: [PATCH net-next 0/6] vsock: add local transport support

On Tue, Nov 19, 2019 at 12:01:15PM +0100, Stefano Garzarella wrote:
> This series introduces a new transport (vsock_loopback) to handle
> local communication.
> This could be useful to test vsock core itself and to allow developers
> to test their applications without launching a VM.
>
> Before this series, vmci and virtio transports allowed this behavior,
> but only in the guest.
> We are moving the loopback handling in a new transport, because it
> might be useful to provide this feature also in the host or when
> no H2G/G2H transports (hyperv, virtio, vmci) are loaded.
>
> The user can use the loopback with the new VMADDR_CID_LOCAL (that
> replaces VMADDR_CID_RESERVED) in any condition.
> Otherwise, if the G2H transport is loaded, it can also use the guest
> local CID as previously supported by vmci and virtio transports.
> If G2H transport is not loaded, the user can also use VMADDR_CID_HOST
> for local communication.
>
> Patch 1 is a cleanup to build virtio_transport_common without virtio
> Patch 2 adds the new VMADDR_CID_LOCAL, replacing VMADDR_CID_RESERVED
> Patch 3 adds a new feature flag to register a loopback transport
> Patch 4 adds the new vsock_loopback transport based on the loopback
> implementation of virtio_transport
> Patch 5 implements the logic to use the local transport for loopback
> communication
> Patch 6 removes the loopback from virtio_transport
>
> @Jorgen: Do you think it might be a problem to replace
> VMADDR_CID_RESERVED with VMADDR_CID_LOCAL?
>
> Thanks,
> Stefano
>
> Stefano Garzarella (6):
> vsock/virtio_transport_common: remove unused virtio header includes
> vsock: add VMADDR_CID_LOCAL definition
> vsock: add local transport support in the vsock core
> vsock: add vsock_loopback transport
> vsock: use local transport when it is loaded
> vsock/virtio: remove loopback handling
>
> MAINTAINERS | 1 +
> include/net/af_vsock.h | 2 +
> include/uapi/linux/vm_sockets.h | 8 +-
> net/vmw_vsock/Kconfig | 12 ++
> net/vmw_vsock/Makefile | 1 +
> net/vmw_vsock/af_vsock.c | 49 +++++-
> net/vmw_vsock/virtio_transport.c | 61 +------
> net/vmw_vsock/virtio_transport_common.c | 3 -
> net/vmw_vsock/vmci_transport.c | 2 +-
> net/vmw_vsock/vsock_loopback.c | 217 ++++++++++++++++++++++++
> 10 files changed, 283 insertions(+), 73 deletions(-)
> create mode 100644 net/vmw_vsock/vsock_loopback.c

Please see my comments. Otherwise:

Reviewed-by: Stefan Hajnoczi <[email protected]>

Attachments:

2019-11-21 09:50:05

by Stefan Hajnoczi

Subject: Re: [PATCH net-next 5/6] vsock: use local transport when it is loaded

On Tue, Nov 19, 2019 at 12:01:20PM +0100, Stefano Garzarella wrote:
> @@ -420,9 +436,10 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> new_transport = transport_dgram;
> break;
> case SOCK_STREAM:
> - if (remote_cid <= VMADDR_CID_HOST ||
> - (transport_g2h &&
> - remote_cid == transport_g2h->get_local_cid()))
> + if (vsock_use_local_transport(remote_cid))
> + new_transport = transport_local;
> + else if (remote_cid == VMADDR_CID_HOST ||
> + remote_cid == VMADDR_CID_HYPERVISOR)
> new_transport = transport_g2h;
> else
> new_transport = transport_h2g;

We used to send VMADDR_CID_RESERVED to the host. Now we send
VMADDR_CID_RESERVED (LOCAL) to the guest when there is no
transport_local loaded?

If this is correct, is there a justification for this change? It seems
safest to retain existing behavior.

Attachments:

2019-11-21 10:03:58

by Stefano Garzarella

Subject: Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport

On Thu, Nov 21, 2019 at 09:34:58AM +0000, Stefan Hajnoczi wrote:
> On Tue, Nov 19, 2019 at 12:01:19PM +0100, Stefano Garzarella wrote:
>
> Ideas for long-term changes below.
>
> Reviewed-by: Stefan Hajnoczi <[email protected]>
>

Thanks for reviewing!

> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 760049454a23..c2a3dc3113ba 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17239,6 +17239,7 @@ F: net/vmw_vsock/diag.c
> > F: net/vmw_vsock/af_vsock_tap.c
> > F: net/vmw_vsock/virtio_transport_common.c
> > F: net/vmw_vsock/virtio_transport.c
> > +F: net/vmw_vsock/vsock_loopback.c
> > F: drivers/net/vsockmon.c
> > F: drivers/vhost/vsock.c
> > F: tools/testing/vsock/
>
> At this point you are most active in virtio-vsock and I am reviewing
> patches on a best-effort basis. Feel free to add yourself as
> maintainer.
>

Sure, I'd be happy to maintain it.

> > diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
> > new file mode 100644
> > index 000000000000..3d1c1a88305f
> > --- /dev/null
> > +++ b/net/vmw_vsock/vsock_loopback.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * loopback transport for vsock using virtio_transport_common APIs
> > + *
> > + * Copyright (C) 2013-2019 Red Hat, Inc.
> > + * Author: Asias He <[email protected]>
> > + * Stefan Hajnoczi <[email protected]>
> > + * Stefano Garzarella <[email protected]>
> > + *
> > + */
> > +#include <linux/spinlock.h>
> > +#include <linux/module.h>
> > +#include <linux/list.h>
> > +#include <linux/virtio_vsock.h>
>
> Is it time to rename the generic functionality in
> virtio_transport_common.c? This doesn't have anything to do with virtio
> :).
>

Completely agree, new transports could use it to handle the protocol without
reimplementing things already done.

> > +
> > +static struct workqueue_struct *vsock_loopback_workqueue;
> > +static struct vsock_loopback *the_vsock_loopback;
>
> the_vsock_loopback could be a static global variable (not a pointer) and
> vsock_loopback_workqueue could also be included in the struct.
>
> The RCU pointer is really a way to synchronize vsock_loopback_send_pkt()
> and vsock_loopback_cancel_pkt() with module exit. There is no other
> reason for using a pointer.
>
> It's cleaner to implement the synchronization once in af_vsock.c (or
> virtio_transport_common.c) instead of making each transport do it.
> Maybe try_module_get() and related APIs provide the necessary semantics
> so that core vsock code can hold the transport module while it's being
> used to send/cancel a packet.

Right, the module cannot be unloaded until open sockets, so here the
synchronization is not needed.

The synchronization come from virtio-vsock device that can be
hot-unplugged while sockets are still open, but that can't happen here.

I will remove the pointers and RCU in the v2.

Can I keep your R-b or do you prefer to watch v2 first?

>
> > +MODULE_ALIAS_NETPROTO(PF_VSOCK);
>
> Why does this module define the alias for PF_VSOCK? Doesn't another
> module already define this alias?

It is a way to load this module when PF_VSOCK is starting to be used.
MODULE_ALIAS_NETPROTO(PF_VSOCK) is already defined in vmci_transport
and hyperv_transport. IIUC it is used for the same reason.

In virtio_transport we don't need it because it will be loaded when
the PCI device is discovered.

Do you think there's a better way?
Should I include the vsock_loopback transport directly in af_vsock
without creating a new module?

Thanks,
Stefano

2019-11-21 10:05:38

by Stefano Garzarella

Subject: Re: [PATCH net-next 5/6] vsock: use local transport when it is loaded

On Thu, Nov 21, 2019 at 09:46:14AM +0000, Stefan Hajnoczi wrote:
> On Tue, Nov 19, 2019 at 12:01:20PM +0100, Stefano Garzarella wrote:
> > @@ -420,9 +436,10 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> > new_transport = transport_dgram;
> > break;
> > case SOCK_STREAM:
> > - if (remote_cid <= VMADDR_CID_HOST ||
> > - (transport_g2h &&
> > - remote_cid == transport_g2h->get_local_cid()))
> > + if (vsock_use_local_transport(remote_cid))
> > + new_transport = transport_local;
> > + else if (remote_cid == VMADDR_CID_HOST ||
> > + remote_cid == VMADDR_CID_HYPERVISOR)
> > new_transport = transport_g2h;
> > else
> > new_transport = transport_h2g;
>
> We used to send VMADDR_CID_RESERVED to the host. Now we send
> VMADDR_CID_RESERVED (LOCAL) to the guest when there is no
> transport_local loaded?
>
> If this is correct, is there a justification for this change? It seems
> safest to retain existing behavior.

You're right, I'll revert this change in v2.

Thanks,
Stefano

2019-11-21 10:07:03

by Stefano Garzarella

Subject: Re: [PATCH net-next 0/6] vsock: add local transport support

On Thu, Nov 21, 2019 at 09:46:43AM +0000, Stefan Hajnoczi wrote:
> On Tue, Nov 19, 2019 at 12:01:15PM +0100, Stefano Garzarella wrote:
> > This series introduces a new transport (vsock_loopback) to handle
> > local communication.
> > This could be useful to test vsock core itself and to allow developers
> > to test their applications without launching a VM.
> >
> > Before this series, vmci and virtio transports allowed this behavior,
> > but only in the guest.
> > We are moving the loopback handling in a new transport, because it
> > might be useful to provide this feature also in the host or when
> > no H2G/G2H transports (hyperv, virtio, vmci) are loaded.
> >
> > The user can use the loopback with the new VMADDR_CID_LOCAL (that
> > replaces VMADDR_CID_RESERVED) in any condition.
> > Otherwise, if the G2H transport is loaded, it can also use the guest
> > local CID as previously supported by vmci and virtio transports.
> > If G2H transport is not loaded, the user can also use VMADDR_CID_HOST
> > for local communication.
> >
> > Patch 1 is a cleanup to build virtio_transport_common without virtio
> > Patch 2 adds the new VMADDR_CID_LOCAL, replacing VMADDR_CID_RESERVED
> > Patch 3 adds a new feature flag to register a loopback transport
> > Patch 4 adds the new vsock_loopback transport based on the loopback
> > implementation of virtio_transport
> > Patch 5 implements the logic to use the local transport for loopback
> > communication
> > Patch 6 removes the loopback from virtio_transport
> >
> > @Jorgen: Do you think it might be a problem to replace
> > VMADDR_CID_RESERVED with VMADDR_CID_LOCAL?
> >
> > Thanks,
> > Stefano
> >
> > Stefano Garzarella (6):
> > vsock/virtio_transport_common: remove unused virtio header includes
> > vsock: add VMADDR_CID_LOCAL definition
> > vsock: add local transport support in the vsock core
> > vsock: add vsock_loopback transport
> > vsock: use local transport when it is loaded
> > vsock/virtio: remove loopback handling
> >
> > MAINTAINERS | 1 +
> > include/net/af_vsock.h | 2 +
> > include/uapi/linux/vm_sockets.h | 8 +-
> > net/vmw_vsock/Kconfig | 12 ++
> > net/vmw_vsock/Makefile | 1 +
> > net/vmw_vsock/af_vsock.c | 49 +++++-
> > net/vmw_vsock/virtio_transport.c | 61 +------
> > net/vmw_vsock/virtio_transport_common.c | 3 -
> > net/vmw_vsock/vmci_transport.c | 2 +-
> > net/vmw_vsock/vsock_loopback.c | 217 ++++++++++++++++++++++++
> > 10 files changed, 283 insertions(+), 73 deletions(-)
> > create mode 100644 net/vmw_vsock/vsock_loopback.c
>
> Please see my comments. Otherwise:
>
> Reviewed-by: Stefan Hajnoczi <[email protected]>

Thanks!
I'll send a v2 following your comments.

Stefano

2019-11-21 14:49:37

by Jorgen Hansen

Subject: RE: [PATCH net-next 0/6] vsock: add local transport support

> From: Stefano Garzarella [mailto:[email protected]]
> Sent: Tuesday, November 19, 2019 12:01 PM
> This series introduces a new transport (vsock_loopback) to handle
> local communication.
> This could be useful to test vsock core itself and to allow developers
> to test their applications without launching a VM.
>
> Before this series, vmci and virtio transports allowed this behavior,
> but only in the guest.
> We are moving the loopback handling in a new transport, because it
> might be useful to provide this feature also in the host or when
> no H2G/G2H transports (hyperv, virtio, vmci) are loaded.
>
> The user can use the loopback with the new VMADDR_CID_LOCAL (that
> replaces VMADDR_CID_RESERVED) in any condition.
> Otherwise, if the G2H transport is loaded, it can also use the guest
> local CID as previously supported by vmci and virtio transports.
> If G2H transport is not loaded, the user can also use VMADDR_CID_HOST
> for local communication.
>
> Patch 1 is a cleanup to build virtio_transport_common without virtio
> Patch 2 adds the new VMADDR_CID_LOCAL, replacing
> VMADDR_CID_RESERVED
> Patch 3 adds a new feature flag to register a loopback transport
> Patch 4 adds the new vsock_loopback transport based on the loopback
> implementation of virtio_transport
> Patch 5 implements the logic to use the local transport for loopback
> communication
> Patch 6 removes the loopback from virtio_transport
>
> @Jorgen: Do you think it might be a problem to replace
> VMADDR_CID_RESERVED with VMADDR_CID_LOCAL?

No, that should be fine. It has never allowed for use with stream sockets in
AF_VSOCK. The only potential use would be for datagram sockets, but that
side appears to be unaffected by your changes, since loopback is only
introduced for SOCK_STREAM.

>
> Thanks,
> Stefano
>
> Stefano Garzarella (6):
> vsock/virtio_transport_common: remove unused virtio header includes
> vsock: add VMADDR_CID_LOCAL definition
> vsock: add local transport support in the vsock core
> vsock: add vsock_loopback transport
> vsock: use local transport when it is loaded
> vsock/virtio: remove loopback handling
>
> MAINTAINERS | 1 +
> include/net/af_vsock.h | 2 +
> include/uapi/linux/vm_sockets.h | 8 +-
> net/vmw_vsock/Kconfig | 12 ++
> net/vmw_vsock/Makefile | 1 +
> net/vmw_vsock/af_vsock.c | 49 +++++-
> net/vmw_vsock/virtio_transport.c | 61 +------
> net/vmw_vsock/virtio_transport_common.c | 3 -
> net/vmw_vsock/vmci_transport.c | 2 +-
> net/vmw_vsock/vsock_loopback.c | 217
> ++++++++++++++++++++++++
> 10 files changed, 283 insertions(+), 73 deletions(-)
> create mode 100644 net/vmw_vsock/vsock_loopback.c
>
> --
> 2.21.0

2019-11-21 14:50:53

by Jorgen Hansen

Subject: RE: [PATCH net-next 2/6] vsock: add VMADDR_CID_LOCAL definition

> From: Stefano Garzarella [mailto:[email protected]]
> Sent: Tuesday, November 19, 2019 12:01 PM
>
> The VMADDR_CID_RESERVED (1) was used by VMCI, but now it is not
> used anymore, so we can reuse it for local communication
> (loopback) adding the new well-know CID: VMADDR_CID_LOCAL.
>
> Cc: Jorgen Hansen <[email protected]>
> Signed-off-by: Stefano Garzarella <[email protected]>
> ---
> include/uapi/linux/vm_sockets.h | 8 +++++---
> net/vmw_vsock/vmci_transport.c | 2 +-
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/vm_sockets.h
> b/include/uapi/linux/vm_sockets.h
> index 68d57c5e99bc..fd0ed7221645 100644
> --- a/include/uapi/linux/vm_sockets.h
> +++ b/include/uapi/linux/vm_sockets.h
> @@ -99,11 +99,13 @@
>
> #define VMADDR_CID_HYPERVISOR 0
>
> -/* This CID is specific to VMCI and can be considered reserved (even VMCI
> - * doesn't use it anymore, it's a legacy value from an older release).
> +/* Use this as the destination CID in an address when referring to the
> + * local communication (loopback).
> + * (This was VMADDR_CID_RESERVED, but even VMCI doesn't use it
> anymore,
> + * it was a legacy value from an older release).
> */
>
> -#define VMADDR_CID_RESERVED 1
> +#define VMADDR_CID_LOCAL 1
>
> /* Use this as the destination CID in an address when referring to the host
> * (any process other than the hypervisor). VMCI relies on it being 2, but
> diff --git a/net/vmw_vsock/vmci_transport.c
> b/net/vmw_vsock/vmci_transport.c
> index 644d32e43d23..4b8b1150a738 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -648,7 +648,7 @@ static int vmci_transport_recv_dgram_cb(void *data,
> struct vmci_datagram *dg)
> static bool vmci_transport_stream_allow(u32 cid, u32 port)
> {
> static const u32 non_socket_contexts[] = {
> - VMADDR_CID_RESERVED,
> + VMADDR_CID_LOCAL,
> };
> int i;
>
> --
> 2.21.0

Reviewed-by: Jorgen Hansen <[email protected]>

2019-11-21 15:06:12

by Jorgen Hansen

Subject: RE: [PATCH net-next 3/6] vsock: add local transport support in the vsock core

> From: Stefano Garzarella [mailto:[email protected]]
> Sent: Tuesday, November 19, 2019 12:01 PM
> To: [email protected]
>
> This patch allows to register a transport able to handle
> local communication (loopback).
>
> Signed-off-by: Stefano Garzarella <[email protected]>
> ---
> include/net/af_vsock.h | 2 ++
> net/vmw_vsock/af_vsock.c | 17 ++++++++++++++++-
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> index 4206dc6d813f..b1c717286993 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -98,6 +98,8 @@ struct vsock_transport_send_notify_data {
> #define VSOCK_TRANSPORT_F_G2H 0x00000002
> /* Transport provides DGRAM communication */
> #define VSOCK_TRANSPORT_F_DGRAM 0x00000004
> +/* Transport provides local (loopback) communication */
> +#define VSOCK_TRANSPORT_F_LOCAL 0x00000008
>
> struct vsock_transport {
> struct module *module;
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index cc8659838bf2..c9e5bad59dc1 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -136,6 +136,8 @@ static const struct vsock_transport *transport_h2g;
> static const struct vsock_transport *transport_g2h;
> /* Transport used for DGRAM communication */
> static const struct vsock_transport *transport_dgram;
> +/* Transport used for local communication */
> +static const struct vsock_transport *transport_local;
> static DEFINE_MUTEX(vsock_register_mutex);
>
> /**** UTILS ****/
> @@ -2130,7 +2132,7 @@ EXPORT_SYMBOL_GPL(vsock_core_get_transport);
>
> int vsock_core_register(const struct vsock_transport *t, int features)
> {
> - const struct vsock_transport *t_h2g, *t_g2h, *t_dgram;
> + const struct vsock_transport *t_h2g, *t_g2h, *t_dgram, *t_local;
> int err = mutex_lock_interruptible(&vsock_register_mutex);
>
> if (err)
> @@ -2139,6 +2141,7 @@ int vsock_core_register(const struct
> vsock_transport *t, int features)
> t_h2g = transport_h2g;
> t_g2h = transport_g2h;
> t_dgram = transport_dgram;
> + t_local = transport_local;
>
> if (features & VSOCK_TRANSPORT_F_H2G) {
> if (t_h2g) {
> @@ -2164,9 +2167,18 @@ int vsock_core_register(const struct
> vsock_transport *t, int features)
> t_dgram = t;
> }
>
> + if (features & VSOCK_TRANSPORT_F_LOCAL) {
> + if (t_local) {
> + err = -EBUSY;
> + goto err_busy;
> + }
> + t_local = t;
> + }
> +
> transport_h2g = t_h2g;
> transport_g2h = t_g2h;
> transport_dgram = t_dgram;
> + transport_local = t_local;
>
> err_busy:
> mutex_unlock(&vsock_register_mutex);
> @@ -2187,6 +2199,9 @@ void vsock_core_unregister(const struct
> vsock_transport *t)
> if (transport_dgram == t)
> transport_dgram = NULL;
>
> + if (transport_local == t)
> + transport_local = NULL;
> +
> mutex_unlock(&vsock_register_mutex);
> }
> EXPORT_SYMBOL_GPL(vsock_core_unregister);
> --
> 2.21.0

Having loopback support as a separate transport fits nicely, but do we need to support
different variants of loopback? It could just be built in.

Thanks,
Jorgen

2019-11-21 15:15:38

by Stefano Garzarella

Subject: Re: [PATCH net-next 0/6] vsock: add local transport support

On Thu, Nov 21, 2019 at 02:45:32PM +0000, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:[email protected]]
> > Sent: Tuesday, November 19, 2019 12:01 PM
> > This series introduces a new transport (vsock_loopback) to handle
> > local communication.
> > This could be useful to test vsock core itself and to allow developers
> > to test their applications without launching a VM.
> >
> > Before this series, vmci and virtio transports allowed this behavior,
> > but only in the guest.
> > We are moving the loopback handling in a new transport, because it
> > might be useful to provide this feature also in the host or when
> > no H2G/G2H transports (hyperv, virtio, vmci) are loaded.
> >
> > The user can use the loopback with the new VMADDR_CID_LOCAL (that
> > replaces VMADDR_CID_RESERVED) in any condition.
> > Otherwise, if the G2H transport is loaded, it can also use the guest
> > local CID as previously supported by vmci and virtio transports.
> > If G2H transport is not loaded, the user can also use VMADDR_CID_HOST
> > for local communication.
> >
> > Patch 1 is a cleanup to build virtio_transport_common without virtio
> > Patch 2 adds the new VMADDR_CID_LOCAL, replacing
> > VMADDR_CID_RESERVED
> > Patch 3 adds a new feature flag to register a loopback transport
> > Patch 4 adds the new vsock_loopback transport based on the loopback
> > implementation of virtio_transport
> > Patch 5 implements the logic to use the local transport for loopback
> > communication
> > Patch 6 removes the loopback from virtio_transport
> >
> > @Jorgen: Do you think it might be a problem to replace
> > VMADDR_CID_RESERVED with VMADDR_CID_LOCAL?
>
> No, that should be fine. It has never allowed for use with stream sockets in
> AF_VSOCK. The only potential use would be for datagram sockets, but that
> side appears to be unaffected by your changes, since loopback is only
> introduced for SOCK_STREAM.
>

Yes, datagram sockets are not affected.

Thanks for the clarification,
Stefano

2019-11-21 15:24:16

by Stefano Garzarella

Subject: Re: [PATCH net-next 3/6] vsock: add local transport support in the vsock core

On Thu, Nov 21, 2019 at 03:04:18PM +0000, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:[email protected]]
> > Sent: Tuesday, November 19, 2019 12:01 PM
> > To: [email protected]
> >
> > This patch allows to register a transport able to handle
> > local communication (loopback).
> >
> > Signed-off-by: Stefano Garzarella <[email protected]>
> > ---
> > include/net/af_vsock.h | 2 ++
> > net/vmw_vsock/af_vsock.c | 17 ++++++++++++++++-
> > 2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> > index 4206dc6d813f..b1c717286993 100644
> > --- a/include/net/af_vsock.h
> > +++ b/include/net/af_vsock.h
> > @@ -98,6 +98,8 @@ struct vsock_transport_send_notify_data {
> > #define VSOCK_TRANSPORT_F_G2H 0x00000002
> > /* Transport provides DGRAM communication */
> > #define VSOCK_TRANSPORT_F_DGRAM 0x00000004
> > +/* Transport provides local (loopback) communication */
> > +#define VSOCK_TRANSPORT_F_LOCAL 0x00000008
> >
> > struct vsock_transport {
> > struct module *module;
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index cc8659838bf2..c9e5bad59dc1 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -136,6 +136,8 @@ static const struct vsock_transport *transport_h2g;
> > static const struct vsock_transport *transport_g2h;
> > /* Transport used for DGRAM communication */
> > static const struct vsock_transport *transport_dgram;
> > +/* Transport used for local communication */
> > +static const struct vsock_transport *transport_local;
> > static DEFINE_MUTEX(vsock_register_mutex);
> >
> > /**** UTILS ****/
> > @@ -2130,7 +2132,7 @@ EXPORT_SYMBOL_GPL(vsock_core_get_transport);
> >
> > int vsock_core_register(const struct vsock_transport *t, int features)
> > {
> > - const struct vsock_transport *t_h2g, *t_g2h, *t_dgram;
> > + const struct vsock_transport *t_h2g, *t_g2h, *t_dgram, *t_local;
> > int err = mutex_lock_interruptible(&vsock_register_mutex);
> >
> > if (err)
> > @@ -2139,6 +2141,7 @@ int vsock_core_register(const struct
> > vsock_transport *t, int features)
> > t_h2g = transport_h2g;
> > t_g2h = transport_g2h;
> > t_dgram = transport_dgram;
> > + t_local = transport_local;
> >
> > if (features & VSOCK_TRANSPORT_F_H2G) {
> > if (t_h2g) {
> > @@ -2164,9 +2167,18 @@ int vsock_core_register(const struct
> > vsock_transport *t, int features)
> > t_dgram = t;
> > }
> >
> > + if (features & VSOCK_TRANSPORT_F_LOCAL) {
> > + if (t_local) {
> > + err = -EBUSY;
> > + goto err_busy;
> > + }
> > + t_local = t;
> > + }
> > +
> > transport_h2g = t_h2g;
> > transport_g2h = t_g2h;
> > transport_dgram = t_dgram;
> > + transport_local = t_local;
> >
> > err_busy:
> > mutex_unlock(&vsock_register_mutex);
> > @@ -2187,6 +2199,9 @@ void vsock_core_unregister(const struct
> > vsock_transport *t)
> > if (transport_dgram == t)
> > transport_dgram = NULL;
> >
> > + if (transport_local == t)
> > + transport_local = NULL;
> > +
> > mutex_unlock(&vsock_register_mutex);
> > }
> > EXPORT_SYMBOL_GPL(vsock_core_unregister);
> > --
> > 2.21.0
>
> Having loopback support as a separate transport fits nicely, but do we need to support
> different variants of loopback? It could just be built in.

I agree with you, indeed initially I developed it as built in, but
DEPMOD found a cyclic dependency because vsock_transport use
virtio_transport_common that use vsock, so if I include vsock_transport
in the vsock module, DEPMOD is not happy.

I don't know how to break this cyclic dependency, do you have any ideas?

Thanks,
Stefano

2019-11-21 15:29:07

by Stefano Garzarella

Subject: Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport

On Thu, Nov 21, 2019 at 10:59:48AM +0100, Stefano Garzarella wrote:
> On Thu, Nov 21, 2019 at 09:34:58AM +0000, Stefan Hajnoczi wrote:
> > On Tue, Nov 19, 2019 at 12:01:19PM +0100, Stefano Garzarella wrote:
> >
> > Ideas for long-term changes below.
> >
> > Reviewed-by: Stefan Hajnoczi <[email protected]>
> >
>
> Thanks for reviewing!
>
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 760049454a23..c2a3dc3113ba 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -17239,6 +17239,7 @@ F: net/vmw_vsock/diag.c
> > > F: net/vmw_vsock/af_vsock_tap.c
> > > F: net/vmw_vsock/virtio_transport_common.c
> > > F: net/vmw_vsock/virtio_transport.c
> > > +F: net/vmw_vsock/vsock_loopback.c
> > > F: drivers/net/vsockmon.c
> > > F: drivers/vhost/vsock.c
> > > F: tools/testing/vsock/
> >
> > At this point you are most active in virtio-vsock and I am reviewing
> > patches on a best-effort basis. Feel free to add yourself as
> > maintainer.
> >
>
> Sure, I'd be happy to maintain it.
>
> > > diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
> > > new file mode 100644
> > > index 000000000000..3d1c1a88305f
> > > --- /dev/null
> > > +++ b/net/vmw_vsock/vsock_loopback.c
> > > @@ -0,0 +1,217 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * loopback transport for vsock using virtio_transport_common APIs
> > > + *
> > > + * Copyright (C) 2013-2019 Red Hat, Inc.
> > > + * Author: Asias He <[email protected]>
> > > + * Stefan Hajnoczi <[email protected]>
> > > + * Stefano Garzarella <[email protected]>
> > > + *
> > > + */
> > > +#include <linux/spinlock.h>
> > > +#include <linux/module.h>
> > > +#include <linux/list.h>
> > > +#include <linux/virtio_vsock.h>
> >
> > Is it time to rename the generic functionality in
> > virtio_transport_common.c? This doesn't have anything to do with virtio
> > :).
> >
>
> Completely agree, new transports could use it to handle the protocol without
> reimplementing things already done.
>
> > > +
> > > +static struct workqueue_struct *vsock_loopback_workqueue;
> > > +static struct vsock_loopback *the_vsock_loopback;
> >
> > the_vsock_loopback could be a static global variable (not a pointer) and
> > vsock_loopback_workqueue could also be included in the struct.
> >
> > The RCU pointer is really a way to synchronize vsock_loopback_send_pkt()
> > and vsock_loopback_cancel_pkt() with module exit. There is no other
> > reason for using a pointer.
> >
> > It's cleaner to implement the synchronization once in af_vsock.c (or
> > virtio_transport_common.c) instead of making each transport do it.
> > Maybe try_module_get() and related APIs provide the necessary semantics
> > so that core vsock code can hold the transport module while it's being
> > used to send/cancel a packet.
>
> Right, the module cannot be unloaded until open sockets, so here the
> synchronization is not needed.
>
> The synchronization come from virtio-vsock device that can be
> hot-unplugged while sockets are still open, but that can't happen here.
>
> I will remove the pointers and RCU in the v2.
>
> Can I keep your R-b or do you prefer to watch v2 first?
>
> >
> > > +MODULE_ALIAS_NETPROTO(PF_VSOCK);
> >
> > Why does this module define the alias for PF_VSOCK? Doesn't another
> > module already define this alias?
>
> It is a way to load this module when PF_VSOCK is starting to be used.
> MODULE_ALIAS_NETPROTO(PF_VSOCK) is already defined in vmci_transport
> and hyperv_transport. IIUC it is used for the same reason.
>
> In virtio_transport we don't need it because it will be loaded when
> the PCI device is discovered.
>
> Do you think there's a better way?
> Should I include the vsock_loopback transport directly in af_vsock
> without creating a new module?
>

That last thing I said may not be possible:
I remembered that I tried, but DEPMOD found a cyclic dependency because
vsock_transport use virtio_transport_common that use vsock, so if I
include vsock_transport in the vsock module, DEPMOD is not happy.

Do you think it's okay in this case to keep MODULE_ALIAS_NETPROTO(PF_VSOCK)
or is there a better way?

Thanks,
Stefano

2019-11-21 15:55:22

by Jorgen Hansen

Subject: RE: [PATCH net-next 3/6] vsock: add local transport support in the vsock core

> From: Stefano Garzarella [mailto:[email protected]]
> Sent: Thursday, November 21, 2019 4:22 PM
>
> On Thu, Nov 21, 2019 at 03:04:18PM +0000, Jorgen Hansen wrote:
> > > From: Stefano Garzarella [mailto:[email protected]]
> > > Sent: Tuesday, November 19, 2019 12:01 PM
> > > To: [email protected]
> > >
> > > This patch allows to register a transport able to handle
> > > local communication (loopback).
> > >
> > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > ---
> > > include/net/af_vsock.h | 2 ++
> > > net/vmw_vsock/af_vsock.c | 17 ++++++++++++++++-
> > > 2 files changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> > > index 4206dc6d813f..b1c717286993 100644
> > > --- a/include/net/af_vsock.h
> > > +++ b/include/net/af_vsock.h
> > > @@ -98,6 +98,8 @@ struct vsock_transport_send_notify_data {
> > > #define VSOCK_TRANSPORT_F_G2H 0x00000002
> > > /* Transport provides DGRAM communication */
> > > #define VSOCK_TRANSPORT_F_DGRAM 0x00000004
> > > +/* Transport provides local (loopback) communication */
> > > +#define VSOCK_TRANSPORT_F_LOCAL 0x00000008
> > >
> > > struct vsock_transport {
> > > struct module *module;
> > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > > index cc8659838bf2..c9e5bad59dc1 100644
> > > --- a/net/vmw_vsock/af_vsock.c
> > > +++ b/net/vmw_vsock/af_vsock.c
> > > @@ -136,6 +136,8 @@ static const struct vsock_transport
> *transport_h2g;
> > > static const struct vsock_transport *transport_g2h;
> > > /* Transport used for DGRAM communication */
> > > static const struct vsock_transport *transport_dgram;
> > > +/* Transport used for local communication */
> > > +static const struct vsock_transport *transport_local;
> > > static DEFINE_MUTEX(vsock_register_mutex);
> > >
> > > /**** UTILS ****/
> > > @@ -2130,7 +2132,7 @@
> EXPORT_SYMBOL_GPL(vsock_core_get_transport);
> > >
> > > int vsock_core_register(const struct vsock_transport *t, int features)
> > > {
> > > - const struct vsock_transport *t_h2g, *t_g2h, *t_dgram;
> > > + const struct vsock_transport *t_h2g, *t_g2h, *t_dgram, *t_local;
> > > int err = mutex_lock_interruptible(&vsock_register_mutex);
> > >
> > > if (err)
> > > @@ -2139,6 +2141,7 @@ int vsock_core_register(const struct
> > > vsock_transport *t, int features)
> > > t_h2g = transport_h2g;
> > > t_g2h = transport_g2h;
> > > t_dgram = transport_dgram;
> > > + t_local = transport_local;
> > >
> > > if (features & VSOCK_TRANSPORT_F_H2G) {
> > > if (t_h2g) {
> > > @@ -2164,9 +2167,18 @@ int vsock_core_register(const struct
> > > vsock_transport *t, int features)
> > > t_dgram = t;
> > > }
> > >
> > > + if (features & VSOCK_TRANSPORT_F_LOCAL) {
> > > + if (t_local) {
> > > + err = -EBUSY;
> > > + goto err_busy;
> > > + }
> > > + t_local = t;
> > > + }
> > > +
> > > transport_h2g = t_h2g;
> > > transport_g2h = t_g2h;
> > > transport_dgram = t_dgram;
> > > + transport_local = t_local;
> > >
> > > err_busy:
> > > mutex_unlock(&vsock_register_mutex);
> > > @@ -2187,6 +2199,9 @@ void vsock_core_unregister(const struct
> > > vsock_transport *t)
> > > if (transport_dgram == t)
> > > transport_dgram = NULL;
> > >
> > > + if (transport_local == t)
> > > + transport_local = NULL;
> > > +
> > > mutex_unlock(&vsock_register_mutex);
> > > }
> > > EXPORT_SYMBOL_GPL(vsock_core_unregister);
> > > --
> > > 2.21.0
> >
> > Having loopback support as a separate transport fits nicely, but do we need
> to support
> > different variants of loopback? It could just be built in.
>
> I agree with you, indeed initially I developed it as built in, but
> DEPMOD found a cyclic dependency because vsock_transport use
> virtio_transport_common that use vsock, so if I include vsock_transport
> in the vsock module, DEPMOD is not happy.
>
> I don't know how to break this cyclic dependency, do you have any ideas?

One way to view this would be that the loopback transport and the support
it uses from virtio_transport_common are independent of virtio as such,
and could be part of the af_vsock module if loopback is configured. So
in a way, the virtio g2h and h2g transports would be extensions of the
built in loopback transport. But that brings in quite a bit of code so
it could be better to just keep it as is.

Thanks,
Jorgen

2019-11-21 16:17:18

by Stefano Garzarella

Subject: Re: [PATCH net-next 3/6] vsock: add local transport support in the vsock core

On Thu, Nov 21, 2019 at 03:53:47PM +0000, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:[email protected]]
> > Sent: Thursday, November 21, 2019 4:22 PM
> >
> > On Thu, Nov 21, 2019 at 03:04:18PM +0000, Jorgen Hansen wrote:
> > > > From: Stefano Garzarella [mailto:[email protected]]
> > > > Sent: Tuesday, November 19, 2019 12:01 PM
> > > > To: [email protected]
> > > >
> > > > This patch allows to register a transport able to handle
> > > > local communication (loopback).
> > > >
> > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > ---
> > > > include/net/af_vsock.h | 2 ++
> > > > net/vmw_vsock/af_vsock.c | 17 ++++++++++++++++-
> > > > 2 files changed, 18 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> > > > index 4206dc6d813f..b1c717286993 100644
> > > > --- a/include/net/af_vsock.h
> > > > +++ b/include/net/af_vsock.h
> > > > @@ -98,6 +98,8 @@ struct vsock_transport_send_notify_data {
> > > > #define VSOCK_TRANSPORT_F_G2H 0x00000002
> > > > /* Transport provides DGRAM communication */
> > > > #define VSOCK_TRANSPORT_F_DGRAM 0x00000004
> > > > +/* Transport provides local (loopback) communication */
> > > > +#define VSOCK_TRANSPORT_F_LOCAL 0x00000008
> > > >
> > > > struct vsock_transport {
> > > > struct module *module;
> > > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > > > index cc8659838bf2..c9e5bad59dc1 100644
> > > > --- a/net/vmw_vsock/af_vsock.c
> > > > +++ b/net/vmw_vsock/af_vsock.c
> > > > @@ -136,6 +136,8 @@ static const struct vsock_transport
> > *transport_h2g;
> > > > static const struct vsock_transport *transport_g2h;
> > > > /* Transport used for DGRAM communication */
> > > > static const struct vsock_transport *transport_dgram;
> > > > +/* Transport used for local communication */
> > > > +static const struct vsock_transport *transport_local;
> > > > static DEFINE_MUTEX(vsock_register_mutex);
> > > >
> > > > /**** UTILS ****/
> > > > @@ -2130,7 +2132,7 @@
> > EXPORT_SYMBOL_GPL(vsock_core_get_transport);
> > > >
> > > > int vsock_core_register(const struct vsock_transport *t, int features)
> > > > {
> > > > - const struct vsock_transport *t_h2g, *t_g2h, *t_dgram;
> > > > + const struct vsock_transport *t_h2g, *t_g2h, *t_dgram, *t_local;
> > > > int err = mutex_lock_interruptible(&vsock_register_mutex);
> > > >
> > > > if (err)
> > > > @@ -2139,6 +2141,7 @@ int vsock_core_register(const struct
> > > > vsock_transport *t, int features)
> > > > t_h2g = transport_h2g;
> > > > t_g2h = transport_g2h;
> > > > t_dgram = transport_dgram;
> > > > + t_local = transport_local;
> > > >
> > > > if (features & VSOCK_TRANSPORT_F_H2G) {
> > > > if (t_h2g) {
> > > > @@ -2164,9 +2167,18 @@ int vsock_core_register(const struct
> > > > vsock_transport *t, int features)
> > > > t_dgram = t;
> > > > }
> > > >
> > > > + if (features & VSOCK_TRANSPORT_F_LOCAL) {
> > > > + if (t_local) {
> > > > + err = -EBUSY;
> > > > + goto err_busy;
> > > > + }
> > > > + t_local = t;
> > > > + }
> > > > +
> > > > transport_h2g = t_h2g;
> > > > transport_g2h = t_g2h;
> > > > transport_dgram = t_dgram;
> > > > + transport_local = t_local;
> > > >
> > > > err_busy:
> > > > mutex_unlock(&vsock_register_mutex);
> > > > @@ -2187,6 +2199,9 @@ void vsock_core_unregister(const struct
> > > > vsock_transport *t)
> > > > if (transport_dgram == t)
> > > > transport_dgram = NULL;
> > > >
> > > > + if (transport_local == t)
> > > > + transport_local = NULL;
> > > > +
> > > > mutex_unlock(&vsock_register_mutex);
> > > > }
> > > > EXPORT_SYMBOL_GPL(vsock_core_unregister);
> > > > --
> > > > 2.21.0
> > >
> > > Having loopback support as a separate transport fits nicely, but do we need
> > to support
> > > different variants of loopback? It could just be built in.
> >
> > I agree with you, indeed initially I developed it as built in, but
> > DEPMOD found a cyclic dependency because vsock_transport use
> > virtio_transport_common that use vsock, so if I include vsock_transport
> > in the vsock module, DEPMOD is not happy.
> >
> > I don't know how to break this cyclic dependency, do you have any ideas?
>
> One way to view this would be that the loopback transport and the support
> it uses from virtio_transport_common are independent of virtio as such,
> and could be part of the af_vsock module if loopback is configured. So
> in a way, the virtio g2h and h2g transports would be extensions of the
> built in loopback transport. But that brings in quite a bit of code so
> it could be better to just keep it as is.

Great idea!

Stefan already suggested (as a long-term goal) to rename the generic
functionality in virtio_transport_common.c

Maybe I can do both in another series later on, since it requires enough
changes.

Thanks,
Stefano

2019-11-21 16:23:48

by Jorgen Hansen

Subject: RE: [PATCH net-next 3/6] vsock: add local transport support in the vsock core

> From: Stefano Garzarella [mailto:[email protected]]
> Sent: Thursday, November 21, 2019 5:13 PM
>
> On Thu, Nov 21, 2019 at 03:53:47PM +0000, Jorgen Hansen wrote:
> > > From: Stefano Garzarella [mailto:[email protected]]
> > > Sent: Thursday, November 21, 2019 4:22 PM
> > >
> > > On Thu, Nov 21, 2019 at 03:04:18PM +0000, Jorgen Hansen wrote:
> > > > > From: Stefano Garzarella [mailto:[email protected]]
> > > > > Sent: Tuesday, November 19, 2019 12:01 PM
> > > > > To: [email protected]
> > > > >
> > > > > This patch allows to register a transport able to handle
> > > > > local communication (loopback).
> > > > >
> > > > > Signed-off-by: Stefano Garzarella <[email protected]>
> > > > > ---
> > > > > include/net/af_vsock.h | 2 ++
> > > > > net/vmw_vsock/af_vsock.c | 17 ++++++++++++++++-
> > > > > 2 files changed, 18 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> > > > > index 4206dc6d813f..b1c717286993 100644
> > > > > --- a/include/net/af_vsock.h
> > > > > +++ b/include/net/af_vsock.h
> > > > > @@ -98,6 +98,8 @@ struct vsock_transport_send_notify_data {
> > > > > #define VSOCK_TRANSPORT_F_G2H 0x00000002
> > > > > /* Transport provides DGRAM communication */
> > > > > #define VSOCK_TRANSPORT_F_DGRAM 0x00000004
> > > > > +/* Transport provides local (loopback) communication */
> > > > > +#define VSOCK_TRANSPORT_F_LOCAL 0x00000008
> > > > >
> > > > > struct vsock_transport {
> > > > > struct module *module;
> > > > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > > > > index cc8659838bf2..c9e5bad59dc1 100644
> > > > > --- a/net/vmw_vsock/af_vsock.c
> > > > > +++ b/net/vmw_vsock/af_vsock.c
> > > > > @@ -136,6 +136,8 @@ static const struct vsock_transport
> > > *transport_h2g;
> > > > > static const struct vsock_transport *transport_g2h;
> > > > > /* Transport used for DGRAM communication */
> > > > > static const struct vsock_transport *transport_dgram;
> > > > > +/* Transport used for local communication */
> > > > > +static const struct vsock_transport *transport_local;
> > > > > static DEFINE_MUTEX(vsock_register_mutex);
> > > > >
> > > > > /**** UTILS ****/
> > > > > @@ -2130,7 +2132,7 @@
> > > EXPORT_SYMBOL_GPL(vsock_core_get_transport);
> > > > >
> > > > > int vsock_core_register(const struct vsock_transport *t, int features)
> > > > > {
> > > > > - const struct vsock_transport *t_h2g, *t_g2h, *t_dgram;
> > > > > + const struct vsock_transport *t_h2g, *t_g2h, *t_dgram,
> *t_local;
> > > > > int err = mutex_lock_interruptible(&vsock_register_mutex);
> > > > >
> > > > > if (err)
> > > > > @@ -2139,6 +2141,7 @@ int vsock_core_register(const struct
> > > > > vsock_transport *t, int features)
> > > > > t_h2g = transport_h2g;
> > > > > t_g2h = transport_g2h;
> > > > > t_dgram = transport_dgram;
> > > > > + t_local = transport_local;
> > > > >
> > > > > if (features & VSOCK_TRANSPORT_F_H2G) {
> > > > > if (t_h2g) {
> > > > > @@ -2164,9 +2167,18 @@ int vsock_core_register(const struct
> > > > > vsock_transport *t, int features)
> > > > > t_dgram = t;
> > > > > }
> > > > >
> > > > > + if (features & VSOCK_TRANSPORT_F_LOCAL) {
> > > > > + if (t_local) {
> > > > > + err = -EBUSY;
> > > > > + goto err_busy;
> > > > > + }
> > > > > + t_local = t;
> > > > > + }
> > > > > +
> > > > > transport_h2g = t_h2g;
> > > > > transport_g2h = t_g2h;
> > > > > transport_dgram = t_dgram;
> > > > > + transport_local = t_local;
> > > > >
> > > > > err_busy:
> > > > > mutex_unlock(&vsock_register_mutex);
> > > > > @@ -2187,6 +2199,9 @@ void vsock_core_unregister(const struct
> > > > > vsock_transport *t)
> > > > > if (transport_dgram == t)
> > > > > transport_dgram = NULL;
> > > > >
> > > > > + if (transport_local == t)
> > > > > + transport_local = NULL;
> > > > > +
> > > > > mutex_unlock(&vsock_register_mutex);
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(vsock_core_unregister);
> > > > > --
> > > > > 2.21.0
> > > >
> > > > Having loopback support as a separate transport fits nicely, but do we
> need
> > > to support
> > > > different variants of loopback? It could just be built in.
> > >
> > > I agree with you, indeed initially I developed it as built in, but
> > > DEPMOD found a cyclic dependency because vsock_transport use
> > > virtio_transport_common that use vsock, so if I include vsock_transport
> > > in the vsock module, DEPMOD is not happy.
> > >
> > > I don't know how to break this cyclic dependency, do you have any ideas?
> >
> > One way to view this would be that the loopback transport and the support
> > it uses from virtio_transport_common are independent of virtio as such,
> > and could be part of the af_vsock module if loopback is configured. So
> > in a way, the virtio g2h and h2g transports would be extensions of the
> > built in loopback transport. But that brings in quite a bit of code so
> > it could be better to just keep it as is.
>
> Great idea!
>
> Stefan already suggested (as a long-term goal) to rename the generic
> functionality in virtio_transport_common.c
>
> Maybe I can do both in another series later on, since it requires enough
> changes.

Sounds good to me.

Thanks,
Jorgen

2019-11-22 09:27:51

by Stefan Hajnoczi

Subject: Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport

On Thu, Nov 21, 2019 at 04:25:17PM +0100, Stefano Garzarella wrote:
> On Thu, Nov 21, 2019 at 10:59:48AM +0100, Stefano Garzarella wrote:
> > On Thu, Nov 21, 2019 at 09:34:58AM +0000, Stefan Hajnoczi wrote:
> > > On Tue, Nov 19, 2019 at 12:01:19PM +0100, Stefano Garzarella wrote:
> > > > +static struct workqueue_struct *vsock_loopback_workqueue;
> > > > +static struct vsock_loopback *the_vsock_loopback;
> > >
> > > the_vsock_loopback could be a static global variable (not a pointer) and
> > > vsock_loopback_workqueue could also be included in the struct.
> > >
> > > The RCU pointer is really a way to synchronize vsock_loopback_send_pkt()
> > > and vsock_loopback_cancel_pkt() with module exit. There is no other
> > > reason for using a pointer.
> > >
> > > It's cleaner to implement the synchronization once in af_vsock.c (or
> > > virtio_transport_common.c) instead of making each transport do it.
> > > Maybe try_module_get() and related APIs provide the necessary semantics
> > > so that core vsock code can hold the transport module while it's being
> > > used to send/cancel a packet.
> >
> > Right, the module cannot be unloaded until open sockets, so here the
> > synchronization is not needed.
> >
> > The synchronization come from virtio-vsock device that can be
> > hot-unplugged while sockets are still open, but that can't happen here.
> >
> > I will remove the pointers and RCU in the v2.
> >
> > Can I keep your R-b or do you prefer to watch v2 first?

I'd like to review v2.

> > > > +MODULE_ALIAS_NETPROTO(PF_VSOCK);
> > >
> > > Why does this module define the alias for PF_VSOCK? Doesn't another
> > > module already define this alias?
> >
> > It is a way to load this module when PF_VSOCK is starting to be used.
> > MODULE_ALIAS_NETPROTO(PF_VSOCK) is already defined in vmci_transport
> > and hyperv_transport. IIUC it is used for the same reason.
> >
> > In virtio_transport we don't need it because it will be loaded when
> > the PCI device is discovered.
> >
> > Do you think there's a better way?
> > Should I include the vsock_loopback transport directly in af_vsock
> > without creating a new module?
> >
>
> That last thing I said may not be possible:
> I remembered that I tried, but DEPMOD found a cyclic dependency because
> vsock_transport use virtio_transport_common that use vsock, so if I
> include vsock_transport in the vsock module, DEPMOD is not happy.
>
> Do you think it's okay in this case to keep MODULE_ALIAS_NETPROTO(PF_VSOCK)
> or is there a better way?

The reason I asked is because the semantics of duplicate module aliases
aren't clear to me. Do all modules with the same alias get loaded?
Or just the first? Or ...?

Stefan

Attachments:

2019-11-22 10:04:03

by Stefano Garzarella

Subject: Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport

On Fri, Nov 22, 2019 at 09:25:46AM +0000, Stefan Hajnoczi wrote:
> On Thu, Nov 21, 2019 at 04:25:17PM +0100, Stefano Garzarella wrote:
> > On Thu, Nov 21, 2019 at 10:59:48AM +0100, Stefano Garzarella wrote:
> > > On Thu, Nov 21, 2019 at 09:34:58AM +0000, Stefan Hajnoczi wrote:
> > > > On Tue, Nov 19, 2019 at 12:01:19PM +0100, Stefano Garzarella wrote:
> > > > > +static struct workqueue_struct *vsock_loopback_workqueue;
> > > > > +static struct vsock_loopback *the_vsock_loopback;
> > > >
> > > > the_vsock_loopback could be a static global variable (not a pointer) and
> > > > vsock_loopback_workqueue could also be included in the struct.
> > > >
> > > > The RCU pointer is really a way to synchronize vsock_loopback_send_pkt()
> > > > and vsock_loopback_cancel_pkt() with module exit. There is no other
> > > > reason for using a pointer.
> > > >
> > > > It's cleaner to implement the synchronization once in af_vsock.c (or
> > > > virtio_transport_common.c) instead of making each transport do it.
> > > > Maybe try_module_get() and related APIs provide the necessary semantics
> > > > so that core vsock code can hold the transport module while it's being
> > > > used to send/cancel a packet.
> > >
> > > Right, the module cannot be unloaded until open sockets, so here the
> > > synchronization is not needed.
> > >
> > > The synchronization come from virtio-vsock device that can be
> > > hot-unplugged while sockets are still open, but that can't happen here.
> > >
> > > I will remove the pointers and RCU in the v2.
> > >
> > > Can I keep your R-b or do you prefer to watch v2 first?
>
> I'd like to review v2.
>

Sure!

> > > > > +MODULE_ALIAS_NETPROTO(PF_VSOCK);
> > > >
> > > > Why does this module define the alias for PF_VSOCK? Doesn't another
> > > > module already define this alias?
> > >
> > > It is a way to load this module when PF_VSOCK is starting to be used.
> > > MODULE_ALIAS_NETPROTO(PF_VSOCK) is already defined in vmci_transport
> > > and hyperv_transport. IIUC it is used for the same reason.
> > >
> > > In virtio_transport we don't need it because it will be loaded when
> > > the PCI device is discovered.
> > >
> > > Do you think there's a better way?
> > > Should I include the vsock_loopback transport directly in af_vsock
> > > without creating a new module?
> > >
> >
> > That last thing I said may not be possible:
> > I remembered that I tried, but DEPMOD found a cyclic dependency because
> > vsock_transport use virtio_transport_common that use vsock, so if I
> > include vsock_transport in the vsock module, DEPMOD is not happy.
> >
> > Do you think it's okay in this case to keep MODULE_ALIAS_NETPROTO(PF_VSOCK)
> > or is there a better way?
>
> The reason I asked is because the semantics of duplicate module aliases
> aren't clear to me. Do all modules with the same alias get loaded?
> Or just the first? Or ...?

It wasn't clear to me either, but when I tried, I saw that all modules
with the same alias got loaded.

Stefano