From: Gerard Garcia <redacted>
This patch applies over the mst vhost git repository:
http://git.kernel.org/cgit/linux/kernel/git/mst/vhost.git
This was already been sent as a RFC where several issues where fixed.
This is the summary of changes from the first RFC:
v2:
* Do not clone skb, instead take ownership before transmitting.
* Split tap functions from af_vsock.c.
* Simplify vsockmon header to remove unnecessary padding and
set little endian byte order.
* Various simple fixes from the comments received to the first RFC.
Additionally, this version changes:
* Add len field to the vsockmon header to ease parsing.
* Pack vsockmon header.
* Various simple fixes and styling.
Overview:
Virtual socket transports operate at kernel level therefore, there is no easy
way to see the traffic exchanged between virtual machines and hypervisors that
communicate using AF_VSOCK sockets. In addition, being able to see the control
messages exchanged by the transports may be useful for debugging and
optimization purposes. This patch adds a virtual device that may be used to see
the traffic exchanged between virtual machines and hypervisors through AF_VSOCK
sockets.
Its structure is based on the nlmon device and this version just targets the
virtio transport, but support for the VMCI transport can be easily implemented.
The vsockmon header contains a generic header and includes the header specific to
the transport. The generic header allows to follow an AF_VSOCK stream without
having to dig into the details of the transport while the transport header
gives more detail which may be useful for troubleshooting and debugging.
Testing:
To set up a vsockmon device:
ip link add type vsockmon
ip link set vsockmon0 up
The Wireshark development version (master branch) includes a vsock dissector
that is capable of parsing packets received through vsockmon. The dissector
needs to be manually selected.
Thanks to Stefan Hajnoczi for his help.
Gerard
Gerard Garcia (3):
vsockmon: Add tap functions.
vsockmon: Add vsockmon device.
vsockmon: Add virtio vsock hooks
drivers/net/Kconfig | 8 ++
drivers/net/Makefile | 1 +
drivers/net/vsockmon.c | 168 ++++++++++++++++++++++++++++++++++++++++++
drivers/vhost/vsock.c | 72 ++++++++++++++++++
include/net/af_vsock.h | 13 ++++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/if_arp.h | 1 +
include/uapi/linux/vsockmon.h | 35 +++++++++
net/vmw_vsock/Makefile | 2 +-
net/vmw_vsock/af_vsock_tap.c | 114 ++++++++++++++++++++++++++++
10 files changed, 414 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/vsockmon.c
create mode 100644 include/uapi/linux/vsockmon.h
create mode 100644 net/vmw_vsock/af_vsock_tap.c
--
2.9.1
@@ -0,0 +1,114 @@+/*+*TapfunctionsforAF_VSOCKsockets.+*+*Codebasedonnet/netlink/af_netlink.ctapfunctions.+*+*Thisprogramisfreesoftware;youcanredistributeitand/or+*modifyitunderthetermsoftheGNUGeneralPublicLicense+*aspublishedbytheFreeSoftwareFoundation;eitherversion+*2oftheLicense,or(atyouroption)anylaterversion.+*/++#include<net/sock.h>+#include<net/af_vsock.h>+#include<linux/if_arp.h>++staticDEFINE_SPINLOCK(vsock_tap_lock);+staticstructlist_headvsock_tap_all__read_mostly=+LIST_HEAD_INIT(vsock_tap_all);++intvsock_add_tap(structvsock_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);+++return0;+}+EXPORT_SYMBOL_GPL(vsock_add_tap);++int__vsock_remove_tap(structvsock_tap*vt){+boolfound=false;+structvsock_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;+gotoout;+}+}++pr_warn("__vsock_remove_tap: %p not found\n",vt);+out:+spin_unlock(&vsock_tap_lock);++if(found)+module_put(vt->module);++returnfound?0:-ENODEV;+}++intvsock_remove_tap(structvsock_tap*vt)+{+intret;++ret=__vsock_remove_tap(vt);+synchronize_net();++returnret;+}+EXPORT_SYMBOL_GPL(vsock_remove_tap);++staticint__vsock_deliver_tap_skb(structsk_buff*skb,+structnet_device*dev)+{+intret=0;++if(skb){+dev_hold(dev);+/* Take skb ownership so it is not consumed in dev_queue_xmit.+*dev_queue_xmitwilldropareferencesothereferencecount+*willreset.+*/+skb_get(skb);+skb->dev=dev;+ret=dev_queue_xmit(skb);+if(unlikely(ret>0))+ret=net_xmit_errno(ret);++dev_put(dev);+}++returnret;+}++staticvoid__vsock_deliver_tap(structsk_buff*skb)+{+intret;+structvsock_tap*tmp;++list_for_each_entry_rcu(tmp,&vsock_tap_all,list){+ret=__vsock_deliver_tap_skb(skb,tmp->dev);+if(unlikely(ret))+break;+}+}++voidvsock_deliver_tap(structsk_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);
@@ -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. */++structaf_vsockmon_hdr{+__le64src_cid;+__le64dst_cid;+__le32src_port;+__le32dst_port;+__le16op;/* enum af_vsockmon_op */+__le16t;/* enum af_vosckmon_t */+__le16len;/* sizeof(t_hdr) */+union{+structvirtio_vsock_hdrvirtio_hdr;+}t_hdr;+}__attribute__((packed));++enumaf_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,+};++enumaf_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
From: Gerard Garcia <redacted>
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 <redacted>
---
drivers/vhost/vsock.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
From: Stefan Hajnoczi <stefanha@redhat.com> Date: 2016-08-10 18:46:46
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.
@@ -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. */++structaf_vsockmon_hdr{+__le64src_cid;+__le64dst_cid;+__le32src_port;+__le32dst_port;+__le16op;/* enum af_vsockmon_op */+__le16t;/* enum af_vosckmon_t */+__le16len;/* sizeof(t_hdr) */+union{+structvirtio_vsock_hdrvirtio_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.
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.
On Mon, Aug 08, 2016 at 06:14:40PM +0200, ggarcia@abra.uab.cat wrote:
quoted
+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.
@@ -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. */++structaf_vsockmon_hdr{+__le64src_cid;+__le64dst_cid;+__le32src_port;+__le32dst_port;+__le16op;/* enum af_vsockmon_op */+__le16t;/* enum af_vosckmon_t */+__le16len;/* sizeof(t_hdr) */+union{+structvirtio_vsock_hdrvirtio_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.
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.
@@ -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. */++structaf_vsockmon_hdr{+__le64src_cid;+__le64dst_cid;+__le32src_port;+__le32dst_port;+__le16op;/* enum af_vsockmon_op */+__le16t;/* enum af_vosckmon_t */+__le16len;/* sizeof(t_hdr) */+union{+structvirtio_vsock_hdrvirtio_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
@@ -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. */++structaf_vsockmon_hdr{+__le64src_cid;+__le64dst_cid;+__le32src_port;+__le32dst_port;+__le16op;/* enum af_vsockmon_op */+__le16t;/* enum af_vosckmon_t */+__le16len;/* sizeof(t_hdr) */+union{+structvirtio_vsock_hdrvirtio_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.
@@ -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. */++structaf_vsockmon_hdr{+__le64src_cid;+__le64dst_cid;+__le32src_port;+__le32dst_port;+__le16op;/* enum af_vsockmon_op */+__le16t;/* enum af_vosckmon_t */+__le16len;/* sizeof(t_hdr) */+union{+structvirtio_vsock_hdrvirtio_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