NFS: add AF_VSOCK support

The tcp_read_sock() interface dequeues skbs and gives them to the caller's callback function for processing. This interface can avoid data copies since the caller accesses the skb instead of using its own receive buffer.

This patch implements vsock_read_sock() for AF_VSOCK SOCK_STREAM sockets. The implementation is only for virtio-vsock at this time, not for the VMware VMCI transport. It is not zero-copy yet because the virtio-vsock receive queue does not consist of skbs.

The tcp_read_sock()-like interface is needed for AF_VSOCK sunrpc support.

Signed-off-by Stefan Hajnoczi <[email protected]>

---
 include/linux/virtio_vsock.h            |  4 ++
 include/net/af_vsock.h                  |  5 +++
 drivers/vhost/vsock.c                   |  1 +
 net/vmw_vsock/af_vsock.c                | 16 ++++++++
 net/vmw_vsock/virtio_transport.c        |  1 +
 net/vmw_vsock/virtio_transport_common.c | 66 +++++++++++++++++++++++++++++++++
 net/vmw_vsock/vmci_transport.c          |  8 ++++
 7 files changed, 101 insertions(+)

include/linux/virtio_vsock.h +4 -0

index ab13f07..d6cf428 100644
@@ -5,6 +5,7 @@
 #include <linux/socket.h>
 #include <net/sock.h>
 #include <net/af_vsock.h>
+#include <net/tcp.h> /* for sk_read_actor_t */
 
 #define VIRTIO_VSOCK_DEFAULT_MIN_BUF_SIZE	128
 #define VIRTIO_VSOCK_DEFAULT_BUF_SIZE		(1024 * 256)

@@ -126,6 +127,9 @@ int virtio_transport_notify_send_post_enqueue(struct vsock_sock *vsk,
 u64 virtio_transport_stream_rcvhiwat(struct vsock_sock *vsk);
 bool virtio_transport_stream_is_active(struct vsock_sock *vsk);
 bool virtio_transport_stream_allow(u32 cid, u32 port);
+int virtio_transport_stream_read_sock(struct vsock_sock *vsk,
+				      read_descriptor_t *desc,
+				      sk_read_actor_t recv_actor);
 int virtio_transport_dgram_bind(struct vsock_sock *vsk,
 				struct sockaddr_vm *addr);
 bool virtio_transport_dgram_allow(u32 cid, u32 port);

include/net/af_vsock.h +5 -0

index f9fb566..85fa345 100644
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 #include <linux/workqueue.h>
 #include <linux/vm_sockets.h>
+#include <net/tcp.h> /* for sk_read_actor_t */
 
 #include "vsock_addr.h"
 

@@ -73,6 +74,8 @@ struct vsock_sock {
 	void *trans;
 };
 
+int vsock_read_sock(struct sock *sk, read_descriptor_t *desc,
+		    sk_read_actor_t recv_actor);
 s64 vsock_stream_has_data(struct vsock_sock *vsk);
 s64 vsock_stream_has_space(struct vsock_sock *vsk);
 void vsock_pending_work(struct work_struct *work);

@@ -125,6 +128,8 @@ struct vsock_transport {
 	u64 (*stream_rcvhiwat)(struct vsock_sock *);
 	bool (*stream_is_active)(struct vsock_sock *);
 	bool (*stream_allow)(u32 cid, u32 port);
+	int (*stream_read_sock)(struct vsock_sock *, read_descriptor_t *desc,
+				sk_read_actor_t recv_actor);
 
 	/* Notification. */
 	int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);

drivers/vhost/vsock.c +1 -0

index 3acef3c..5128e41 100644
@@ -734,6 +734,7 @@ static struct virtio_transport vhost_transport = {
 		.stream_rcvhiwat          = virtio_transport_stream_rcvhiwat,
 		.stream_is_active         = virtio_transport_stream_is_active,
 		.stream_allow             = virtio_transport_stream_allow,
+		.stream_read_sock         = virtio_transport_stream_read_sock,
 
 		.notify_poll_in           = virtio_transport_notify_poll_in,
 		.notify_poll_out          = virtio_transport_notify_poll_out,

net/vmw_vsock/af_vsock.c +16 -0

index dfc8c51e..2ae94b5a 100644
@@ -706,6 +706,22 @@ static void vsock_sk_destruct(struct sock *sk)
 	put_cred(vsk->owner);
 }
 
+/* See documentation for tcp_read_sock() */
+int vsock_read_sock(struct sock *sk, read_descriptor_t *desc,
+		    sk_read_actor_t recv_actor)
+{
+	struct vsock_sock *vsp = vsock_sk(sk);
+
+	if (sk->sk_type != SOCK_STREAM)
+		return -EOPNOTSUPP;
+
+	if (sk->sk_state != SS_CONNECTED && sk->sk_state != SS_DISCONNECTING)
+		return -ENOTCONN;
+
+	return transport->stream_read_sock(vsp, desc, recv_actor);
+}
+EXPORT_SYMBOL(vsock_read_sock);
+
 static int vsock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
 	int err;

net/vmw_vsock/virtio_transport.c +1 -0

index 403d86e80..8264307 100644
@@ -520,6 +520,7 @@ static struct virtio_transport virtio_transport = {
 		.stream_rcvhiwat          = virtio_transport_stream_rcvhiwat,
 		.stream_is_active         = virtio_transport_stream_is_active,
 		.stream_allow             = virtio_transport_stream_allow,
+		.stream_read_sock         = virtio_transport_stream_read_sock,
 
 		.notify_poll_in           = virtio_transport_notify_poll_in,
 		.notify_poll_out          = virtio_transport_notify_poll_out,

net/vmw_vsock/virtio_transport_common.c +66 -0

index 18e2479..510d9e1 100644
@@ -316,6 +316,72 @@ virtio_transport_stream_dequeue(struct vsock_sock *vsk,
 EXPORT_SYMBOL_GPL(virtio_transport_stream_dequeue);
 
 int
+virtio_transport_stream_read_sock(struct vsock_sock *vsk,
+				  read_descriptor_t *desc,
+				  sk_read_actor_t recv_actor)
+{
+	struct virtio_vsock_sock *vvs;
+	int ret = 0;
+
+	vvs = vsk->trans;
+
+	spin_lock_bh(&vvs->rx_lock);
+	while (!list_empty(&vvs->rx_queue)) {
+		struct virtio_vsock_pkt *pkt;
+		struct sk_buff *skb;
+		size_t len;
+		int used;
+
+		pkt = list_first_entry(&vvs->rx_queue,
+				       struct virtio_vsock_pkt, list);
+
+		/* sk_lock is held by caller so no one else can dequeue.
+		 * Unlock rx_lock so recv_actor() can sleep.
+		 */
+		spin_unlock_bh(&vvs->rx_lock);
+
+		len = pkt->len - pkt->off;
+		skb = alloc_skb(len, GFP_KERNEL);
+		if (!skb) {
+			ret = -ENOMEM;
+			goto out_nolock;
+		}
+
+		memcpy(skb_put(skb, len),
+		       pkt->buf + pkt->off,
+		       len);
+
+		used = recv_actor(desc, skb, 0, len);
+
+		kfree_skb(skb);
+
+		spin_lock_bh(&vvs->rx_lock);
+
+		if (used > 0) {
+			ret += used;
+			pkt->off += used;
+			if (pkt->off == pkt->len) {
+				virtio_transport_dec_rx_pkt(vvs, pkt);
+				list_del(&pkt->list);
+				virtio_transport_free_pkt(pkt);
+			}
+		}
+
+		if (used <= 0 || !desc->count)
+			break;
+	}
+	spin_unlock_bh(&vvs->rx_lock);
+
+out_nolock:
+	if (ret > 0)
+		virtio_transport_send_credit_update(vsk,
+				VIRTIO_VSOCK_TYPE_STREAM, NULL);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(virtio_transport_stream_read_sock);
+
+int
 virtio_transport_dgram_dequeue(struct vsock_sock *vsk,
 			       struct msghdr *msg,
 			       size_t len, int flags)

net/vmw_vsock/vmci_transport.c +7 -0

index 10ae782..dfbdb4c 100644
@@ -646,6 +646,13 @@ static bool vmci_transport_stream_allow(u32 cid, u32 port)
 	return true;
 }
 
+static int vmci_transport_stream_read_sock(struct vsock_sock *vsk,
+					   read_descriptor_t *desc,
+					   sk_read_actor_t recv_actor)
+{
+	return -EOPNOTSUPP; /* not yet implemented */
+}
+
 /* This is invoked as part of a tasklet that's scheduled when the VMCI
  * interrupt fires.  This is run in bottom-half context but it defers most of
  * its work to the packet handling work queue.

net/vmw_vsock/vmci_transport.c static const struct vsock_transport vmci_transport = { · line 2074

Quoted 8 lines · Stefan Hajnoczi

net/vmw_vsock/vmci_transport.c +1 -0

@@ -2061,6 +2068,7 @@ static const struct vsock_transport vmci_transport = {
 	.stream_rcvhiwat = vmci_transport_stream_rcvhiwat,
 	.stream_is_active = vmci_transport_stream_is_active,
 	.stream_allow = vmci_transport_stream_allow,
+	.stream_read_sock = vmci_transport_stream_read_sock,
 	.notify_poll_in = vmci_transport_notify_poll_in,
 	.notify_poll_out = vmci_transport_notify_poll_out,
 	.notify_recv_init = vmci_transport_notify_recv_init,

Do we need this for the hyperv driver as well?

Signed-off-by Stefan Hajnoczi <[email protected]>

---
 include/linux/sunrpc/xprt.h |   1 +
 net/sunrpc/xprtsock.c       | 385 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 381 insertions(+), 5 deletions(-)

include/linux/sunrpc/xprt.h +1 -0

index eab1c74..c038d8a 100644
@@ -170,6 +170,7 @@ enum xprt_transports {
 	XPRT_TRANSPORT_RDMA	= 256,
 	XPRT_TRANSPORT_BC_RDMA	= XPRT_TRANSPORT_RDMA | XPRT_TRANSPORT_BC,
 	XPRT_TRANSPORT_LOCAL	= 257,
+	XPRT_TRANSPORT_VSOCK	= 258,
 };
 
 struct rpc_xprt {

net/sunrpc/xprtsock.c +118 -5

index fd0c8b1..cc343b91 100644
@@ -46,6 +46,7 @@
 #include <net/checksum.h>
 #include <net/udp.h>
 #include <net/tcp.h>
+#include <net/af_vsock.h>
 
 #include <trace/events/sunrpc.h>
 

@@ -271,6 +272,13 @@ static void xs_format_common_peer_addresses(struct rpc_xprt *xprt)
 		sin6 = xs_addr_in6(xprt);
 		snprintf(buf, sizeof(buf), "%pi6", &sin6->sin6_addr);
 		break;
+	case AF_VSOCK:
+		(void)rpc_ntop(sap, buf, sizeof(buf));
+		xprt->address_strings[RPC_DISPLAY_ADDR] =
+						kstrdup(buf, GFP_KERNEL);
+		snprintf(buf, sizeof(buf), "%08x",
+			 ((struct sockaddr_vm *)sap)->svm_cid);
+		break;
 	default:
 		BUG();
 	}

@@ -1881,21 +1889,30 @@ static int xs_bind(struct sock_xprt *transport, struct socket *sock)
 			nloop++;
 	} while (err == -EADDRINUSE && nloop != 2);
 
-	if (myaddr.ss_family == AF_INET)
+	switch (myaddr.ss_family) {
+	case AF_INET:
 		dprintk("RPC:       %s %pI4:%u: %s (%d)\n", __func__,
 				&((struct sockaddr_in *)&myaddr)->sin_addr,
 				port, err ? "failed" : "ok", err);
-	else
+		break;
+	case AF_INET6:
 		dprintk("RPC:       %s %pI6:%u: %s (%d)\n", __func__,
 				&((struct sockaddr_in6 *)&myaddr)->sin6_addr,
 				port, err ? "failed" : "ok", err);
+		break;
+	case AF_VSOCK:
+		dprintk("RPC:       %s %u:%u: %s (%d)\n", __func__,
+				((struct sockaddr_vm *)&myaddr)->svm_cid,
+				port, err ? "failed" : "ok", err);
+		break;
+	}
 	return err;
 }
 
 /*
- * We don't support autobind on AF_LOCAL sockets
+ * We don't support autobind on AF_LOCAL and AF_VSOCK sockets
  */
-static void xs_local_rpcbind(struct rpc_task *task)
+static void xs_dummy_rpcbind(struct rpc_task *task)
 {
 	xprt_set_bound(task->tk_xprt);
 }

@@ -1932,6 +1949,14 @@ static inline void xs_reclassify_socket6(struct socket *sock)
 		&xs_slock_key[1], "sk_lock-AF_INET6-RPC", &xs_key[1]);
 }
 
+static inline void xs_reclassify_socket_vsock(struct socket *sock)
+{
+	struct sock *sk = sock->sk;
+
+	sock_lock_init_class_and_name(sk, "slock-AF_VSOCK-RPC",
+		&xs_slock_key[1], "sk_lock-AF_VSOCK-RPC", &xs_key[1]);
+}
+
 static inline void xs_reclassify_socket(int family, struct socket *sock)
 {
 	if (WARN_ON_ONCE(!sock_allow_reclassification(sock->sk)))

@@ -1947,6 +1972,9 @@ static inline void xs_reclassify_socket(int family, struct socket *sock)
 	case AF_INET6:
 		xs_reclassify_socket6(sock);
 		break;
+	case AF_VSOCK:
+		xs_reclassify_socket_vsock(sock);
+		break;
 	}
 }
 #else

