netdev - [RFC PATCH net-next v6 01/14] af_vsock: generalize vsock_dgram

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: <d1126dd8-cc6c-be20-7b55-83a0517d14d0@salutedevices.com>
Date: Mon, 29 Jul 2024 22:25:55 +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 01/14] af_vsock: generalize
 vsock_dgram_recvmsg() to all transports

> @@ -1273,11 +1273,15 @@ static int vsock_dgram_connect(struct socket *sock,
>  int vsock_dgram_recvmsg(struct socket *sock, struct msghdr *msg,
>  			size_t len, int flags)
>  {
> +	struct vsock_skb_cb *vsock_cb;
>  #ifdef CONFIG_BPF_SYSCALL
>  	const struct proto *prot;
>  #endif
>  	struct vsock_sock *vsk;
> +	struct sk_buff *skb;
> +	size_t payload_len;
>  	struct sock *sk;
> +	int err;
>  
>  	sk = sock->sk;
>  	vsk = vsock_sk(sk);
> @@ -1288,7 +1292,43 @@ int vsock_dgram_recvmsg(struct socket *sock, struct msghdr *msg,
>  		return prot->recvmsg(sk, msg, len, flags, NULL);
>  #endif
>  
> -	return vsk->transport->dgram_dequeue(vsk, msg, len, flags);
> +	if (flags & MSG_OOB || flags & MSG_ERRQUEUE)
> +		return -EOPNOTSUPP;
> +
> +	if (unlikely(flags & MSG_ERRQUEUE))
> +		return sock_recv_errqueue(sk, msg, len, SOL_VSOCK, 0);
> +
> +	/* Retrieve the head sk_buff from the socket's receive queue. */
> +	err = 0;
> +	skb = skb_recv_datagram(sk_vsock(vsk), flags, &err);
> +	if (!skb)
> +		return err;
> +
> +	payload_len = skb->len;
> +
> +	if (payload_len > len) {
> +		payload_len = len;
> +		msg->msg_flags |= MSG_TRUNC;
> +	}
> +
> +	/* Place the datagram payload in the user's iovec. */
> +	err = skb_copy_datagram_msg(skb, 0, msg, payload_len);
> +	if (err)
> +		goto out;
> +
> +	if (msg->msg_name) {
> +		/* Provide the address of the sender. */
> +		DECLARE_SOCKADDR(struct sockaddr_vm *, vm_addr, msg->msg_name);
> +
> +		vsock_cb = vsock_skb_cb(skb);

May be we can declare 'vsock_cb' here ? Reducing its scope.

> +		vsock_addr_init(vm_addr, vsock_cb->src_cid, vsock_cb->src_port);
> +		msg->msg_namelen = sizeof(*vm_addr);
> +	}
> +	err = payload_len;
> +
> +out:
> +	skb_free_datagram(&vsk->sk, skb);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(vsock_dgram_recvmsg);
>  


> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -610,6 +610,7 @@ vmci_transport_datagram_create_hnd(u32 resource_id,
>  
>  static int vmci_transport_recv_dgram_cb(void *data, struct vmci_datagram *dg)
>  {
> +	struct vsock_skb_cb *vsock_cb;
>  	struct sock *sk;
>  	size_t size;
>  	struct sk_buff *skb;
> @@ -637,10 +638,14 @@ static int vmci_transport_recv_dgram_cb(void *data, struct vmci_datagram *dg)
>  	if (!skb)
>  		return VMCI_ERROR_NO_MEM;
>  
> +	vsock_cb = vsock_skb_cb(skb);
> +	vsock_cb->src_cid = dg->src.context;
> +	vsock_cb->src_port = dg->src.resource;
>  	/* sk_receive_skb() will do a sock_put(), so hold here. */
>  	sock_hold(sk);
>  	skb_put(skb, size);
>  	memcpy(skb->data, dg, size);
> +	skb_pull(skb, VMCI_DG_HEADERSIZE);

Small suggestion: here we do:

1) skb_put(skb, size of entire datagram)
2) memcpy(entire datagram)
3) skb_pull(VMCI_DG_HEADERSIZE)

If we provide only data to the upper layer, we can do:
1) skb_put(dg->payload_size)
2) memcpy(dg->payload_size)

Also (I'm no expert in VMCI), i guess using dg->payload_size is safer
to know number of data bytes, instead of using VMCI_DG_HEADERSIZE.

WDYT?

>  	sk_receive_skb(sk, skb, 0);
>  
>  	return VMCI_SUCCESS;
> @@ -1731,59 +1736,6 @@ static int vmci_transport_dgram_enqueue(
>  	return err - sizeof(*dg);
>  }
>  

Thanks