netdev - Re: [PATCH RFC net-next v3 7/8] vsock: Add lockless sendmsg() support

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]

Date: Wed, 31 May 2023 18:22:19 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Bobby Eshleman <bobby.eshleman@...edance.com>
Cc: Stefan Hajnoczi <stefanha@...hat.com>,
	Stefano Garzarella <sgarzare@...hat.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Jason Wang <jasowang@...hat.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	"K. Y. Srinivasan" <kys@...rosoft.com>,
	Haiyang Zhang <haiyangz@...rosoft.com>,
	Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
	Bryan Tan <bryantan@...are.com>, Vishnu Dasa <vdasa@...are.com>,
	VMware PV-Drivers Reviewers <pv-drivers@...are.com>,
	kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hyperv@...r.kernel.org
Subject: Re: [PATCH RFC net-next v3 7/8] vsock: Add lockless sendmsg() support

On Wed, May 31, 2023 at 12:35:11AM +0000, Bobby Eshleman wrote:

...

Hi Bobby,

some more feedback from my side.

> Throughput metrics for single-threaded SOCK_DGRAM and
> single/multi-threaded SOCK_STREAM showed no statistically signficant

nit: s/signficant/significant/

> throughput changes (lowest p-value reaching 0.27), with the range of the
> mean difference ranging between -5% to +1%.
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@...edance.com>

...

> @@ -120,8 +125,8 @@ struct vsock_transport {
>  
>  	/* DGRAM. */
>  	int (*dgram_bind)(struct vsock_sock *, struct sockaddr_vm *);
> -	int (*dgram_enqueue)(struct vsock_sock *, struct sockaddr_vm *,
> -			     struct msghdr *, size_t len);
> +	int (*dgram_enqueue)(const struct vsock_transport *, struct vsock_sock *,
> +			     struct sockaddr_vm *, struct msghdr *, size_t len);

Perhaps just a personal preference, but the arguments for these callbacks
could have names.

>  	bool (*dgram_allow)(u32 cid, u32 port);
>  	int (*dgram_get_cid)(struct sk_buff *skb, unsigned int *cid);
>  	int (*dgram_get_port)(struct sk_buff *skb, unsigned int *port);
> @@ -196,6 +201,17 @@ void vsock_core_unregister(const struct vsock_transport *t);
>  /* The transport may downcast this to access transport-specific functions */
>  const struct vsock_transport *vsock_core_get_transport(struct vsock_sock *vsk);
>  
> +static inline struct vsock_remote_info *
> +vsock_core_get_remote_info(struct vsock_sock *vsk)
> +{
> +

nit: no blank line here

> +	/* vsk->remote_info may be accessed if the rcu read lock is held OR the
> +	 * socket lock is held
> +	 */
> +	return rcu_dereference_check(vsk->remote_info,
> +				     lockdep_sock_is_held(sk_vsock(vsk)));
> +}
> +
>  /**** UTILS ****/
>  
>  /* vsock_table_lock must be held */

...

> @@ -300,17 +449,36 @@ static void vsock_insert_unbound(struct vsock_sock *vsk)
>  	spin_unlock_bh(&vsock_table_lock);
>  }
>  
> -void vsock_insert_connected(struct vsock_sock *vsk)
> +int vsock_insert_connected(struct vsock_sock *vsk)
>  {
> -	struct list_head *list = vsock_connected_sockets(
> -		&vsk->remote_addr, &vsk->local_addr);
> +	struct list_head *list;
> +	struct vsock_remote_info *remote_info;

nit: I know that this file doesn't follow the reverse xmas tree
     scheme - longest line to shortest - for local variable declarations.
     But as networking code I think it would be good towards towards
     that scheme as code is changed.

	struct vsock_remote_info *remote_info;
	struct list_head *list;

> +
> +	rcu_read_lock();
> +	remote_info = vsock_core_get_remote_info(vsk);
> +	if (!remote_info) {
> +		rcu_read_unlock();
> +		return -EINVAL;
> +	}
> +	list = vsock_connected_sockets(&remote_info->addr, &vsk->local_addr);
> +	rcu_read_unlock();
>  
>  	spin_lock_bh(&vsock_table_lock);
>  	__vsock_insert_connected(list, vsk);
>  	spin_unlock_bh(&vsock_table_lock);
> +
> +	return 0;
>  }

...

> @@ -1120,7 +1122,9 @@ virtio_transport_recv_connecting(struct sock *sk,
>  	case VIRTIO_VSOCK_OP_RESPONSE:
>  		sk->sk_state = TCP_ESTABLISHED;
>  		sk->sk_socket->state = SS_CONNECTED;
> -		vsock_insert_connected(vsk);
> +		err = vsock_insert_connected(vsk);
> +		if (err)
> +			goto destroy;

The destroy label uses skerr, but it is uninitialised here.

A W=1 or C=1 will probably tell you this.

>  		sk->sk_state_change(sk);
>  		break;
>  	case VIRTIO_VSOCK_OP_INVALID:

...