@@ -2743,7 +2771,7 @@ static struct rpc_xprt_ops xs_local_ops = {
 	.reserve_xprt		= xprt_reserve_xprt,
 	.release_xprt		= xs_tcp_release_xprt,
 	.alloc_slot		= xprt_alloc_slot,
-	.rpcbind		= xs_local_rpcbind,
+	.rpcbind		= xs_dummy_rpcbind,
 	.set_port		= xs_local_set_port,
 	.connect		= xs_local_connect,
 	.buf_alloc		= rpc_malloc,

@@ -2836,6 +2864,10 @@ static int xs_init_anyaddr(const int family, struct sockaddr *sap)
 		.sin6_family		= AF_INET6,
 		.sin6_addr		= IN6ADDR_ANY_INIT,
 	};
+	static const struct sockaddr_vm svm = {
+		.svm_family		= AF_VSOCK,
+		.svm_cid		= VMADDR_CID_ANY,
+	};
 
 	switch (family) {
 	case AF_LOCAL:

@@ -2846,6 +2878,9 @@ static int xs_init_anyaddr(const int family, struct sockaddr *sap)
 	case AF_INET6:
 		memcpy(sap, &sin6, sizeof(sin6));
 		break;
+	case AF_VSOCK:
+		memcpy(sap, &svm, sizeof(svm));
+		break;
 	default:
 		dprintk("RPC:       %s: Bad address family\n", __func__);
 		return -EAFNOSUPPORT;

@@ -3203,6 +3238,330 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	return ret;
 }
 
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+/**
+ * xs_vsock_state_change - callback to handle vsock socket state changes
+ * @sk: socket whose state has changed
+ *
+ */
+static void xs_vsock_state_change(struct sock *sk)
+{
+	struct rpc_xprt *xprt;
+
+	read_lock_bh(&sk->sk_callback_lock);
+	if (!(xprt = xprt_from_sock(sk)))
+		goto out;
+	dprintk("RPC:       %s client %p...\n", __func__, xprt);
+	dprintk("RPC:       state %x conn %d dead %d zapped %d sk_shutdown %d\n",
+			sk->sk_state, xprt_connected(xprt),
+			sock_flag(sk, SOCK_DEAD),
+			sock_flag(sk, SOCK_ZAPPED),
+			sk->sk_shutdown);
+
+	trace_rpc_socket_state_change(xprt, sk->sk_socket);
+
+	switch (sk->sk_state) {
+	case SS_CONNECTING:
+		/* Do nothing */
+		break;
+
+	case SS_CONNECTED:
+		spin_lock(&xprt->transport_lock);
+		if (!xprt_test_and_set_connected(xprt)) {
+			xs_stream_reset_state(xprt, vsock_read_sock);
+			xprt->connect_cookie++;
+
+			xprt_wake_pending_tasks(xprt, -EAGAIN);
+		}
+		spin_unlock(&xprt->transport_lock);
+		break;
+
+	case SS_DISCONNECTING:
+		/* TODO do we need to distinguish between various shutdown (client-side/server-side)? */
+		/* The client initiated a shutdown of the socket */
+		xprt->connect_cookie++;
+		xprt->reestablish_timeout = 0;
+		set_bit(XPRT_CLOSING, &xprt->state);
+		smp_mb__before_atomic();
+		clear_bit(XPRT_CONNECTED, &xprt->state);
+		clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
+		smp_mb__after_atomic();
+		break;
+
+	case SS_UNCONNECTED:
+		xs_sock_mark_closed(xprt);
+		break;
+	}
+
+ out:
+	read_unlock_bh(&sk->sk_callback_lock);
+}
+
+/**
+ * xs_vsock_error_report - callback to handle vsock socket state errors
+ * @sk: socket
+ *
+ * Note: we don't call sock_error() since there may be a rpc_task
+ * using the socket, and so we don't want to clear sk->sk_err.
+ */
+static void xs_vsock_error_report(struct sock *sk)
+{
+	struct rpc_xprt *xprt;
+	int err;
+
+	read_lock_bh(&sk->sk_callback_lock);
+	if (!(xprt = xprt_from_sock(sk)))
+		goto out;
+
+	err = -sk->sk_err;
+	if (err == 0)
+		goto out;

net/sunrpc/xprtsock.c +10 -0

+
+/**
+ * xs_vsock_finish_connecting - initialize and connect socket
+ */
+static int xs_vsock_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
+{
+	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
+	int ret = -ENOTCONN;
+
+	if (!transport->inet) {

net/sunrpc/xprtsock.c +230 -0

+
+		xprt_clear_connected(xprt);
+
+		/* Reset to new socket */
+		transport->sock = sock;
+		transport->inet = sk;
+
+		write_unlock_bh(&sk->sk_callback_lock);
+	}
+
+	if (!xprt_bound(xprt))
+		goto out;
+
+	xs_set_memalloc(xprt);
+
+	/* Tell the socket layer to start connecting... */
+	xprt->stat.connect_count++;
+	xprt->stat.connect_start = jiffies;
+	ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
+	switch (ret) {
+	case 0:
+		xs_set_srcport(transport, sock);
+	case -EINPROGRESS:
+		/* SYN_SENT! */
+		if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
+			xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
+	}
+out:
+	return ret;
+}
+
+/**
+ * xs_vsock_setup_socket - create a vsock socket and connect to a remote endpoint
+ *
+ * Invoked by a work queue tasklet.
+ */
+static void xs_vsock_setup_socket(struct work_struct *work)
+{
+	struct sock_xprt *transport =
+		container_of(work, struct sock_xprt, connect_worker.work);
+	struct socket *sock = transport->sock;
+	struct rpc_xprt *xprt = &transport->xprt;
+	int status = -EIO;
+
+	if (!sock) {
+		sock = xs_create_sock(xprt, transport,
+				xs_addr(xprt)->sa_family, SOCK_STREAM,
+				0, true);
+		if (IS_ERR(sock)) {
+			status = PTR_ERR(sock);
+			goto out;
+		}
+	}
+
+	dprintk("RPC:       worker connecting xprt %p via %s to "
+				"%s (port %s)\n", xprt,
+			xprt->address_strings[RPC_DISPLAY_PROTO],
+			xprt->address_strings[RPC_DISPLAY_ADDR],
+			xprt->address_strings[RPC_DISPLAY_PORT]);
+
+	status = xs_vsock_finish_connecting(xprt, sock);
+	trace_rpc_socket_connect(xprt, sock, status);
+	dprintk("RPC:       %p connect status %d connected %d sock state %d\n",
+			xprt, -status, xprt_connected(xprt),
+			sock->sk->sk_state);
+	switch (status) {
+	default:
+		printk("%s: connect returned unhandled error %d\n",
+			__func__, status);
+	case -EADDRNOTAVAIL:
+		/* We're probably in TIME_WAIT. Get rid of existing socket,
+		 * and retry
+		 */
+		xs_tcp_force_close(xprt);
+		break;
+	case 0:
+	case -EINPROGRESS:
+	case -EALREADY:
+		xprt_unlock_connect(xprt, transport);
+		xprt_clear_connecting(xprt);
+		return;
+	case -EINVAL:
+		/* Happens, for instance, if the user specified a link
+		 * local IPv6 address without a scope-id.
+		 */
+	case -ECONNREFUSED:
+	case -ECONNRESET:
+	case -ENETUNREACH:
+	case -EADDRINUSE:
+	case -ENOBUFS:
+		/* retry with existing socket, after a delay */
+		xs_tcp_force_close(xprt);
+		goto out;
+	}
+	status = -EAGAIN;
+out:
+	xprt_unlock_connect(xprt, transport);
+	xprt_clear_connecting(xprt);
+	xprt_wake_pending_tasks(xprt, status);
+}
+
+/**
+ * xs_vsock_print_stats - display vsock socket-specifc stats
+ * @xprt: rpc_xprt struct containing statistics
+ * @seq: output file
+ *
+ */
+static void xs_vsock_print_stats(struct rpc_xprt *xprt, struct seq_file *seq)
+{
+	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
+	long idle_time = 0;
+
+	if (xprt_connected(xprt))
+		idle_time = (long)(jiffies - xprt->last_used) / HZ;
+
+	seq_printf(seq, "\txprt:\tvsock %u %lu %lu %lu %ld %lu %lu %lu "
+			"%llu %llu %lu %llu %llu\n",
+			transport->srcport,
+			xprt->stat.bind_count,
+			xprt->stat.connect_count,
+			xprt->stat.connect_time,
+			idle_time,
+			xprt->stat.sends,
+			xprt->stat.recvs,
+			xprt->stat.bad_xids,
+			xprt->stat.req_u,
+			xprt->stat.bklog_u,
+			xprt->stat.max_slots,
+			xprt->stat.sending_u,
+			xprt->stat.pending_u);
+}
+
+static struct rpc_xprt_ops xs_vsock_ops = {
+	.reserve_xprt		= xprt_reserve_xprt,
+	.release_xprt		= xs_tcp_release_xprt,
+	.alloc_slot		= xprt_lock_and_alloc_slot,
+	.rpcbind		= xs_dummy_rpcbind,
+	.set_port		= xs_set_port,
+	.connect		= xs_connect,
+	.buf_alloc		= rpc_malloc,
+	.buf_free		= rpc_free,
+	.send_request		= xs_tcp_send_request,
+	.set_retrans_timeout	= xprt_set_retrans_timeout_def,
+	.close			= xs_tcp_shutdown,
+	.destroy		= xs_destroy,
+	.print_stats		= xs_vsock_print_stats,
+};
+
+static const struct rpc_timeout xs_vsock_default_timeout = {
+	.to_initval = 60 * HZ,
+	.to_maxval = 60 * HZ,
+	.to_retries = 2,
+};
+
+/**
+ * xs_setup_vsock - Set up transport to use a vsock socket
+ * @args: rpc transport creation arguments
+ *
+ */
+static struct rpc_xprt *xs_setup_vsock(struct xprt_create *args)
+{
+	struct sockaddr_vm *addr = (struct sockaddr_vm *)args->dstaddr;
+	struct sock_xprt *transport;
+	struct rpc_xprt *xprt;
+	struct rpc_xprt *ret;
+
+	xprt = xs_setup_xprt(args, xprt_tcp_slot_table_entries,
+			     xprt_max_tcp_slot_table_entries);
+	if (IS_ERR(xprt))
+		return xprt;
+	transport = container_of(xprt, struct sock_xprt, xprt);
+
+	xprt->prot = 0;
+	xprt->tsh_size = sizeof(rpc_fraghdr) / sizeof(u32);
+	xprt->max_payload = RPC_MAX_FRAGMENT_SIZE;
+
+	xprt->bind_timeout = XS_BIND_TO;
+	xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
+	xprt->idle_timeout = XS_IDLE_DISC_TO;
+
+	xprt->ops = &xs_vsock_ops;
+	xprt->timeout = &xs_vsock_default_timeout;
+
+	INIT_WORK(&transport->recv_worker, xs_stream_data_receive_workfn);
+	INIT_DELAYED_WORK(&transport->connect_worker, xs_vsock_setup_socket);
+
+	switch (addr->svm_family) {
+	case AF_VSOCK:
+		if (addr->svm_port == 0) {
+			dprintk("RPC:       autobind not supported with AF_VSOCK\n");
+			ret = ERR_PTR(-EINVAL);
+			goto out_err;
+		}
+		xprt_set_bound(xprt);
+		xs_format_peer_addresses(xprt, "vsock", "vsock" /* TODO register official netid? */);
+		break;
+	default:
+		ret = ERR_PTR(-EAFNOSUPPORT);
+		goto out_err;
+	}
+
+	dprintk("RPC:       set up xprt to %s (port %s) via AF_VSOCK\n",
+		xprt->address_strings[RPC_DISPLAY_ADDR],
+		xprt->address_strings[RPC_DISPLAY_PORT]);
+
+	if (try_module_get(THIS_MODULE))
+		return xprt;
+	ret = ERR_PTR(-EINVAL);
+out_err:
+	xs_xprt_free(xprt);
+	return ret;
+}
+#endif
+
 static struct xprt_class	xs_local_transport = {
 	.list		= LIST_HEAD_INIT(xs_local_transport.list),
 	.name		= "named UNIX socket",

@@ -3235,6 +3594,16 @@ static struct xprt_class	xs_bc_tcp_transport = {
 	.setup		= xs_setup_bc_tcp,
 };
 
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+static struct xprt_class	xs_vsock_transport = {
+	.list		= LIST_HEAD_INIT(xs_vsock_transport.list),
+	.name		= "vsock",
+	.owner		= THIS_MODULE,
+	.ident		= XPRT_TRANSPORT_VSOCK,
+	.setup		= xs_setup_vsock,
+};
+#endif
+
 /**
  * init_socket_xprt - set up xprtsock's sysctls, register with RPC client
  *

@@ -3250,6 +3619,9 @@ int init_socket_xprt(void)
 	xprt_register_transport(&xs_udp_transport);
 	xprt_register_transport(&xs_tcp_transport);
 	xprt_register_transport(&xs_bc_tcp_transport);
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+	xprt_register_transport(&xs_vsock_transport);
+#endif
 
 	return 0;
 }

@@ -3271,6 +3643,9 @@ void cleanup_socket_xprt(void)
 	xprt_unregister_transport(&xs_udp_transport);
 	xprt_unregister_transport(&xs_tcp_transport);
 	xprt_unregister_transport(&xs_bc_tcp_transport);
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+	xprt_unregister_transport(&xs_vsock_transport);
+#endif
 }
 
 static int param_set_uint_minmax(const char *val,

Quoted 19 lines · Stefan Hajnoczi

net/sunrpc/xprtsock.c +19 -0

+/**
+ * xs_vsock_error_report - callback to handle vsock socket state errors
+ * @sk: socket
+ *
+ * Note: we don't call sock_error() since there may be a rpc_task
+ * using the socket, and so we don't want to clear sk->sk_err.
+ */
+static void xs_vsock_error_report(struct sock *sk)
+{
+	struct rpc_xprt *xprt;
+	int err;
+
+	read_lock_bh(&sk->sk_callback_lock);
+	if (!(xprt = xprt_from_sock(sk)))
+		goto out;
+
+	err = -sk->sk_err;
+	if (err == 0)
+		goto out;

net/sunrpc/xprtsock.c static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args) · line 3328

Quoted 10 lines

net/sunrpc/xprtsock.c +10 -0

+	/* Is this a reset event? */
+	if (sk->sk_state == SS_UNCONNECTED)
+		xs_sock_mark_closed(xprt);
+	dprintk("RPC:       %s client %p, error=%d...\n",
+			__func__, xprt, -err);
+	trace_rpc_socket_error(xprt, sk->sk_socket, err);
+	xprt_wake_pending_tasks(xprt, err);
+ out:
+	read_unlock_bh(&sk->sk_callback_lock);
+}

Hmm ok...so we have this to avoid some TCP specific stuff in xs_error_report, I guess?

I wonder if AF_LOCAL transport should be using the function above, rather than xs_error_report? If so, maybe we should rename:

    xs_error_report -> xs_tcp_error_report
    xs_vsock_error_report -> xs_stream_error_report

Might be good to do that cleanup first as a preparatory patch.

Quoted 14 lines

net/sunrpc/xprtsock.c +14 -0

+
+/**
+ * xs_vsock_finish_connecting - initialize and connect socket
+ */
+static int xs_vsock_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
+{
+	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
+	int ret = -ENOTCONN;
+
+	if (!transport->inet) {
+		struct sock *sk = sock->sk;
+
+		write_lock_bh(&sk->sk_callback_lock);
+

net/sunrpc/xprtsock.c static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args) · line 3348

Quoted 6 lines

net/sunrpc/xprtsock.c +6 -0

+		xs_save_old_callbacks(transport, sk);
+
+		sk->sk_user_data = xprt;
+		sk->sk_data_ready = xs_data_ready;
+		sk->sk_state_change = xs_vsock_state_change;
+		sk->sk_write_space = xs_tcp_write_space;

Might should rename xs_tcp_write_space to xs_stream_write_space?

net/sunrpc/xprtsock.c static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args) · line 3350

Quoted 2 lines

net/sunrpc/xprtsock.c +2 -0

+		sk->sk_error_report = xs_vsock_error_report;
+		sk->sk_allocation = GFP_ATOMIC;

Why GFP_ATOMIC here? The other finish routines use GFP_NOIO.

svc_bc_tcp_create() is a helper function that simply calls svc_bc_create_socket() with an added IPPROTO_TCP argument.

svc_bc_create_socket() then checks that the protocol argument is indeed IPPROTO_TCP.

This isn't necessary since svc_bc_tcp_create() is the only svc_bc_create_socket() caller. The next patch adds a second caller for AF_VSOCK where IPPROTO_TCP will not be used.

Scrap this scheme and just call svc_bc_create_socket() directly.

Signed-off-by Stefan Hajnoczi <[email protected]>

---
 net/sunrpc/svcsock.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

net/sunrpc/svcsock.c +3 -12

index 2b720fa..394e7a2 100644
@@ -71,7 +71,7 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *, int,
 					  struct net *, struct sockaddr *,
 					  int, int);
 #if defined(CONFIG_SUNRPC_BACKCHANNEL)
