Re: [PATCH 3/3] vsockmon: Add virtio vsock hooks

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:

quoted

+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

quoted

+	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.