netdev - Re: [PATCH v4 3/3] VSOCK: Add virtio vsock vsockmon hooks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5E1B910C-0113-494A-A46D-41F9686BD525@vmware.com>
Date:   Thu, 20 Apr 2017 12:35:40 +0000
From:   "Jorgen S. Hansen" <jhansen@...are.com>
To:     Stefan Hajnoczi <stefanha@...hat.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Zhu Yanjun <zyjzyj2000@...il.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Gerard Garcia <ggarcia@...a.uab.cat>
Subject: Re: [PATCH v4 3/3] VSOCK: Add virtio vsock vsockmon hooks

> 
> +/* Packet capture */
> +void virtio_transport_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
> +{
> +	struct sk_buff *skb;
> +	struct af_vsockmon_hdr *hdr;
> +	unsigned char *t_hdr, *payload;
> +
> +	skb = alloc_skb(sizeof(*hdr) + sizeof(pkt->hdr) + pkt->len,
> +			GFP_ATOMIC);

So with the current API, an skb will always be allocated, even if there are no listeners? And we’ll copy the payload into the skb as well, if there is any. Would it make sense to introduce a check here (exposed as part of the vsock tap API), that skips all that in the case of no listeners? In the common case, there won’t be any listeners, so it would save some cycles.

> +	if (!skb)
> +		return; /* nevermind if we cannot capture the packet */
> +
> +	hdr = (struct af_vsockmon_hdr *)skb_put(skb, sizeof(*hdr));
> +
> +	/* pkt->hdr is little-endian so no need to byteswap here */
> +	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->transport = cpu_to_le16(AF_VSOCK_TRANSPORT_VIRTIO);
> +	hdr->len = cpu_to_le16(sizeof(pkt->hdr));
> +	hdr->reserved[0] = hdr->reserved[1] = 0;
> +
> +	switch(cpu_to_le16(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;
> +	}
> +
> +	t_hdr = skb_put(skb, sizeof(pkt->hdr));
> +	memcpy(t_hdr, &pkt->hdr, sizeof(pkt->hdr));
> +
> +	if (pkt->len) {
> +		payload = skb_put(skb, pkt->len);
> +		memcpy(payload, pkt->buf, pkt->len);
> +	}
> +
> +	vsock_deliver_tap(skb);
> +}
> +EXPORT_SYMBOL_GPL(virtio_transport_deliver_tap_pkt);
> +
> static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
> 					  struct virtio_vsock_pkt_info *info)
> {
> -- 
> 2.9.3
>