-static struct svc_xprt *svc_bc_create_socket(struct svc_serv *, int,
+static struct svc_xprt *svc_bc_create_socket(struct svc_serv *,
 					     struct net *, struct sockaddr *,
 					     int, int);
 static void svc_bc_sock_free(struct svc_xprt *xprt);

@@ -1212,25 +1212,17 @@ static struct svc_xprt *svc_tcp_create(struct svc_serv *serv,
 }
 
 #if defined(CONFIG_SUNRPC_BACKCHANNEL)
-static struct svc_xprt *svc_bc_create_socket(struct svc_serv *, int,
+static struct svc_xprt *svc_bc_create_socket(struct svc_serv *,
 					     struct net *, struct sockaddr *,
 					     int, int);
 static void svc_bc_sock_free(struct svc_xprt *xprt);
 
-static struct svc_xprt *svc_bc_tcp_create(struct svc_serv *serv,
-				       struct net *net,
-				       struct sockaddr *sa, int salen,
-				       int flags)
-{
-	return svc_bc_create_socket(serv, IPPROTO_TCP, net, sa, salen, flags);
-}
-
 static void svc_bc_tcp_sock_detach(struct svc_xprt *xprt)
 {
 }
 
 static struct svc_xprt_ops svc_tcp_bc_ops = {
-	.xpo_create = svc_bc_tcp_create,
+	.xpo_create = svc_bc_create_socket,
 	.xpo_detach = svc_bc_tcp_sock_detach,
 	.xpo_free = svc_bc_sock_free,
 	.xpo_prep_reply_hdr = svc_tcp_prep_reply_hdr,

@@ -1615,7 +1607,6 @@ static void svc_sock_free(struct svc_xprt *xprt)
  * Create a back channel svc_xprt which shares the fore channel socket.
  */
 static struct svc_xprt *svc_bc_create_socket(struct svc_serv *serv,
-					     int protocol,
 					     struct net *net,
 					     struct sockaddr *sin, int len,
 					     int flags)

@@ -1623,12 +1614,6 @@ static struct svc_xprt *svc_bc_create_socket(struct svc_serv *serv,
 	struct svc_sock *svsk;
 	struct svc_xprt *xprt;

Allow creation of AF_VSOCK service xprts. This is needed for the "vsock-bc" backchannel.

Signed-off-by Stefan Hajnoczi <[email protected]>

---
 include/linux/sunrpc/svc_xprt.h | 12 ++++++++++++
 net/sunrpc/svc_xprt.c           | 18 ++++++++++++++++++
 2 files changed, 30 insertions(+)

include/linux/sunrpc/svc_xprt.h +12 -0

index ddb7f94..938f9ee 100644
@@ -8,6 +8,7 @@
 #define SUNRPC_SVC_XPRT_H
 
 #include <linux/sunrpc/svc.h>
+#include <linux/vm_sockets.h>
 
 struct module;
 

@@ -156,12 +157,15 @@ static inline unsigned short svc_addr_port(const struct sockaddr *sa)
 {
 	const struct sockaddr_in *sin = (const struct sockaddr_in *)sa;
 	const struct sockaddr_in6 *sin6 = (const struct sockaddr_in6 *)sa;
+	const struct sockaddr_vm *svm = (const struct sockaddr_vm *)sa;
 
 	switch (sa->sa_family) {
 	case AF_INET:
 		return ntohs(sin->sin_port);
 	case AF_INET6:
 		return ntohs(sin6->sin6_port);
+	case AF_VSOCK:
+		return svm->svm_port;
 	}
 
 	return 0;

@@ -174,6 +178,8 @@ static inline size_t svc_addr_len(const struct sockaddr *sa)
 		return sizeof(struct sockaddr_in);
 	case AF_INET6:
 		return sizeof(struct sockaddr_in6);
+	case AF_VSOCK:
+		return sizeof(struct sockaddr_vm);
 	}
 	BUG();
 }

@@ -193,6 +199,7 @@ static inline char *__svc_print_addr(const struct sockaddr *addr,
 {
 	const struct sockaddr_in *sin = (const struct sockaddr_in *)addr;
 	const struct sockaddr_in6 *sin6 = (const struct sockaddr_in6 *)addr;
+	const struct sockaddr_vm *svm = (const struct sockaddr_vm *)addr;
 
 	switch (addr->sa_family) {
 	case AF_INET:

@@ -206,6 +213,11 @@ static inline char *__svc_print_addr(const struct sockaddr *addr,
 			ntohs(sin6->sin6_port));
 		break;
 
+	case AF_VSOCK:
+		snprintf(buf, len, "%u, port=%u",
+			 svm->svm_cid, svm->svm_port);
+		break;
+
 	default:
 		snprintf(buf, len, "unknown address type: %d", addr->sa_family);
 		break;

net/sunrpc/svc_xprt.c +14 -0

index 7bfe1fb..2a8ccc1 100644
@@ -10,6 +10,7 @@
 #include <linux/kthread.h>
 #include <linux/slab.h>
 #include <net/sock.h>
+#include <net/af_vsock.h>
 #include <linux/sunrpc/addr.h>
 #include <linux/sunrpc/stats.h>
 #include <linux/sunrpc/svc_xprt.h>

@@ -195,6 +196,13 @@ static struct svc_xprt *__svc_xpo_create(struct svc_xprt_class *xcl,
 		.sin6_port		= htons(port),
 	};
 #endif
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+	struct sockaddr_vm svm = {
+		.svm_family		= AF_VSOCK,
+		.svm_cid		= VMADDR_CID_ANY,
+		.svm_port		= port,
+	};
+#endif
 	struct sockaddr *sap;
 	size_t len;
 

@@ -209,6 +217,12 @@ static struct svc_xprt *__svc_xpo_create(struct svc_xprt_class *xcl,
 		len = sizeof(sin6);
 		break;
 #endif
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+	case AF_VSOCK:
+		sap = (struct sockaddr *)&svm;
+		len = sizeof(svm);
+		break;
+#endif
 	default:
 		return ERR_PTR(-EAFNOSUPPORT);
 	}

net/sunrpc/svc_xprt.c int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

net/sunrpc/svc_xprt.c +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
+	case AF_VSOCK:
+		return ((struct sockaddr_vm *)sin)->svm_port <=
+			LAST_RESERVED_PORT;
+
 	default:
 		return 0;
 	}

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

Quoted 17 lines · Jeff Layton

net/sunrpc/svc_xprt.c int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

net/sunrpc/svc_xprt.c +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
+	case AF_VSOCK:
+		return ((struct sockaddr_vm *)sin)->svm_port <=
+			LAST_RESERVED_PORT;
+
 	default:
 		return 0;
 	}

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Quoted 4 lines

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

AF_VSOCK has the same 0-1023 privileged port range as TCP.

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Jeff Layton

net/sunrpc/svc_xprt.c int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

net/sunrpc/svc_xprt.c +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
+	case AF_VSOCK:
+		return ((struct sockaddr_vm *)sin)->svm_port <=
+			LAST_RESERVED_PORT;
+
 	default:
 		return 0;
 	}

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Quoted 7 lines

Quoted 4 lines

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

AF_VSOCK has the same 0-1023 privileged port range as TCP.

But why? And, given that you have 32-bits for a port with AF_VSOCK vs the 16 bits on an AF_INET/AF_INET6, why is the range so pitifully small?

Reserved ports are a bit of a dinosaur holdover from when being root on a machine on the Internet meant something. With v4.1 it's much less of an issue, but in the "olden days", reserved port exhaustion could be a real problem.

Mandating low ports can also be a problem in other way. Some well known services use ports in the ephemeral range, and if your service starts late and someone else has taken the port for an ephemeral one, you're out of luck.

I think we have to ask ourselves:

Should the ability to open a low port inside of a VM carry any significance at all to an RPC server? I'd suggest not, and I think it'd be good to word the RFC to make that explicitly clear.

Quoted 40 lines · Jeff Layton

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Jeff Layton

net/sunrpc/svc_xprt.c int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

net/sunrpc/svc_xprt.c +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
+	case AF_VSOCK:
+		return ((struct sockaddr_vm *)sin)->svm_port <=
+			LAST_RESERVED_PORT;
+
 	default:
 		return 0;
 	}

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Quoted 7 lines

Quoted 4 lines

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

AF_VSOCK has the same 0-1023 privileged port range as TCP.

But why? And, given that you have 32-bits for a port with AF_VSOCK vs the 16 bits on an AF_INET/AF_INET6, why is the range so pitifully small?

Reserved ports are a bit of a dinosaur holdover from when being root on a machine on the Internet meant something. With v4.1 it's much less of an issue, but in the "olden days", reserved port exhaustion could be a real problem.

Mandating low ports can also be a problem in other way. Some well known services use ports in the ephemeral range, and if your service starts late and someone else has taken the port for an ephemeral one, you're out of luck.

Quoted 5 lines

I think we have to ask ourselves:

Should the ability to open a low port inside of a VM carry any significance at all to an RPC server? I'd suggest not, and I think it'd be good to word the RFC to make that explicitly clear.

AF_VSOCK has had the reserved port range since it was first merged in 2013. That's before my time but I do see some use for identifying connections coming from privileged processes.

Given that TCP has the same privileged port range, is there any reason why AF_VSOCK would be any worse off than TCP for having it?

Quoted 50 lines · Stefan Hajnoczi

Quoted 40 lines · Jeff Layton

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Jeff Layton

net/sunrpc/svc_xprt.c int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

net/sunrpc/svc_xprt.c +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
+	case AF_VSOCK:
+		return ((struct sockaddr_vm *)sin)->svm_port <=
+			LAST_RESERVED_PORT;
+
 	default:
 		return 0;
 	}

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Quoted 7 lines

Quoted 4 lines

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

AF_VSOCK has the same 0-1023 privileged port range as TCP.

But why? And, given that you have 32-bits for a port with AF_VSOCK vs the 16 bits on an AF_INET/AF_INET6, why is the range so pitifully small?

Reserved ports are a bit of a dinosaur holdover from when being root on a machine on the Internet meant something. With v4.1 it's much less of an issue, but in the "olden days", reserved port exhaustion could be a real problem.

Mandating low ports can also be a problem in other way. Some well known services use ports in the ephemeral range, and if your service starts late and someone else has taken the port for an ephemeral one, you're out of luck.

Quoted 5 lines

I think we have to ask ourselves:

Should the ability to open a low port inside of a VM carry any significance at all to an RPC server? I'd suggest not, and I think it'd be good to word the RFC to make that explicitly clear.

Quoted 6 lines

AF_VSOCK has had the reserved port range since it was first merged in 2013. That's before my time but I do see some use for identifying connections coming from privileged processes.

Given that TCP has the same privileged port range, is there any reason why AF_VSOCK would be any worse off than TCP for having it?

I agree with Jeff that we need to think carefully about this.

I don't especially care for the privileged port check, but:

In this case, you are inventing an RPC transport that makes it impossible to use strong security (ie, RPCSEC_GSS). We should be careful about removing the check because only AUTH_NULL and AUTH_UNIX security can be used in this kind of deployment.

Privileged ports are easy to spoof if there is an opportunity for a MitM attack to alter the port number of RPCs in transit. With VSOCK there may be no such opportunity, thus privileged ports might provide an adequate level of security here. I make that claim with no deep experience of VSOCK environments.

When writing the VSOCK-related RFCs, you will need to provide a very clear and concise rationale to the IESG for purposely not supporting the use of RPCSEC_GSS. It will start with "well, these RPCs do not flow on open networks and are thus not subject to MitM attacks"; then proceed to careful discussion of how the server will guard against rogue users on guests, and assumptions about the trust relationship between the guests and the host. You will have to make AUTH_UNIX into a defensible deployment choice, and port privilege might be a part of that.

Note also that the NFSv4 standards require that implementations can support RPCSEC_GSS. NFSv4 on VSOCK cannot. Something will have to be done about that.

Quoted 49 lines · Stefan Hajnoczi

Quoted 40 lines · Jeff Layton

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Jeff Layton

net/sunrpc/svc_xprt.c int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

net/sunrpc/svc_xprt.c +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
+	case AF_VSOCK:
+		return ((struct sockaddr_vm *)sin)->svm_port <=
+			LAST_RESERVED_PORT;
+
 	default:
 		return 0;
 	}

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Quoted 7 lines

