* [PATCH 1/3] vsockmon: Add tap functions.
2016-08-08 16:14 [PATCH 0/3] vsockmon: virtual device to monitor AF_VSOCK sockets ggarcia
@ 2016-08-08 16:14 ` ggarcia
2016-08-10 11:40 ` Stefan Hajnoczi
2016-08-08 16:14 ` [PATCH 2/3] vsockmon: Add vsockmon device ggarcia
2016-08-08 16:14 ` [PATCH 3/3] vsockmon: Add virtio vsock hooks ggarcia
2 siblings, 1 reply; 13+ messages in thread
From: ggarcia @ 2016-08-08 16:14 UTC (permalink / raw)
To: netdev; +Cc: stefanha, jhansen, mst, Gerard Garcia
From: Gerard Garcia <ggarcia@deic.uab.cat>
Add tap functions that can be used by the vsock transports to
deliver packets to vsockmon virtual network devices.
Signed-off-by: Gerard Garcia <ggarcia@deic.uab.cat>
---
include/net/af_vsock.h | 13 +++++
include/uapi/linux/if_arp.h | 1 +
net/vmw_vsock/Makefile | 2 +-
net/vmw_vsock/af_vsock_tap.c | 114 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 129 insertions(+), 1 deletion(-)
create mode 100644 net/vmw_vsock/af_vsock_tap.c
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index f275896..f7c51b1 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -185,4 +185,17 @@ struct sock *vsock_find_connected_socket(struct sockaddr_vm *src,
void vsock_remove_sock(struct vsock_sock *vsk);
void vsock_for_each_connected_socket(void (*fn)(struct sock *sk));
+/**** TAP ****/
+
+struct vsock_tap {
+ struct net_device *dev;
+ struct module *module;
+ struct list_head list;
+};
+
+int vsock_init_tap(void);
+int vsock_add_tap(struct vsock_tap *vt);
+int vsock_remove_tap(struct vsock_tap *vt);
+void vsock_deliver_tap(struct sk_buff *skb);
+
#endif /* __AF_VSOCK_H__ */
diff --git a/include/uapi/linux/if_arp.h b/include/uapi/linux/if_arp.h
index 4d024d7..cf73510 100644
--- a/include/uapi/linux/if_arp.h
+++ b/include/uapi/linux/if_arp.h
@@ -95,6 +95,7 @@
#define ARPHRD_IP6GRE 823 /* GRE over IPv6 */
#define ARPHRD_NETLINK 824 /* Netlink header */
#define ARPHRD_6LOWPAN 825 /* IPv6 over LoWPAN */
+#define ARPHRD_VSOCKMON 826 /* Vsock monitor header */
#define ARPHRD_VOID 0xFFFF /* Void type, nothing is known */
#define ARPHRD_NONE 0xFFFE /* zero header length */
diff --git a/net/vmw_vsock/Makefile b/net/vmw_vsock/Makefile
index bc27c70..09fc2eb 100644
--- a/net/vmw_vsock/Makefile
+++ b/net/vmw_vsock/Makefile
@@ -3,7 +3,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
-vsock-y += af_vsock.o vsock_addr.o
+vsock-y += af_vsock.o af_vsock_tap.o vsock_addr.o
vmw_vsock_vmci_transport-y += vmci_transport.o vmci_transport_notify.o \
vmci_transport_notify_qstate.o
diff --git a/net/vmw_vsock/af_vsock_tap.c b/net/vmw_vsock/af_vsock_tap.c
new file mode 100644
index 0000000..427b3b3
--- /dev/null
+++ b/net/vmw_vsock/af_vsock_tap.c
@@ -0,0 +1,114 @@
+/*
+ * Tap functions for AF_VSOCK sockets.
+ *
+ * Code based on net/netlink/af_netlink.c tap functions.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <net/sock.h>
+#include <net/af_vsock.h>
+#include <linux/if_arp.h>
+
+static DEFINE_SPINLOCK(vsock_tap_lock);
+static struct list_head vsock_tap_all __read_mostly =
+ LIST_HEAD_INIT(vsock_tap_all);
+
+int vsock_add_tap(struct vsock_tap *vt) {
+ if (unlikely(vt->dev->type != ARPHRD_VSOCKMON))
+ return -EINVAL;
+
+ __module_get(vt->module);
+
+ spin_lock(&vsock_tap_lock);
+ list_add_rcu(&vt->list, &vsock_tap_all);
+ spin_unlock(&vsock_tap_lock);
+
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(vsock_add_tap);
+
+int __vsock_remove_tap(struct vsock_tap *vt) {
+ bool found = false;
+ struct vsock_tap *tmp;
+
+ spin_lock(&vsock_tap_lock);
+
+ list_for_each_entry(tmp, &vsock_tap_all, list) {
+ if (vt == tmp) {
+ list_del_rcu(&vt->list);
+ found = true;
+ goto out;
+ }
+ }
+
+ pr_warn("__vsock_remove_tap: %p not found\n", vt);
+out:
+ spin_unlock(&vsock_tap_lock);
+
+ if (found)
+ module_put(vt->module);
+
+ return found ? 0 : -ENODEV;
+}
+
+int vsock_remove_tap(struct vsock_tap *vt)
+{
+ int ret;
+
+ ret = __vsock_remove_tap(vt);
+ synchronize_net();
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(vsock_remove_tap);
+
+static int __vsock_deliver_tap_skb(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ int ret = 0;
+
+ if (skb) {
+ dev_hold(dev);
+ /* Take skb ownership so it is not consumed in dev_queue_xmit.
+ * dev_queue_xmit will drop a reference so the reference count
+ * will reset.
+ */
+ skb_get(skb);
+ skb->dev = dev;
+ ret = dev_queue_xmit(skb);
+ if (unlikely(ret > 0))
+ ret = net_xmit_errno(ret);
+
+ dev_put(dev);
+ }
+
+ return ret;
+}
+
+static void __vsock_deliver_tap(struct sk_buff *skb)
+{
+ int ret;
+ struct vsock_tap *tmp;
+
+ list_for_each_entry_rcu(tmp, &vsock_tap_all, list) {
+ ret = __vsock_deliver_tap_skb(skb, tmp->dev);
+ if (unlikely(ret))
+ break;
+ }
+}
+
+void vsock_deliver_tap(struct sk_buff *skb)
+{
+ rcu_read_lock();
+
+ if (unlikely(!list_empty(&vsock_tap_all)))
+ __vsock_deliver_tap(skb);
+
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(vsock_deliver_tap);
--
2.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] vsockmon: Add tap functions.
2016-08-08 16:14 ` [PATCH 1/3] vsockmon: Add tap functions ggarcia
@ 2016-08-10 11:40 ` Stefan Hajnoczi
2016-08-12 14:15 ` Gerard Garcia
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Hajnoczi @ 2016-08-10 11:40 UTC (permalink / raw)
To: ggarcia; +Cc: netdev, jhansen, mst
[-- Attachment #1: Type: text/plain, Size: 592 bytes --]
On Mon, Aug 08, 2016 at 06:14:40PM +0200, ggarcia@abra.uab.cat wrote:
> +static int __vsock_deliver_tap_skb(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + int ret = 0;
> +
> + if (skb) {
> + dev_hold(dev);
> + /* Take skb ownership so it is not consumed in dev_queue_xmit.
> + * dev_queue_xmit will drop a reference so the reference count
> + * will reset.
> + */
> + skb_get(skb);
Netlink clones the skb instead of adding a reference. I guess this is
because the skb might be modified later on? Perhaps there are race
conditions if the original skb is shared.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] vsockmon: Add tap functions.
2016-08-10 11:40 ` Stefan Hajnoczi
@ 2016-08-12 14:15 ` Gerard Garcia
0 siblings, 0 replies; 13+ messages in thread
From: Gerard Garcia @ 2016-08-12 14:15 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: netdev, jhansen, mst
On 08/10/2016 01:40 PM, Stefan Hajnoczi wrote:
> On Mon, Aug 08, 2016 at 06:14:40PM +0200, ggarcia@abra.uab.cat wrote:
>> +static int __vsock_deliver_tap_skb(struct sk_buff *skb,
>> + struct net_device *dev)
>> +{
>> + int ret = 0;
>> +
>> + if (skb) {
>> + dev_hold(dev);
>> + /* Take skb ownership so it is not consumed in dev_queue_xmit.
>> + * dev_queue_xmit will drop a reference so the reference count
>> + * will reset.
>> + */
>> + skb_get(skb);
>
> Netlink clones the skb instead of adding a reference. I guess this is
> because the skb might be modified later on? Perhaps there are race
> conditions if the original skb is shared.
>
Seems that it is responsibility of the functions processing the skb to
make sure that it is not modified (without performing a copy) if it is a
shared skb, as it is the case. vsockmon doesn't modify it and from what
I understand it is not modified along the tx path but, of course, I
could be missing something.
As it is not performance critical it will be safer to just clone the skb
so I'll do that.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] vsockmon: Add vsockmon device.
2016-08-08 16:14 [PATCH 0/3] vsockmon: virtual device to monitor AF_VSOCK sockets ggarcia
2016-08-08 16:14 ` [PATCH 1/3] vsockmon: Add tap functions ggarcia
@ 2016-08-08 16:14 ` ggarcia
2016-08-10 11:51 ` Stefan Hajnoczi
2016-08-08 16:14 ` [PATCH 3/3] vsockmon: Add virtio vsock hooks ggarcia
2 siblings, 1 reply; 13+ messages in thread
From: ggarcia @ 2016-08-08 16:14 UTC (permalink / raw)
To: netdev; +Cc: stefanha, jhansen, mst, Gerard Garcia
From: Gerard Garcia <ggarcia@deic.uab.cat>
Add vsockmon virtual network device that receives packets from the vsock
transports and exposes them to user space.
Based on the nlmon device.
Signed-off-by: Gerard Garcia <ggarcia@deic.uab.cat>
---
drivers/net/Kconfig | 8 ++
drivers/net/Makefile | 1 +
drivers/net/vsockmon.c | 168 ++++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/vsockmon.h | 35 +++++++++
5 files changed, 213 insertions(+)
create mode 100644 drivers/net/vsockmon.c
create mode 100644 include/uapi/linux/vsockmon.h
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 0c5415b..42c43b6 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -330,6 +330,14 @@ config NET_VRF
This option enables the support for mapping interfaces into VRF's. The
support enables VRF devices.
+config VSOCKMON
+ tristate "Virtual vsock monitoring device"
+ depends on VHOST_VSOCK
+ ---help---
+ This option enables a monitoring net device for vsock sockets. It is
+ mostly intended for developers or support to debug vsock issues. If
+ unsure, say N.
+
endif # NET_CORE
config SUNGEM_PHY
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 7336cbd..e2188d4 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_GENEVE) += geneve.o
obj-$(CONFIG_GTP) += gtp.o
obj-$(CONFIG_NLMON) += nlmon.o
obj-$(CONFIG_NET_VRF) += vrf.o
+obj-$(CONFIG_VSOCKMON) += vsockmon.o
#
# Networking Drivers
diff --git a/drivers/net/vsockmon.c b/drivers/net/vsockmon.c
new file mode 100644
index 0000000..9ad4f0a
--- /dev/null
+++ b/drivers/net/vsockmon.c
@@ -0,0 +1,168 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/if_arp.h>
+#include <net/rtnetlink.h>
+#include <net/sock.h>
+#include <net/af_vsock.h>
+#include <uapi/linux/vsockmon.h>
+#include <linux/virtio_vsock.h>
+
+/* Virtio transport max packet size plus header */
+#define DEFAULT_MTU VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + sizeof(struct af_vsockmon_hdr);
+
+struct pcpu_lstats {
+ u64 rx_packets;
+ u64 rx_bytes;
+ struct u64_stats_sync syncp;
+};
+
+static int vsockmon_dev_init(struct net_device *dev)
+{
+ dev->lstats = netdev_alloc_pcpu_stats(struct pcpu_lstats);
+ return dev->lstats == NULL ? -ENOMEM : 0;
+}
+
+static void vsockmon_dev_uninit(struct net_device *dev)
+{
+ free_percpu(dev->lstats);
+}
+
+struct vsockmon {
+ struct vsock_tap vt;
+};
+
+static int vsockmon_open(struct net_device *dev)
+{
+ struct vsockmon *vsockmon = netdev_priv(dev);
+
+ vsockmon->vt.dev = dev;
+ vsockmon->vt.module = THIS_MODULE;
+ return vsock_add_tap(&vsockmon->vt);
+}
+
+static int vsockmon_close(struct net_device *dev) {
+ struct vsockmon *vsockmon = netdev_priv(dev);
+
+ return vsock_remove_tap(&vsockmon->vt);
+}
+
+static netdev_tx_t vsockmon_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+ int len = skb->len;
+ struct pcpu_lstats *stats = this_cpu_ptr(dev->lstats);
+
+ u64_stats_update_begin(&stats->syncp);
+ stats->rx_bytes += len;
+ stats->rx_packets++;
+ u64_stats_update_end(&stats->syncp);
+
+ dev_kfree_skb(skb);
+
+ return NETDEV_TX_OK;
+}
+
+static struct rtnl_link_stats64 *
+vsockmon_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
+{
+ int i;
+ u64 bytes = 0, packets = 0;
+
+ for_each_possible_cpu(i) {
+ const struct pcpu_lstats *vstats;
+ u64 tbytes, tpackets;
+ unsigned int start;
+
+ vstats = per_cpu_ptr(dev->lstats, i);
+
+ do {
+ start = u64_stats_fetch_begin_irq(&vstats->syncp);
+ tbytes = vstats->rx_bytes;
+ tpackets = vstats->rx_packets;
+ } while (u64_stats_fetch_retry_irq(&vstats->syncp, start));
+
+ packets += tpackets;
+ bytes += tbytes;
+ }
+
+ stats->rx_packets = packets;
+ stats->tx_packets = 0;
+
+ stats->rx_bytes = bytes;
+ stats->tx_bytes = 0;
+
+ return stats;
+}
+
+static int vsockmon_is_valid_mtu(int new_mtu)
+{
+ return new_mtu >= (int) sizeof(struct af_vsockmon_hdr);
+}
+
+static int vsockmon_change_mtu(struct net_device *dev, int new_mtu)
+{
+ if (!vsockmon_is_valid_mtu(new_mtu))
+ return -EINVAL;
+
+ dev->mtu = new_mtu;
+ return 0;
+}
+
+static const struct net_device_ops vsockmon_ops = {
+ .ndo_init = vsockmon_dev_init,
+ .ndo_uninit = vsockmon_dev_uninit,
+ .ndo_open = vsockmon_open,
+ .ndo_stop = vsockmon_close,
+ .ndo_start_xmit = vsockmon_xmit,
+ .ndo_get_stats64 = vsockmon_get_stats64,
+ .ndo_change_mtu = vsockmon_change_mtu,
+};
+
+static u32 always_on(struct net_device *dev)
+{
+ return 1;
+}
+
+static const struct ethtool_ops vsockmon_ethtool_ops = {
+ .get_link = always_on,
+};
+
+static void vsockmon_setup(struct net_device *dev)
+{
+ dev->type = ARPHRD_VSOCKMON;
+ dev->priv_flags |= IFF_NO_QUEUE;
+
+ dev->netdev_ops = &vsockmon_ops;
+ dev->ethtool_ops = &vsockmon_ethtool_ops;
+ dev->destructor = free_netdev;
+
+ dev->features = NETIF_F_SG | NETIF_F_FRAGLIST |
+ NETIF_F_HIGHDMA | NETIF_F_LLTX;
+
+ dev->flags = IFF_NOARP;
+
+ dev->mtu = DEFAULT_MTU;
+}
+
+static struct rtnl_link_ops vsockmon_link_ops __read_mostly = {
+ .kind = "vsockmon",
+ .priv_size = sizeof(struct vsockmon),
+ .setup = vsockmon_setup,
+};
+
+static __init int vsockmon_register(void)
+{
+ return rtnl_link_register(&vsockmon_link_ops);
+}
+
+static __exit void vsockmon_unregister(void)
+{
+ rtnl_link_unregister(&vsockmon_link_ops);
+}
+
+module_init(vsockmon_register);
+module_exit(vsockmon_unregister);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Gerard Garcia <ggarcia@deic.uab.cat>");
+MODULE_DESCRIPTION("Vsock monitoring device. Based on nlmon device.");
+MODULE_ALIAS_RTNL_LINK("vsockmon");
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 3cf0116..380f485 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -455,6 +455,7 @@ header-y += virtio_scsi.h
header-y += virtio_types.h
header-y += virtio_vsock.h
header-y += vm_sockets.h
+header-y += vsockmon.h
header-y += vt.h
header-y += wait.h
header-y += wanrouter.h
diff --git a/include/uapi/linux/vsockmon.h b/include/uapi/linux/vsockmon.h
new file mode 100644
index 0000000..739b4bf
--- /dev/null
+++ b/include/uapi/linux/vsockmon.h
@@ -0,0 +1,35 @@
+#ifndef _UAPI_VSOCKMON_H
+#define _UAPI_VSOCKMON_H
+
+#include <linux/virtio_vsock.h>
+
+/* Structure of packets received trought the vsockmon device. */
+
+struct af_vsockmon_hdr {
+ __le64 src_cid;
+ __le64 dst_cid;
+ __le32 src_port;
+ __le32 dst_port;
+ __le16 op; /* enum af_vsockmon_op */
+ __le16 t; /* enum af_vosckmon_t */
+ __le16 len; /* sizeof(t_hdr) */
+ union {
+ struct virtio_vsock_hdr virtio_hdr;
+ } t_hdr;
+} __attribute__((packed));
+
+enum af_vsockmon_op {
+ AF_VSOCK_OP_UNKNOWN = 0,
+ AF_VSOCK_OP_CONNECT = 1,
+ AF_VSOCK_OP_DISCONNECT = 2,
+ AF_VSOCK_OP_CONTROL = 3,
+ AF_VSOCK_OP_PAYLOAD = 4,
+};
+
+enum af_vsockmon_t {
+ AF_VSOCK_T_UNKNOWN = 0,
+ AF_VSOCK_T_NO_INFO = 1, /* No transport information */
+ AF_VSOCK_T_VIRTIO = 2, /* Virtio transport header */
+};
+
+#endif
--
2.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] vsockmon: Add vsockmon device.
2016-08-08 16:14 ` [PATCH 2/3] vsockmon: Add vsockmon device ggarcia
@ 2016-08-10 11:51 ` Stefan Hajnoczi
2016-08-12 14:17 ` Gerard Garcia
2016-08-15 16:38 ` David Laight
0 siblings, 2 replies; 13+ messages in thread
From: Stefan Hajnoczi @ 2016-08-10 11:51 UTC (permalink / raw)
To: ggarcia; +Cc: netdev, jhansen, mst
[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]
On Mon, Aug 08, 2016 at 06:14:41PM +0200, ggarcia@abra.uab.cat wrote:
> diff --git a/include/uapi/linux/vsockmon.h b/include/uapi/linux/vsockmon.h
> new file mode 100644
> index 0000000..739b4bf
> --- /dev/null
> +++ b/include/uapi/linux/vsockmon.h
> @@ -0,0 +1,35 @@
> +#ifndef _UAPI_VSOCKMON_H
> +#define _UAPI_VSOCKMON_H
> +
> +#include <linux/virtio_vsock.h>
> +
> +/* Structure of packets received trought the vsockmon device. */
> +
> +struct af_vsockmon_hdr {
> + __le64 src_cid;
> + __le64 dst_cid;
> + __le32 src_port;
> + __le32 dst_port;
> + __le16 op; /* enum af_vsockmon_op */
> + __le16 t; /* enum af_vosckmon_t */
> + __le16 len; /* sizeof(t_hdr) */
> + union {
> + struct virtio_vsock_hdr virtio_hdr;
> + } t_hdr;
> +} __attribute__((packed));
This struct will change in size if/when VMCI transport support is added.
Existing binaries that were complied against the old header file would
use an incorrect size.
It would be cleaner to drop t_hdr from the struct and force users to
explicitly use af_vsockmon_hdr.len to handle the size of the headers.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] vsockmon: Add vsockmon device.
2016-08-10 11:51 ` Stefan Hajnoczi
@ 2016-08-12 14:17 ` Gerard Garcia
2016-08-15 16:38 ` David Laight
1 sibling, 0 replies; 13+ messages in thread
From: Gerard Garcia @ 2016-08-12 14:17 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: netdev, jhansen, mst
On 08/10/2016 01:51 PM, Stefan Hajnoczi wrote:
> On Mon, Aug 08, 2016 at 06:14:41PM +0200, ggarcia@abra.uab.cat wrote:
>> diff --git a/include/uapi/linux/vsockmon.h b/include/uapi/linux/vsockmon.h
>> new file mode 100644
>> index 0000000..739b4bf
>> --- /dev/null
>> +++ b/include/uapi/linux/vsockmon.h
>> @@ -0,0 +1,35 @@
>> +#ifndef _UAPI_VSOCKMON_H
>> +#define _UAPI_VSOCKMON_H
>> +
>> +#include <linux/virtio_vsock.h>
>> +
>> +/* Structure of packets received trought the vsockmon device. */
>> +
>> +struct af_vsockmon_hdr {
>> + __le64 src_cid;
>> + __le64 dst_cid;
>> + __le32 src_port;
>> + __le32 dst_port;
>> + __le16 op; /* enum af_vsockmon_op */
>> + __le16 t; /* enum af_vosckmon_t */
>> + __le16 len; /* sizeof(t_hdr) */
>> + union {
>> + struct virtio_vsock_hdr virtio_hdr;
>> + } t_hdr;
>> +} __attribute__((packed));
>
> This struct will change in size if/when VMCI transport support is added.
> Existing binaries that were complied against the old header file would
> use an incorrect size.
>
> It would be cleaner to drop t_hdr from the struct and force users to
> explicitly use af_vsockmon_hdr.len to handle the size of the headers.
>
Ok, I'll remove the t_hdr union and put a comment explaining that after
the vsockmon header there are len bytes of the transport header.
^ permalink raw reply [flat|nested] 13+ messages in thread* RE: [PATCH 2/3] vsockmon: Add vsockmon device.
2016-08-10 11:51 ` Stefan Hajnoczi
2016-08-12 14:17 ` Gerard Garcia
@ 2016-08-15 16:38 ` David Laight
2016-08-15 17:23 ` David Miller
1 sibling, 1 reply; 13+ messages in thread
From: David Laight @ 2016-08-15 16:38 UTC (permalink / raw)
To: 'Stefan Hajnoczi', ggarcia@abra.uab.cat
Cc: netdev@vger.kernel.org, jhansen@vmware.com, mst@redhat.com
From: Stefan Hajnoczi
> Sent: 10 August 2016 12:52
> On Mon, Aug 08, 2016 at 06:14:41PM +0200, ggarcia@abra.uab.cat wrote:
> > diff --git a/include/uapi/linux/vsockmon.h b/include/uapi/linux/vsockmon.h
> > new file mode 100644
> > index 0000000..739b4bf
> > --- /dev/null
> > +++ b/include/uapi/linux/vsockmon.h
> > @@ -0,0 +1,35 @@
> > +#ifndef _UAPI_VSOCKMON_H
> > +#define _UAPI_VSOCKMON_H
> > +
> > +#include <linux/virtio_vsock.h>
> > +
> > +/* Structure of packets received trought the vsockmon device. */
> > +
> > +struct af_vsockmon_hdr {
> > + __le64 src_cid;
> > + __le64 dst_cid;
> > + __le32 src_port;
> > + __le32 dst_port;
> > + __le16 op; /* enum af_vsockmon_op */
> > + __le16 t; /* enum af_vosckmon_t */
> > + __le16 len; /* sizeof(t_hdr) */
> > + union {
> > + struct virtio_vsock_hdr virtio_hdr;
> > + } t_hdr;
> > +} __attribute__((packed));
...
Gah, another 'packed' structure.
Have you looked at the amount of code the sparc64 compiler generates
to access the structure members??
You really want to add another 16bit field and enforce 64bit alignment
on the header and all data blocks.
David
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] vsockmon: Add vsockmon device.
2016-08-15 16:38 ` David Laight
@ 2016-08-15 17:23 ` David Miller
2016-08-22 10:12 ` Gerard Garcia
0 siblings, 1 reply; 13+ messages in thread
From: David Miller @ 2016-08-15 17:23 UTC (permalink / raw)
To: David.Laight; +Cc: stefanha, ggarcia, netdev, jhansen, mst
From: David Laight <David.Laight@ACULAB.COM>
Date: Mon, 15 Aug 2016 16:38:36 +0000
> From: Stefan Hajnoczi
>> Sent: 10 August 2016 12:52
>> On Mon, Aug 08, 2016 at 06:14:41PM +0200, ggarcia@abra.uab.cat wrote:
>> > diff --git a/include/uapi/linux/vsockmon.h b/include/uapi/linux/vsockmon.h
>> > new file mode 100644
>> > index 0000000..739b4bf
>> > --- /dev/null
>> > +++ b/include/uapi/linux/vsockmon.h
>> > @@ -0,0 +1,35 @@
>> > +#ifndef _UAPI_VSOCKMON_H
>> > +#define _UAPI_VSOCKMON_H
>> > +
>> > +#include <linux/virtio_vsock.h>
>> > +
>> > +/* Structure of packets received trought the vsockmon device. */
>> > +
>> > +struct af_vsockmon_hdr {
>> > + __le64 src_cid;
>> > + __le64 dst_cid;
>> > + __le32 src_port;
>> > + __le32 dst_port;
>> > + __le16 op; /* enum af_vsockmon_op */
>> > + __le16 t; /* enum af_vosckmon_t */
>> > + __le16 len; /* sizeof(t_hdr) */
>> > + union {
>> > + struct virtio_vsock_hdr virtio_hdr;
>> > + } t_hdr;
>> > +} __attribute__((packed));
> ...
>
> Gah, another 'packed' structure.
> Have you looked at the amount of code the sparc64 compiler generates
> to access the structure members??
>
> You really want to add another 16bit field and enforce 64bit alignment
> on the header and all data blocks.
Indeed, avoid the packed attribute at all costs.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] vsockmon: Add vsockmon device.
2016-08-15 17:23 ` David Miller
@ 2016-08-22 10:12 ` Gerard Garcia
0 siblings, 0 replies; 13+ messages in thread
From: Gerard Garcia @ 2016-08-22 10:12 UTC (permalink / raw)
To: David Miller, David.Laight; +Cc: stefanha, netdev, jhansen, mst
On 08/15/2016 07:23 PM, David Miller wrote:
> From: David Laight <David.Laight@ACULAB.COM>
> Date: Mon, 15 Aug 2016 16:38:36 +0000
>
>> From: Stefan Hajnoczi
>>> Sent: 10 August 2016 12:52
>>> On Mon, Aug 08, 2016 at 06:14:41PM +0200, ggarcia@abra.uab.cat wrote:
>>>> diff --git a/include/uapi/linux/vsockmon.h b/include/uapi/linux/vsockmon.h
>>>> new file mode 100644
>>>> index 0000000..739b4bf
>>>> --- /dev/null
>>>> +++ b/include/uapi/linux/vsockmon.h
>>>> @@ -0,0 +1,35 @@
>>>> +#ifndef _UAPI_VSOCKMON_H
>>>> +#define _UAPI_VSOCKMON_H
>>>> +
>>>> +#include <linux/virtio_vsock.h>
>>>> +
>>>> +/* Structure of packets received trought the vsockmon device. */
>>>> +
>>>> +struct af_vsockmon_hdr {
>>>> + __le64 src_cid;
>>>> + __le64 dst_cid;
>>>> + __le32 src_port;
>>>> + __le32 dst_port;
>>>> + __le16 op; /* enum af_vsockmon_op */
>>>> + __le16 t; /* enum af_vosckmon_t */
>>>> + __le16 len; /* sizeof(t_hdr) */
>>>> + union {
>>>> + struct virtio_vsock_hdr virtio_hdr;
>>>> + } t_hdr;
>>>> +} __attribute__((packed));
>> ...
>>
>> Gah, another 'packed' structure.
>> Have you looked at the amount of code the sparc64 compiler generates
>> to access the structure members??
>>
>> You really want to add another 16bit field and enforce 64bit alignment
>> on the header and all data blocks.
>
> Indeed, avoid the packed attribute at all costs.
>
I understand. I'll add another 16b field so it is aligned and avoid the
packed attribute.
Gerard
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] vsockmon: Add virtio vsock hooks
2016-08-08 16:14 [PATCH 0/3] vsockmon: virtual device to monitor AF_VSOCK sockets ggarcia
2016-08-08 16:14 ` [PATCH 1/3] vsockmon: Add tap functions ggarcia
2016-08-08 16:14 ` [PATCH 2/3] vsockmon: Add vsockmon device ggarcia
@ 2016-08-08 16:14 ` ggarcia
2016-08-10 11:59 ` Stefan Hajnoczi
2 siblings, 1 reply; 13+ messages in thread
From: ggarcia @ 2016-08-08 16:14 UTC (permalink / raw)
To: netdev; +Cc: stefanha, jhansen, mst, Gerard Garcia
From: Gerard Garcia <ggarcia@deic.uab.cat>
Add hooks to the virtio transport host driver to deliver a copy of
the received and sent messages to all vsockmon virtual network devices.
Signed-off-by: Gerard Garcia <ggarcia@deic.uab.cat>
---
drivers/vhost/vsock.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 0ddf3a2..75b5a46 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -15,8 +15,10 @@
#include <net/sock.h>
#include <linux/virtio_vsock.h>
#include <linux/vhost.h>
+#include <linux/skbuff.h>
#include <net/af_vsock.h>
+#include <uapi/linux/vsockmon.h>
#include "vhost.h"
#define VHOST_VSOCK_DEFAULT_HOST_CID 2
@@ -45,6 +47,68 @@ struct vhost_vsock {
u32 guest_cid;
};
+static struct sk_buff *
+virtio_vsock_pkt_vsockmon_to_vsockmon_skb(struct virtio_vsock_pkt *pkt)
+{
+ struct sk_buff *skb;
+ struct af_vsockmon_hdr *hdr;
+ void *payload;
+
+ u32 skb_len = sizeof(struct af_vsockmon_hdr) + pkt->len;
+
+ skb = alloc_skb(skb_len, GFP_ATOMIC);
+ if (!skb)
+ return NULL;
+
+ hdr = (struct af_vsockmon_hdr *) skb_put(skb, sizeof(*hdr));
+
+ hdr->src_cid = pkt->hdr.src_cid;
+ hdr->src_port = pkt->hdr.src_port;
+ hdr->dst_cid = pkt->hdr.dst_cid;
+ hdr->dst_port = pkt->hdr.dst_port;
+ hdr->t = cpu_to_le16(AF_VSOCK_T_VIRTIO);
+ hdr->len = cpu_to_le16(sizeof(hdr->t_hdr));
+
+ switch(pkt->hdr.op) {
+ case VIRTIO_VSOCK_OP_REQUEST:
+ case VIRTIO_VSOCK_OP_RESPONSE:
+ hdr->op = cpu_to_le16(AF_VSOCK_OP_CONNECT);
+ break;
+ case VIRTIO_VSOCK_OP_RST:
+ case VIRTIO_VSOCK_OP_SHUTDOWN:
+ hdr->op = cpu_to_le16(AF_VSOCK_OP_DISCONNECT);
+ break;
+ case VIRTIO_VSOCK_OP_RW:
+ hdr->op = cpu_to_le16(AF_VSOCK_OP_PAYLOAD);
+ break;
+ case VIRTIO_VSOCK_OP_CREDIT_UPDATE:
+ case VIRTIO_VSOCK_OP_CREDIT_REQUEST:
+ hdr->op = cpu_to_le16(AF_VSOCK_OP_CONTROL);
+ break;
+ default:
+ hdr->op = cpu_to_le16(AF_VSOCK_OP_UNKNOWN);
+ break;
+ }
+
+ hdr->t_hdr.virtio_hdr = pkt->hdr;
+
+ if (pkt->len) {
+ payload = skb_put(skb, pkt->len);
+ memcpy(payload, pkt->buf, pkt->len);
+ }
+
+ return skb;
+}
+
+static void vsock_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
+{
+ struct sk_buff *skb = virtio_vsock_pkt_to_vsockmon_skb(pkt);
+ if (skb) {
+ vsock_deliver_tap(skb);
+ kfree_skb(skb);
+ }
+}
+
static u32 vhost_transport_get_local_cid(void)
{
return VHOST_VSOCK_DEFAULT_HOST_CID;
@@ -168,6 +232,11 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
restart_tx = true;
}
+ /* Deliver to monitoring devices all correctly transmitted
+ * packets.
+ */
+ vsock_deliver_tap_pkt(pkt);
+
virtio_transport_free_pkt(pkt);
}
if (added)
@@ -334,6 +403,9 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
continue;
}
+ /* Deliver to monitoring devices all received packets */
+ vsock_deliver_tap_pkt(pkt);
+
/* Only accept correctly addressed packets */
if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid)
virtio_transport_recv_pkt(pkt);
--
2.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] vsockmon: Add virtio vsock hooks
2016-08-08 16:14 ` [PATCH 3/3] vsockmon: Add virtio vsock hooks ggarcia
@ 2016-08-10 11:59 ` Stefan Hajnoczi
2016-08-12 14:18 ` Gerard Garcia
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Hajnoczi @ 2016-08-10 11:59 UTC (permalink / raw)
To: ggarcia; +Cc: netdev, jhansen, mst
[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]
On Mon, Aug 08, 2016 at 06:14:42PM +0200, ggarcia@abra.uab.cat wrote:
> +static struct sk_buff *
> +virtio_vsock_pkt_vsockmon_to_vsockmon_skb(struct virtio_vsock_pkt *pkt)
> +{
> + struct sk_buff *skb;
> + struct af_vsockmon_hdr *hdr;
> + void *payload;
> +
> + u32 skb_len = sizeof(struct af_vsockmon_hdr) + pkt->len;
> +
> + skb = alloc_skb(skb_len, GFP_ATOMIC);
> + if (!skb)
> + return NULL;
> +
> + hdr = (struct af_vsockmon_hdr *) skb_put(skb, sizeof(*hdr));
> +
> + hdr->src_cid = pkt->hdr.src_cid;
> + hdr->src_port = pkt->hdr.src_port;
> + hdr->dst_cid = pkt->hdr.dst_cid;
> + hdr->dst_port = pkt->hdr.dst_port;
> + hdr->t = cpu_to_le16(AF_VSOCK_T_VIRTIO);
> + hdr->len = cpu_to_le16(sizeof(hdr->t_hdr));
> +
> + switch(pkt->hdr.op) {
Missing le16_to_cpu()
> + case VIRTIO_VSOCK_OP_REQUEST:
> + case VIRTIO_VSOCK_OP_RESPONSE:
> + hdr->op = cpu_to_le16(AF_VSOCK_OP_CONNECT);
> + break;
> + case VIRTIO_VSOCK_OP_RST:
> + case VIRTIO_VSOCK_OP_SHUTDOWN:
> + hdr->op = cpu_to_le16(AF_VSOCK_OP_DISCONNECT);
> + break;
> + case VIRTIO_VSOCK_OP_RW:
> + hdr->op = cpu_to_le16(AF_VSOCK_OP_PAYLOAD);
> + break;
> + case VIRTIO_VSOCK_OP_CREDIT_UPDATE:
> + case VIRTIO_VSOCK_OP_CREDIT_REQUEST:
> + hdr->op = cpu_to_le16(AF_VSOCK_OP_CONTROL);
> + break;
> + default:
> + hdr->op = cpu_to_le16(AF_VSOCK_OP_UNKNOWN);
> + break;
> + }
> +
> + hdr->t_hdr.virtio_hdr = pkt->hdr;
> +
> + if (pkt->len) {
> + payload = skb_put(skb, pkt->len);
> + memcpy(payload, pkt->buf, pkt->len);
> + }
> +
> + return skb;
> +}
> +
> +static void vsock_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
> +{
> + struct sk_buff *skb = virtio_vsock_pkt_to_vsockmon_skb(pkt);
> + if (skb) {
> + vsock_deliver_tap(skb);
> + kfree_skb(skb);
Should this be consume_skb()? The function's doc comment says:
* Functions identically to kfree_skb, but kfree_skb assumes that the frame
* is being dropped after a failure and notes that
This isn't a failure case so kfree_skb() is not the right function.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] vsockmon: Add virtio vsock hooks
2016-08-10 11:59 ` Stefan Hajnoczi
@ 2016-08-12 14:18 ` Gerard Garcia
0 siblings, 0 replies; 13+ messages in thread
From: Gerard Garcia @ 2016-08-12 14:18 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: netdev, jhansen, mst
On 08/10/2016 01:59 PM, Stefan Hajnoczi wrote:
> On Mon, Aug 08, 2016 at 06:14:42PM +0200, ggarcia@abra.uab.cat wrote:
>> +static struct sk_buff *
>> +virtio_vsock_pkt_vsockmon_to_vsockmon_skb(struct virtio_vsock_pkt *pkt)
>> +{
>> + struct sk_buff *skb;
>> + struct af_vsockmon_hdr *hdr;
>> + void *payload;
>> +
>> + u32 skb_len = sizeof(struct af_vsockmon_hdr) + pkt->len;
>> +
>> + skb = alloc_skb(skb_len, GFP_ATOMIC);
>> + if (!skb)
>> + return NULL;
>> +
>> + hdr = (struct af_vsockmon_hdr *) skb_put(skb, sizeof(*hdr));
>> +
>> + hdr->src_cid = pkt->hdr.src_cid;
>> + hdr->src_port = pkt->hdr.src_port;
>> + hdr->dst_cid = pkt->hdr.dst_cid;
>> + hdr->dst_port = pkt->hdr.dst_port;
>> + hdr->t = cpu_to_le16(AF_VSOCK_T_VIRTIO);
>> + hdr->len = cpu_to_le16(sizeof(hdr->t_hdr));
>> +
>> + switch(pkt->hdr.op) {
>
> Missing le16_to_cpu()
>
Right
>> + case VIRTIO_VSOCK_OP_REQUEST:
>> + case VIRTIO_VSOCK_OP_RESPONSE:
>> + hdr->op = cpu_to_le16(AF_VSOCK_OP_CONNECT);
>> + break;
>> + case VIRTIO_VSOCK_OP_RST:
>> + case VIRTIO_VSOCK_OP_SHUTDOWN:
>> + hdr->op = cpu_to_le16(AF_VSOCK_OP_DISCONNECT);
>> + break;
>> + case VIRTIO_VSOCK_OP_RW:
>> + hdr->op = cpu_to_le16(AF_VSOCK_OP_PAYLOAD);
>> + break;
>> + case VIRTIO_VSOCK_OP_CREDIT_UPDATE:
>> + case VIRTIO_VSOCK_OP_CREDIT_REQUEST:
>> + hdr->op = cpu_to_le16(AF_VSOCK_OP_CONTROL);
>> + break;
>> + default:
>> + hdr->op = cpu_to_le16(AF_VSOCK_OP_UNKNOWN);
>> + break;
>> + }
>> +
>> + hdr->t_hdr.virtio_hdr = pkt->hdr;
>> +
>> + if (pkt->len) {
>> + payload = skb_put(skb, pkt->len);
>> + memcpy(payload, pkt->buf, pkt->len);
>> + }
>> +
>> + return skb;
>> +}
>> +
>> +static void vsock_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
>> +{
>> + struct sk_buff *skb = virtio_vsock_pkt_to_vsockmon_skb(pkt);
>> + if (skb) {
>> + vsock_deliver_tap(skb);
>> + kfree_skb(skb);
>
> Should this be consume_skb()? The function's doc comment says:
>
> * Functions identically to kfree_skb, but kfree_skb assumes that the frame
> * is being dropped after a failure and notes that
>
> This isn't a failure case so kfree_skb() is not the right function.
>
I agree, consume_skb is more appropriate.
^ permalink raw reply [flat|nested] 13+ messages in thread