netdev - [RFC PATCH net-next v6 07/14] virtio/vsock: add common datagram send path

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: <e1647f5f-5056-5cf0-e81c-5ef71fd6efd0@salutedevices.com>
Date: Mon, 29 Jul 2024 23:00:36 +0300
From: Arseniy Krasnov <avkrasnov@...utedevices.com>
To: <stefanha@...hat.com>, <sgarzare@...hat.com>, <mst@...hat.com>,
	<jasowang@...hat.com>, <xuanzhuo@...ux.alibaba.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<kys@...rosoft.com>, <haiyangz@...rosoft.com>, <wei.liu@...nel.org>,
	<decui@...rosoft.com>, <bryantan@...are.com>, <vdasa@...are.com>,
	<pv-drivers@...are.com>
CC: <dan.carpenter@...aro.org>, <simon.horman@...igine.com>,
	<oxffffaa@...il.com>, <kvm@...r.kernel.org>,
	<virtualization@...ts.linux-foundation.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-hyperv@...r.kernel.org>,
	<bpf@...r.kernel.org>, <bobby.eshleman@...edance.com>,
	<jiang.wang@...edance.com>, <amery.hung@...edance.com>,
	<ameryhung@...il.com>, <xiyou.wangcong@...il.com>, <kernel@...rdevices.ru>
Subject: [RFC PATCH net-next v6 07/14] virtio/vsock: add common datagram send
 path

Hi,

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index a1c76836d798..46cd1807f8e3 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -1040,13 +1040,98 @@ int virtio_transport_shutdown(struct vsock_sock *vsk, int mode)
>  }
>  EXPORT_SYMBOL_GPL(virtio_transport_shutdown);
>  
> +static int virtio_transport_dgram_send_pkt_info(struct vsock_sock *vsk,
> +						struct virtio_vsock_pkt_info *info)
> +{
> +	u32 src_cid, src_port, dst_cid, dst_port;
> +	const struct vsock_transport *transport;
> +	const struct virtio_transport *t_ops;
> +	struct sock *sk = sk_vsock(vsk);
> +	struct virtio_vsock_hdr *hdr;
> +	struct sk_buff *skb;
> +	void *payload;
> +	int noblock = 0;
> +	int err;
> +
> +	info->type = virtio_transport_get_type(sk_vsock(vsk));
> +
> +	if (info->pkt_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
> +		return -EMSGSIZE;

Small suggestion, i think we can check for packet length earlier ? Before
info->type = ...

> +
> +	transport = vsock_dgram_lookup_transport(info->remote_cid, info->remote_flags);
> +	t_ops = container_of(transport, struct virtio_transport, transport);
> +	if (unlikely(!t_ops))
> +		return -EFAULT;
> +
> +	if (info->msg)
> +		noblock = info->msg->msg_flags & MSG_DONTWAIT;
> +
> +	/* Use sock_alloc_send_skb to throttle by sk_sndbuf. This helps avoid
> +	 * triggering the OOM.
> +	 */
> +	skb = sock_alloc_send_skb(sk, info->pkt_len + VIRTIO_VSOCK_SKB_HEADROOM,
> +				  noblock, &err);
> +	if (!skb)
> +		return err;
> +
> +	skb_reserve(skb, VIRTIO_VSOCK_SKB_HEADROOM);
> +
> +	src_cid = t_ops->transport.get_local_cid();
> +	src_port = vsk->local_addr.svm_port;
> +	dst_cid = info->remote_cid;
> +	dst_port = info->remote_port;
> +
> +	hdr = virtio_vsock_hdr(skb);
> +	hdr->type	= cpu_to_le16(info->type);
> +	hdr->op		= cpu_to_le16(info->op);
> +	hdr->src_cid	= cpu_to_le64(src_cid);
> +	hdr->dst_cid	= cpu_to_le64(dst_cid);
> +	hdr->src_port	= cpu_to_le32(src_port);
> +	hdr->dst_port	= cpu_to_le32(dst_port);
> +	hdr->flags	= cpu_to_le32(info->flags);
> +	hdr->len	= cpu_to_le32(info->pkt_len);

There is function 'virtio_transport_init_hdr()' in this file, may be reuse it ?

> +
> +	if (info->msg && info->pkt_len > 0) {

If pkt_len is 0, do we really need to send such packets ? Because for connectible
sockets, we ignore empty OP_RW packets.

> +		payload = skb_put(skb, info->pkt_len);
> +		err = memcpy_from_msg(payload, info->msg, info->pkt_len);
> +		if (err)
> +			goto out;
> +	}
> +
> +	trace_virtio_transport_alloc_pkt(src_cid, src_port,
> +					 dst_cid, dst_port,
> +					 info->pkt_len,
> +					 info->type,
> +					 info->op,
> +					 info->flags,
> +					 false);

^^^ For SOCK_DGRAM, include/trace/events/vsock_virtio_transport_common.h also should
be updated?

> +
> +	return t_ops->send_pkt(skb);
> +out:
> +	kfree_skb(skb);
> +	return err;
> +}
> +
>  int
>  virtio_transport_dgram_enqueue(struct vsock_sock *vsk,
>  			       struct sockaddr_vm *remote_addr,
>  			       struct msghdr *msg,
>  			       size_t dgram_len)
>  {
> -	return -EOPNOTSUPP;
> +	/* Here we are only using the info struct to retain style uniformity
> +	 * and to ease future refactoring and merging.
> +	 */
> +	struct virtio_vsock_pkt_info info = {
> +		.op = VIRTIO_VSOCK_OP_RW,
> +		.remote_cid = remote_addr->svm_cid,
> +		.remote_port = remote_addr->svm_port,
> +		.remote_flags = remote_addr->svm_flags,
> +		.msg = msg,
> +		.vsk = vsk,
> +		.pkt_len = dgram_len,
> +	};
> +
> +	return virtio_transport_dgram_send_pkt_info(vsk, &info);
>  }
>  EXPORT_SYMBOL_GPL(virtio_transport_dgram_enqueue);
>  
> -- 
> 2.20.1

Thanks, Arseniy