Quoted 4 lines

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

AF_VSOCK has the same 0-1023 privileged port range as TCP.

But why? And, given that you have 32-bits for a port with AF_VSOCK vs the 16 bits on an AF_INET/AF_INET6, why is the range so pitifully small?

Reserved ports are a bit of a dinosaur holdover from when being root on a machine on the Internet meant something. With v4.1 it's much less of an issue, but in the "olden days", reserved port exhaustion could be a real problem.

Mandating low ports can also be a problem in other way. Some well known services use ports in the ephemeral range, and if your service starts late and someone else has taken the port for an ephemeral one, you're out of luck.

Quoted 5 lines

I think we have to ask ourselves:

Should the ability to open a low port inside of a VM carry any significance at all to an RPC server? I'd suggest not, and I think it'd be good to word the RFC to make that explicitly clear.

Quoted 6 lines

AF_VSOCK has had the reserved port range since it was first merged in 2013. That's before my time but I do see some use for identifying connections coming from privileged processes.

Given that TCP has the same privileged port range, is there any reason why AF_VSOCK would be any worse off than TCP for having it?

I agree with Jeff that we need to think carefully about this.

I don't especially care for the privileged port check, but:

In this case, you are inventing an RPC transport that makes it impossible to use strong security (ie, RPCSEC_GSS). We should be careful about removing the check because only AUTH_NULL and AUTH_UNIX security can be used in this kind of deployment.

