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

[Gerard Garcia <ggarcia@abra.uab.cat>]

On 06/01/2016 11:19 PM, Stefan Hajnoczi wrote:
> 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.
True, I have not considered this.
>> +
>> +	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.
>
You are right! and yes, I agree that would be better to avoid the clone. 
As you say I can just take ownership of the skb before dev_queue_xmit() 
in vsock_deliver_tap() so it won't be free'd after the call and avoid 
the clone.
>> +}
>> +
>>   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
>>