Re: [RFC 3/3] vsockmon: Add vsock hooks

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3638 bytes --]

On Sat, May 28, 2016 at 06:29:07PM +0200, ggarcia@abra.uab.cat wrote:
> From: Gerard Garcia <ggarcia@deic.uab.cat>
> 
> Signed-off-by: Gerard Garcia <ggarcia@deic.uab.cat>
> ---
>  drivers/vhost/vsock.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 17bfe4e..8fd5125 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -14,8 +14,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,67 @@ struct vhost_vsock {
>  	u32 guest_cid;
>  };
>  
> +static struct sk_buff *
> +virtio_vsock_pkt_to_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;
> +
> +	skb_reserve(skb, sizeof(struct af_vsockmon_hdr));
> +
> +	if (pkt->len) {
> +		payload = skb_put(skb, pkt->len);
> +		memcpy(payload, pkt->buf, pkt->len);
> +	}
> +
> +	hdr = (struct af_vsockmon_hdr *) skb_push(skb, sizeof(*hdr));
> +	hdr->type = AF_VSOCK_VIRTIO;
> +
> +	switch(pkt->hdr.op) {
> +		case VIRTIO_VSOCK_OP_REQUEST:
> +		case VIRTIO_VSOCK_OP_RESPONSE:
> +			hdr->g_hdr.op = AF_VSOCK_G_OP_CONNECT;
> +			break;
> +		case VIRTIO_VSOCK_OP_RST:
> +		case VIRTIO_VSOCK_OP_SHUTDOWN:
> +			hdr->g_hdr.op = AF_VSOCK_G_OP_DISCONNECT;
> +			break;
> +		case VIRTIO_VSOCK_OP_RW:
> +			hdr->g_hdr.op = AF_VSOCK_G_OP_PAYLOAD;
> +			break;
> +		case VIRTIO_VSOCK_OP_CREDIT_UPDATE:
> +		case VIRTIO_VSOCK_OP_CREDIT_REQUEST:
> +			hdr->g_hdr.op = AF_VSOCK_G_OP_CONTROL;
> +			break;
> +		default:
> +			hdr->g_hdr.op = AF_VSOCK_G_OP_UNKNOWN;
> +			break;
> +	}
> +
> +	hdr->g_hdr.src_cid = pkt->hdr.src_cid;
> +	hdr->g_hdr.src_port = pkt->hdr.src_port;
> +	hdr->g_hdr.dst_cid = pkt->hdr.dst_cid;
> +	hdr->g_hdr.dst_port = pkt->hdr.dst_port;

Careful with endianness here.  pkt->hdr uses little-endian fields.

> +
> +	hdr->t_hdr.virtio_hdr = pkt->hdr;
> +
> +	return skb;
> +}
> +
> +static void vsock_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
> +{
> +	struct sk_buff *skb = virtio_vsock_pkt_to_skb(pkt);
> +	if (skb)
> +		vsock_deliver_tap(skb);

vsock_deliver_tap() doesn't take ownership of skb so it is leaked here!

In fact, given that the core vsock code is not skb-based, perhaps the
tap code shouldn't clone at all.  Just take ownership of the skb.  This
avoids the extra allocation and memcpy.

> +}
> +
>  static u32 vhost_transport_get_local_cid(void)
>  {
>  	return VHOST_VSOCK_DEFAULT_HOST_CID;
> @@ -147,6 +210,11 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
>  
>  		vsock->total_tx_buf -= pkt->len;
>  
> +		/* Deliver to monitoring devices all correctly transmitted
> +		 * packets.
> +		 */
> +		vsock_deliver_tap_pkt(pkt);
> +
>  		virtio_transport_free_pkt(pkt);
>  	}
>  	if (added)
> @@ -367,6 +435,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.8.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]