Privileged ports are easy to spoof if there is an opportunity for a MitM attack to alter the port number of RPCs in transit. With VSOCK there may be no such opportunity, thus privileged ports might provide an adequate level of security here. I make that claim with no deep experience of VSOCK environments.

When writing the VSOCK-related RFCs, you will need to provide a very clear and concise rationale to the IESG for purposely not supporting the use of RPCSEC_GSS. It will start with "well, these RPCs do not flow on open networks and are thus not subject to MitM attacks"; then proceed to careful discussion of how the server will guard against rogue users on guests, and assumptions about the trust relationship between the guests and the host. You will have to make AUTH_UNIX into a defensible deployment choice, and port privilege might be a part of that.

Note also that the NFSv4 standards require that implementations can support RPCSEC_GSS. NFSv4 on VSOCK cannot. Something will have to be done about that.

Quoted 58 lines · Chuck Lever

Quoted 49 lines · Stefan Hajnoczi

Quoted 40 lines · Jeff Layton

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Jeff Layton

Review int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

(quoted diff) +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
+	case AF_VSOCK:
+		return ((struct sockaddr_vm *)sin)->svm_port <=
+			LAST_RESERVED_PORT;
+
 	default:
 		return 0;
 	}

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Quoted 7 lines

Quoted 4 lines

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

AF_VSOCK has the same 0-1023 privileged port range as TCP.

But why? And, given that you have 32-bits for a port with AF_VSOCK vs the 16 bits on an AF_INET/AF_INET6, why is the range so pitifully small?

Reserved ports are a bit of a dinosaur holdover from when being root on a machine on the Internet meant something. With v4.1 it's much less of an issue, but in the "olden days", reserved port exhaustion could be a real problem.

Mandating low ports can also be a problem in other way. Some well known services use ports in the ephemeral range, and if your service starts late and someone else has taken the port for an ephemeral one, you're out of luck.

Quoted 5 lines

I think we have to ask ourselves:

Should the ability to open a low port inside of a VM carry any significance at all to an RPC server? I'd suggest not, and I think it'd be good to word the RFC to make that explicitly clear.

Quoted 6 lines

AF_VSOCK has had the reserved port range since it was first merged in 2013. That's before my time but I do see some use for identifying connections coming from privileged processes.

Given that TCP has the same privileged port range, is there any reason why AF_VSOCK would be any worse off than TCP for having it?

I agree with Jeff that we need to think carefully about this.

Quoted 8 lines

I don't especially care for the privileged port check, but:

In this case, you are inventing an RPC transport that makes it impossible to use strong security (ie, RPCSEC_GSS). We should be careful about removing the check because only AUTH_NULL and AUTH_UNIX security can be used in this kind of deployment.

I know we've discussed this a bit, but does this transport _really_ preclude us from using RPCSEC_GSS? I know we don't have IP addresses here, but hosts on either end of a vsocket will have hostnames.

WRT kerberos, I don't see a reason why both hosts couldn't communicate with a KDC via other means, get tickets and then use those for authenticating over their vsock connection. vsock might make it harder to determine what SPN to use, but we could potentially work around that in other ways.

Quoted 16 lines

Privileged ports are easy to spoof if there is an opportunity for a MitM attack to alter the port number of RPCs in transit. With VSOCK there may be no such opportunity, thus privileged ports might provide an adequate level of security here. I make that claim with no deep experience of VSOCK environments.

When writing the VSOCK-related RFCs, you will need to provide a very clear and concise rationale to the IESG for purposely not supporting the use of RPCSEC_GSS. It will start with "well, these RPCs do not flow on open networks and are thus not subject to MitM attacks"; then proceed to careful discussion of how the server will guard against rogue users on guests, and assumptions about the trust relationship between the guests and the host. You will have to make AUTH_UNIX into a defensible deployment choice, and port privilege might be a part of that.

Quoted 4 lines

Note also that the NFSv4 standards require that implementations can support RPCSEC_GSS. NFSv4 on VSOCK cannot. Something will have to be done about that.

Good point on the reserved port value for AUTH_UNIX. That may be reason enough to keep it in.

Quoted 71 lines · Jeff Layton

Quoted 58 lines · Chuck Lever

Quoted 49 lines · Stefan Hajnoczi

Quoted 40 lines · Jeff Layton

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Jeff Layton

Review int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

(quoted diff) +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
+	case AF_VSOCK:
+		return ((struct sockaddr_vm *)sin)->svm_port <=
+			LAST_RESERVED_PORT;
+
 	default:
 		return 0;
 	}

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Quoted 7 lines

Quoted 4 lines

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

AF_VSOCK has the same 0-1023 privileged port range as TCP.

But why? And, given that you have 32-bits for a port with AF_VSOCK vs the 16 bits on an AF_INET/AF_INET6, why is the range so pitifully small?

Reserved ports are a bit of a dinosaur holdover from when being root on a machine on the Internet meant something. With v4.1 it's much less of an issue, but in the "olden days", reserved port exhaustion could be a real problem.

Mandating low ports can also be a problem in other way. Some well known services use ports in the ephemeral range, and if your service starts late and someone else has taken the port for an ephemeral one, you're out of luck.

Quoted 5 lines

I think we have to ask ourselves:

Should the ability to open a low port inside of a VM carry any significance at all to an RPC server? I'd suggest not, and I think it'd be good to word the RFC to make that explicitly clear.

Quoted 6 lines

AF_VSOCK has had the reserved port range since it was first merged in 2013. That's before my time but I do see some use for identifying connections coming from privileged processes.

Given that TCP has the same privileged port range, is there any reason why AF_VSOCK would be any worse off than TCP for having it?

I agree with Jeff that we need to think carefully about this.

Quoted 8 lines

I don't especially care for the privileged port check, but:

In this case, you are inventing an RPC transport that makes it impossible to use strong security (ie, RPCSEC_GSS). We should be careful about removing the check because only AUTH_NULL and AUTH_UNIX security can be used in this kind of deployment.

Quoted 3 lines

I know we've discussed this a bit, but does this transport _really_ preclude us from using RPCSEC_GSS? I know we don't have IP addresses here, but hosts on either end of a vsocket will have hostnames.

Yes, even for AUTH_UNIX, something has to go in the "hostname" field in the credential. Let's say the guest's uname.

Quoted 5 lines

WRT kerberos, I don't see a reason why both hosts couldn't communicate with a KDC via other means, get tickets and then use those for authenticating over their vsock connection. vsock might make it harder to determine what SPN to use, but we could potentially work around that in other ways.

"No network configuration" implies to me that the KDC (or a proxy for it) would have to reside on the host.

Quoted 3 lines

Quoted 3 lines

Note also that the NFSv4 standards require that implementations can support RPCSEC_GSS. NFSv4 on VSOCK cannot. Something will have to be done about that.

Quoted 79 lines · Chuck Lever

Quoted 71 lines · Jeff Layton

Quoted 58 lines · Chuck Lever

Quoted 49 lines · Stefan Hajnoczi

Quoted 40 lines · Jeff Layton

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Jeff Layton

Review int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

(quoted diff) +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
 	case AF_INET6:
 		return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
 			< PROT_SOCK;
+	case AF_VSOCK:
+		return ((struct sockaddr_vm *)sin)->svm_port <=
+			LAST_RESERVED_PORT;
+
 	default:
 		return 0;
 	}

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Quoted 7 lines

Quoted 4 lines

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

AF_VSOCK has the same 0-1023 privileged port range as TCP.

But why? And, given that you have 32-bits for a port with AF_VSOCK vs the 16 bits on an AF_INET/AF_INET6, why is the range so pitifully small?

Reserved ports are a bit of a dinosaur holdover from when being root on a machine on the Internet meant something. With v4.1 it's much less of an issue, but in the "olden days", reserved port exhaustion could be a real problem.

Mandating low ports can also be a problem in other way. Some well known services use ports in the ephemeral range, and if your service starts late and someone else has taken the port for an ephemeral one, you're out of luck.

Quoted 5 lines

I think we have to ask ourselves:

Should the ability to open a low port inside of a VM carry any significance at all to an RPC server? I'd suggest not, and I think it'd be good to word the RFC to make that explicitly clear.

Quoted 6 lines

AF_VSOCK has had the reserved port range since it was first merged in 2013. That's before my time but I do see some use for identifying connections coming from privileged processes.

Given that TCP has the same privileged port range, is there any reason why AF_VSOCK would be any worse off than TCP for having it?

I agree with Jeff that we need to think carefully about this.

Quoted 8 lines

I don't especially care for the privileged port check, but:

In this case, you are inventing an RPC transport that makes it impossible to use strong security (ie, RPCSEC_GSS). We should be careful about removing the check because only AUTH_NULL and AUTH_UNIX security can be used in this kind of deployment.

Quoted 3 lines

I know we've discussed this a bit, but does this transport _really_ preclude us from using RPCSEC_GSS? I know we don't have IP addresses here, but hosts on either end of a vsocket will have hostnames.

Yes, even for AUTH_UNIX, something has to go in the "hostname" field in the credential. Let's say the guest's uname.

Quoted 8 lines

Quoted 5 lines

WRT kerberos, I don't see a reason why both hosts couldn't communicate with a KDC via other means, get tickets and then use those for authenticating over their vsock connection. vsock might make it harder to determine what SPN to use, but we could potentially work around that in other ways.

"No network configuration" implies to me that the KDC (or a proxy for it) would have to reside on the host.

Their requirement is that network configuration not be mandatory, not that it always be absent.

Then again maybe rpcsec_gss/vsock loses any advantage over rpcsec_gss/tcp if the former always requires a network anyway.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Note also that the NFSv4 standards require that implementations can support RPCSEC_GSS. NFSv4 on VSOCK cannot. Something will have to be done about that.

Which might be: "Make an argument for why that requirement produces no useful result in this case". ?

Quoted 80 lines · Bruce Fields

Quoted 79 lines · Chuck Lever

Quoted 71 lines · Jeff Layton

Quoted 58 lines · Chuck Lever

Quoted 49 lines · Stefan Hajnoczi

Quoted 40 lines · Jeff Layton

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Jeff Layton

Review int svc_port_is_privileged(struct sockaddr *sin) · line 618

Quoted 11 lines · Stefan Hajnoczi

(quoted diff) +4 -0

@@ -595,6 +609,10 @@ int svc_port_is_privileged(struct sockaddr *sin)
      case AF_INET6:
              return ntohs(((struct sockaddr_in6 *)sin)->sin6_port)
                      < PROT_SOCK;
+     case AF_VSOCK:
+             return ((struct sockaddr_vm *)sin)->svm_port <=
+                     LAST_RESERVED_PORT;
+
      default:
              return 0;
      }

Does vsock even have the concept of a privileged port? I would imagine that root in a guest VM would carry no particular significance from an export security standpoint

Quoted 7 lines

Quoted 4 lines

Since you're defining a new transport here, it might be nice write the RFCs to avoid that distinction, if possible.

Note that RDMA just has svc_port_is_privileged always return 1.

AF_VSOCK has the same 0-1023 privileged port range as TCP.

But why? And, given that you have 32-bits for a port with AF_VSOCK vs the 16 bits on an AF_INET/AF_INET6, why is the range so pitifully small?

Reserved ports are a bit of a dinosaur holdover from when being root on a machine on the Internet meant something. With v4.1 it's much less of an issue, but in the "olden days", reserved port exhaustion could be a real problem.

Mandating low ports can also be a problem in other way. Some well known services use ports in the ephemeral range, and if your service starts late and someone else has taken the port for an ephemeral one, you're out of luck.

Quoted 5 lines

I think we have to ask ourselves:

Should the ability to open a low port inside of a VM carry any significance at all to an RPC server? I'd suggest not, and I think it'd be good to word the RFC to make that explicitly clear.

Quoted 6 lines

AF_VSOCK has had the reserved port range since it was first merged in 2013. That's before my time but I do see some use for identifying connections coming from privileged processes.

Given that TCP has the same privileged port range, is there any reason why AF_VSOCK would be any worse off than TCP for having it?

I agree with Jeff that we need to think carefully about this.

Quoted 8 lines

I don't especially care for the privileged port check, but:

In this case, you are inventing an RPC transport that makes it impossible to use strong security (ie, RPCSEC_GSS). We should be careful about removing the check because only AUTH_NULL and AUTH_UNIX security can be used in this kind of deployment.

Quoted 3 lines

I know we've discussed this a bit, but does this transport _really_ preclude us from using RPCSEC_GSS? I know we don't have IP addresses here, but hosts on either end of a vsocket will have hostnames.

Yes, even for AUTH_UNIX, something has to go in the "hostname" field in the credential. Let's say the guest's uname.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

WRT kerberos, I don't see a reason why both hosts couldn't communicate with a KDC via other means, get tickets and then use those for authenticating over their vsock connection. vsock might make it harder to determine what SPN to use, but we could potentially work around that in other ways.

"No network configuration" implies to me that the KDC (or a proxy for it) would have to reside on the host.

A proxy would be fine. The whole point of krb5 is that you can't rely on
the network anyway...

Quoted 7 lines

Their requirement is that network configuration not be mandatory, not that it always be absent.

Then again maybe rpcsec_gss/vsock loses any advantage over rpcsec_gss/tcp if the former always requires a network anyway.

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Note also that the NFSv4 standards require that implementations can support RPCSEC_GSS. NFSv4 on VSOCK cannot. Something will have to be done about that.

Which might be: "Make an argument for why that requirement produces no useful result in this case". ?

I guess it just seems like we can allow RPCSEC_GSS over this channel, even if it's not terribly useful today. I don't think we ought to word in a way that specifically forbids it, unless it really does fall short of some key requirement.

GSSAPI is more than just krb5 too...maybe LIPKEY will make a comeback someday? :)

--
Jeff Layton <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signed-off-by Stefan Hajnoczi <[email protected]>

---
 net/sunrpc/svcsock.c | 214 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 190 insertions(+), 24 deletions(-)

net/sunrpc/svcsock.c +161 -14

index e67b097..86cce8f 100644
@@ -42,7 +42,9 @@
 #include <net/udp.h>
 #include <net/tcp.h>
 #include <net/tcp_states.h>
+#include <net/af_vsock.h>
 #include <linux/uaccess.h>
+#include <asm/uaccess.h>
 #include <asm/ioctls.h>
 #include <trace/events/skb.h>
 

@@ -64,7 +66,7 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *, struct socket *,
 static int		svc_udp_recvfrom(struct svc_rqst *);
 static int		svc_udp_sendto(struct svc_rqst *);
 static void		svc_sock_detach(struct svc_xprt *);
-static void		svc_tcp_sock_detach(struct svc_xprt *);
+static void		svc_common_sock_detach(struct svc_xprt *);
 static void		svc_sock_free(struct svc_xprt *);
 
 static struct svc_xprt *svc_create_socket(struct svc_serv *, int,

@@ -810,9 +812,9 @@ static void svc_tcp_state_change(struct sock *sk)
 }
 
 /*
- * Accept a TCP connection
+ * Accept an incoming connection
  */
-static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)
+static struct svc_xprt *svc_common_accept(struct svc_xprt *xprt)
 {
 	struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
 	struct sockaddr_storage addr;

@@ -824,7 +826,7 @@ static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)
 	int		err, slen;
 	RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]);
 
-	dprintk("svc: tcp_accept %p sock %p\n", svsk, sock);
+	dprintk("svc: %s %p sock %p\n", __func__, svsk, sock);
 	if (!sock)
 		return NULL;
 

@@ -877,7 +879,7 @@ static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)
 	svc_xprt_set_remote(&newsvsk->sk_xprt, sin, slen);
 	err = kernel_getsockname(newsock, sin, &slen);
 	if (unlikely(err < 0)) {
-		dprintk("svc_tcp_accept: kernel_getsockname error %d\n", -err);
+		dprintk("%s: kernel_getsockname error %d\n", __func__, -err);
 		slen = offsetof(struct sockaddr, sa_data);
 	}
 	svc_xprt_set_local(&newsvsk->sk_xprt, sin, slen);

@@ -1131,7 +1133,7 @@ static int svc_tcp_recvfrom(struct svc_rqst *rqstp)
 		rqstp->rq_arg.page_len = rqstp->rq_arg.len - rqstp->rq_arg.head[0].iov_len;
 
 	rqstp->rq_xprt_ctxt   = NULL;
-	rqstp->rq_prot	      = IPPROTO_TCP;
+	rqstp->rq_prot	      = IPPROTO_TCP; /* TODO vsock should either use 0 or IPPROTO_MAX */
 	if (test_bit(XPT_LOCAL, &svsk->sk_xprt.xpt_flags))
 		set_bit(RQ_LOCAL, &rqstp->rq_flags);
 	else

@@ -1200,13 +1202,13 @@ static int svc_tcp_sendto(struct svc_rqst *rqstp)
 }
 
 /*
- * Setup response header. TCP has a 4B record length field.
+ * Setup response header. Byte stream transports have a 4B record length field.
  */
-static void svc_tcp_prep_reply_hdr(struct svc_rqst *rqstp)
+static void svc_stream_prep_reply_hdr(struct svc_rqst *rqstp)
 {
 	struct kvec *resv = &rqstp->rq_res.head[0];
 
-	/* tcp needs a space for the record length... */
+	/* space for the record length... */
 	svc_putnl(resv, 0);
 }
 

@@ -1232,7 +1234,7 @@ static struct svc_xprt_ops svc_tcp_bc_ops = {
 	.xpo_create = svc_bc_create_socket,
 	.xpo_detach = svc_bc_tcp_sock_detach,
 	.xpo_free = svc_bc_sock_free,
-	.xpo_prep_reply_hdr = svc_tcp_prep_reply_hdr,
+	.xpo_prep_reply_hdr = svc_stream_prep_reply_hdr,
 	.xpo_secure_port = svc_sock_secure_port,
 };
 

@@ -1244,6 +1246,145 @@ static struct svc_xprt_class svc_tcp_bc_class = {
 };
 
 #ifdef CONFIG_SUNRPC_XPRT_VSOCK
+/*
+ * A data_ready event on a listening socket means there's a connection
+ * pending. Do not use state_change as a substitute for it.
+ */
+static void svc_vsock_listen_data_ready(struct sock *sk)
+{
+	struct svc_sock	*svsk = (struct svc_sock *)sk->sk_user_data;
+	wait_queue_head_t *wq;
+
+	dprintk("svc: socket %p AF_VSOCK (listen) state change %d\n",
+		sk, sk->sk_state);
+
+	/*
+	 * This callback may called twice when a new connection
+	 * is established as a child socket inherits everything
+	 * from a parent VSOCK_SS_LISTEN socket.
+	 * 1) data_ready method of the parent socket will be called
+	 *    when one of child sockets become SS_CONNECTED.
+	 * 2) data_ready method of the child socket may be called
+	 *    when it receives data before the socket is accepted.
+	 * In case of 2, we should ignore it silently.
+	 */
+	if (sk->sk_state == VSOCK_SS_LISTEN) {
+		if (svsk) {
+			set_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);
+			svc_xprt_enqueue(&svsk->sk_xprt);
+		} else
+			printk("svc: socket %p: no user data\n", sk);
+	}
+
+	wq = sk_sleep(sk);
+	if (wq && waitqueue_active(wq))
+		wake_up_interruptible_all(wq);
+}
+
+/*
+ * A state change on a connected socket means it's dying or dead.
+ */
+static void svc_vsock_state_change(struct sock *sk)
+{
+	struct svc_sock	*svsk = (struct svc_sock *)sk->sk_user_data;
+	wait_queue_head_t *wq = sk_sleep(sk);
+
+	dprintk("svc: socket %p AF_VSOCK (connected) state change %d (svsk %p)\n",
+		sk, sk->sk_state, sk->sk_user_data);
+
+	if (!svsk)
+		printk("svc: socket %p: no user data\n", sk);
+	else {
+		set_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags);
+		svc_xprt_enqueue(&svsk->sk_xprt);
+	}
+	if (wq && waitqueue_active(wq))
+		wake_up_interruptible_all(wq);
+}
+
+static void svc_vsock_data_ready(struct sock *sk)
+{
+	struct svc_sock *svsk = (struct svc_sock *)sk->sk_user_data;
+	wait_queue_head_t *wq = sk_sleep(sk);
+
+	dprintk("svc: socket %p AF_VSOCK data ready (svsk %p)\n",
+		sk, sk->sk_user_data);
+	if (svsk) {
+		set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
+		svc_xprt_enqueue(&svsk->sk_xprt);
+	}
+	if (wq && waitqueue_active(wq))
+		wake_up_interruptible(wq);
+}
+
+static int svc_vsock_has_wspace(struct svc_xprt *xprt)
+{
+	struct svc_sock *svsk =	container_of(xprt, struct svc_sock, sk_xprt);
+
+	if (test_bit(XPT_LISTENER, &xprt->xpt_flags))
+		return 1;
+	return !test_bit(SOCK_NOSPACE, &svsk->sk_sock->flags);
+}
+
+static struct svc_xprt *svc_vsock_create(struct svc_serv *serv,
+					 struct net *net,
+					 struct sockaddr *sa, int salen,
+					 int flags)
+{
+	return svc_create_socket(serv, 0, net, sa, salen, flags);
+}
+
+static struct svc_xprt_ops svc_vsock_ops = {
+	.xpo_create = svc_vsock_create,
+	.xpo_recvfrom = svc_tcp_recvfrom,
+	.xpo_sendto = svc_tcp_sendto,
+	.xpo_release_rqst = svc_release_skb,
+	.xpo_detach = svc_common_sock_detach,
+	.xpo_free = svc_sock_free,
+	.xpo_prep_reply_hdr = svc_stream_prep_reply_hdr,
+	.xpo_has_wspace = svc_vsock_has_wspace,
+	.xpo_accept = svc_common_accept,
+	.xpo_secure_port = svc_sock_secure_port,
+};
+
+static struct svc_xprt_class svc_vsock_class = {
+	.xcl_name = "vsock",
+	.xcl_owner = THIS_MODULE,
+	.xcl_ops = &svc_vsock_ops,
+	.xcl_max_payload = RPCSVC_MAXPAYLOAD,
+	.xcl_ident = XPRT_TRANSPORT_VSOCK,
+};
+
+static void svc_vsock_init(struct svc_sock *svsk, struct svc_serv *serv)
+{
+	struct sock *sk = svsk->sk_sk;
+
+	svc_xprt_init(sock_net(svsk->sk_sock->sk), &svc_vsock_class,
+		      &svsk->sk_xprt, serv);
+	set_bit(XPT_CACHE_AUTH, &svsk->sk_xprt.xpt_flags);
+	set_bit(XPT_CONG_CTRL, &svsk->sk_xprt.xpt_flags);
+	if (sk->sk_state == VSOCK_SS_LISTEN) {
+		dprintk("setting up AF_VSOCK socket for listening\n");
+		set_bit(XPT_LISTENER, &svsk->sk_xprt.xpt_flags);
+		sk->sk_data_ready = svc_vsock_listen_data_ready;
+		set_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);
+	} else {
+		dprintk("setting up AF_VSOCK socket for reading\n");
+		sk->sk_state_change = svc_vsock_state_change;
+		sk->sk_data_ready = svc_vsock_data_ready;
+		sk->sk_write_space = svc_write_space;
+
+		svsk->sk_reclen = 0;
+		svsk->sk_tcplen = 0;
+		svsk->sk_datalen = 0;
+		memset(&svsk->sk_pages[0], 0, sizeof(svsk->sk_pages));
+
+		set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
+		if (sk->sk_state != SS_CONNECTED)
+			set_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags);
+	}
+}
+
 static void svc_bc_vsock_sock_detach(struct svc_xprt *xprt)
 {
 }

@@ -1252,7 +1393,7 @@ static struct svc_xprt_ops svc_vsock_bc_ops = {
 	.xpo_create = svc_bc_create_socket,
 	.xpo_detach = svc_bc_vsock_sock_detach,
 	.xpo_free = svc_bc_sock_free,
-	.xpo_prep_reply_hdr = svc_tcp_prep_reply_hdr,
+	.xpo_prep_reply_hdr = svc_stream_prep_reply_hdr,
 	.xpo_secure_port = svc_sock_secure_port,
 };
 

@@ -1294,11 +1435,11 @@ static struct svc_xprt_ops svc_tcp_ops = {
 	.xpo_recvfrom = svc_tcp_recvfrom,
 	.xpo_sendto = svc_tcp_sendto,
 	.xpo_release_rqst = svc_release_skb,
-	.xpo_detach = svc_tcp_sock_detach,
+	.xpo_detach = svc_common_sock_detach,
 	.xpo_free = svc_sock_free,
-	.xpo_prep_reply_hdr = svc_tcp_prep_reply_hdr,
+	.xpo_prep_reply_hdr = svc_stream_prep_reply_hdr,
 	.xpo_has_wspace = svc_tcp_has_wspace,
-	.xpo_accept = svc_tcp_accept,
+	.xpo_accept = svc_common_accept,
 	.xpo_secure_port = svc_sock_secure_port,
 	.xpo_kill_temp_xprt = svc_tcp_kill_temp_xprt,
 };

@@ -1315,6 +1456,9 @@ void svc_init_xprt_sock(void)
 {
 	svc_reg_xprt_class(&svc_tcp_class);
 	svc_reg_xprt_class(&svc_udp_class);
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+	svc_reg_xprt_class(&svc_vsock_class);
+#endif
 	svc_init_bc_xprt_sock();
 }
 

@@ -1322,6 +1466,9 @@ void svc_cleanup_xprt_sock(void)
 {
 	svc_unreg_xprt_class(&svc_tcp_class);
 	svc_unreg_xprt_class(&svc_udp_class);
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+	svc_unreg_xprt_class(&svc_vsock_class);
+#endif
 	svc_cleanup_bc_xprt_sock();
 }
 

net/sunrpc/svcsock.c +25 -10

@@ -1468,13 +1619,22 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
 
 	if (!so)
 		return err;
-	err = -EAFNOSUPPORT;
-	if ((so->sk->sk_family != PF_INET) && (so->sk->sk_family != PF_INET6))
-		goto out;
-	err =  -EPROTONOSUPPORT;
-	if (so->sk->sk_protocol != IPPROTO_TCP &&
-	    so->sk->sk_protocol != IPPROTO_UDP)
+	err = -EPROTONOSUPPORT;
+	switch (so->sk->sk_family) {
+	case PF_INET:
+	case PF_INET6:
+		if (so->sk->sk_protocol != IPPROTO_TCP &&
+		    so->sk->sk_protocol != IPPROTO_UDP)
+			goto out;
+		break;
+	case PF_VSOCK:
+		if (so->sk->sk_protocol != 0)
+			goto out;
+		break;
+	default:
+		err = -EAFNOSUPPORT;
 		goto out;
+	}
 	err = -EISCONN;
 	if (so->state > SS_UNCONNECTED)
 		goto out;

@@ -1521,7 +1681,8 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv,
 			serv->sv_program->pg_name, protocol,
 			__svc_print_addr(sin, buf, sizeof(buf)));
 
-	if (protocol != IPPROTO_UDP && protocol != IPPROTO_TCP) {
+	if (sin->sa_family != AF_VSOCK &&
+	    protocol != IPPROTO_UDP && protocol != IPPROTO_TCP) {
 		printk(KERN_WARNING "svc: only UDP and TCP "
 				"sockets supported\n");
 		return ERR_PTR(-EINVAL);

@@ -1535,6 +1696,11 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv,
 	case AF_INET:
 		family = PF_INET;
 		break;
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+	case AF_VSOCK:
+		family = PF_VSOCK;
+		break;
+#endif
 	default:
 		return ERR_PTR(-EINVAL);
 	}

@@ -1566,7 +1732,7 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv,
 	if (error < 0)
 		goto bummer;
 
-	if (protocol == IPPROTO_TCP) {
+	if (type == SOCK_STREAM) {
 		if ((error = kernel_listen(sock, 64)) < 0)
 			goto bummer;
 	}

@@ -1607,11 +1773,11 @@ static void svc_sock_detach(struct svc_xprt *xprt)
 /*
  * Disconnect the socket, and reset the callbacks
  */
-static void svc_tcp_sock_detach(struct svc_xprt *xprt)
+static void svc_common_sock_detach(struct svc_xprt *xprt)
 {
 	struct svc_sock *svsk = container_of(xprt, struct svc_sock, sk_xprt);
 
-	dprintk("svc: svc_tcp_sock_detach(%p)\n", svsk);
+	dprintk("svc: %s(%p)\n", __func__, svsk);
 
 	svc_sock_detach(xprt);
 

net/sunrpc/svcsock.c static struct svc_sock *svc_setup_socket(struct svc_serv *serv, · line 1572

Quoted 10 lines · Stefan Hajnoczi

net/sunrpc/svcsock.c +4 -0

@@ -1417,6 +1564,10 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 	/* Initialize the socket */
 	if (sock->type == SOCK_DGRAM)
 		svc_udp_init(svsk, serv);
+#ifdef CONFIG_SUNRPC_XPRT_VSOCK
+	else if (inet->sk_family == AF_VSOCK)
+		svc_vsock_init(svsk, serv);
+#endif
 	else
 		svc_tcp_init(svsk, serv);

The above conditional is a bit of a mess and doesn't really handle the case where the upper layer might pass it something non-sensical (i.e. SOCK_DGRAM + AF_VSOCK).

I think this should vet both values and return ERR_PTR(-EINVAL) if it's not right. Maybe a switch statement on the address family and then check the sock->type in each? We can afford to code a little defensively here.