Skip to contentSkip to conversation

rds: IPv6 support

PATCH Superseded Ka-Cheong Poon 4 participants 18 messages Jun 27, 2018 – Jul 9, 2018

Subsystems (3)

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v2 cover rds: IPv6 support

This patch set adds IPv6 support to the kernel RDS and related modules. Existing RDS apps using IPv4 address continue to run without any problem. New RDS apps which want to use IPv6 address can do so by passing the address in struct sockaddr_in6 to bind(), connect() or sendmsg(). And those apps also need to use the new IPv6 equivalents of some of the existing socket options as the existing options use a 32 bit integer to store IP address.

All RDS code now use struct in6_addr to store IP address. IPv4 address is stored as an IPv4 mapped address.

Header file changes

There are many data structures (RDS socket options) used by RDS apps which use a 32 bit integer to store IP address. To support IPv6, struct in6_addr needs to be used. To ensure backward compatibility, a new data structure is introduced for each of those data structures which use a 32 bit integer to represent an IP address. And new socket options are introduced to use those new structures. This means that existing apps should work without a problem with the new RDS module. For apps which want to use IPv6, those new data structures and socket options can be used. IPv4 mapped address is used to represent IPv4 address in the new data structures.

Internally, all RDS data structures which contain an IP address are changed to use struct in6_addr to store the address. IPv4 address is stored as an IPv4 mapped address. All the functions which take an IP address as argument are also changed to use struct in6_addr.

RDS/RDMA/IB uses a private data (struct rds_ib_connect_private) exchange between endpoints at RDS connection establishment time to support RDMA. This private data exchange uses a 32 bit integer to represent an IP address. This needs to be changed in order to support IPv6. A new private data struct rds6_ib_connect_private is introduced to handle this. To ensure backward compatibility, an IPv6 capable RDS stack uses another RDMA listener port (RDS_CM_PORT) to accept IPv6 connection. And it continues to use the original RDS_PORT for IPv4 RDS connections. When it needs to communicate with an IPv6 peer, it uses the RDS_TCP_PORT to send the connection set up request.

RDS/TCP changes

TCP related code is changed to support IPv6. Note that only an IPv6 TCP listener on port RDS_TCP_PORT is created as it can accept both IPv4 and IPv6 connection requests.

IB/RDMA changes

The initial private data exchange between IB endpoints using RDMA is changed to support IPv6 address instead, if the peer address is IPv6. To ensure backward compatibility, annother RDMA listener port (RDS_CM_PORT) is used to accept IPv6 connection. An IPv6 capable RDS module continues to use the original RDS_PORT for IPv4 RDS connections. When it needs to communicate with an IPv6 peer, it uses the RDS_CM_PORT to send the connection set up request.

Ka-Cheong Poon (3):
rds: Changing IP address internal representation to struct in6_addr
rds: Enable RDS IPv6 support
rds: Extend RDS API for IPv6 support

 include/uapi/linux/rds.h |  71 ++++++++++-
 net/rds/af_rds.c         | 164 ++++++++++++++++++++------
 net/rds/bind.c           | 119 ++++++++++++++-----
 net/rds/cong.c           |  23 ++--
 net/rds/connection.c     | 249 +++++++++++++++++++++++++++++----------
 net/rds/ib.c             | 114 ++++++++++++++++--
 net/rds/ib.h             |  45 +++++--
 net/rds/ib_cm.c          | 301 +++++++++++++++++++++++++++++++++++------------
 net/rds/ib_mr.h          |   2 +
 net/rds/ib_rdma.c        |  24 ++--
 net/rds/ib_recv.c        |  18 +--
 net/rds/ib_send.c        |  10 +-
 net/rds/loop.c           |   7 +-
 net/rds/rdma.c           |   6 +-
 net/rds/rdma_transport.c |  84 ++++++++++---
 net/rds/rdma_transport.h |   2 +
 net/rds/rds.h            |  85 ++++++++-----
 net/rds/recv.c           |  76 +++++++++---
 net/rds/send.c           |  95 ++++++++++++---
 net/rds/tcp.c            | 128 ++++++++++++++++----
 net/rds/tcp.h            |   4 +-
 net/rds/tcp_connect.c    |  68 ++++++++---
 net/rds/tcp_listen.c     |  58 ++++++---
 net/rds/tcp_recv.c       |   9 +-
 net/rds/tcp_send.c       |   4 +-
 net/rds/threads.c        |  69 +++++++++--
 net/rds/transport.c      |  15 ++-
 27 files changed, 1426 insertions(+), 424 deletions(-)
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH v2 1/3 rds: Changing IP address internal representation to struct in6_addr

This patch changes the internal representation of an IP address to use struct in6_addr. IPv4 address is stored as an IPv4 mapped address. All the functions which take an IP address as argument are also changed to use struct in6_addr. But RDS socket layer is not modified such that it still does not accept IPv6 address from an application. And RDS layer does not accept nor initiate IPv6 connections.

v2: Fixed sparse warnings.

---
 net/rds/af_rds.c         | 138 ++++++++++++++++------
 net/rds/bind.c           |  91 +++++++++-----
 net/rds/cong.c           |  23 ++--
 net/rds/connection.c     | 132 +++++++++++++--------
 net/rds/ib.c             |  17 +--
 net/rds/ib.h             |  45 +++++--
 net/rds/ib_cm.c          | 300 +++++++++++++++++++++++++++++++++++------------
 net/rds/ib_rdma.c        |  15 +--
 net/rds/ib_recv.c        |  18 +--
 net/rds/ib_send.c        |  10 +-
 net/rds/loop.c           |   7 +-
 net/rds/rdma.c           |   6 +-
 net/rds/rdma_transport.c |  56 ++++++---
 net/rds/rds.h            |  69 +++++++----
 net/rds/recv.c           |  51 +++++---
 net/rds/send.c           |  67 ++++++++---
 net/rds/tcp.c            |  32 ++++-
 net/rds/tcp_connect.c    |  34 +++---
 net/rds/tcp_listen.c     |  18 +--
 net/rds/tcp_recv.c       |   9 +-
 net/rds/tcp_send.c       |   4 +-
 net/rds/threads.c        |  69 +++++++++--
 net/rds/transport.c      |  15 ++-
 23 files changed, 857 insertions(+), 369 deletions(-)
net/rds/af_rds.c +99 -39
index ab751a1..fc1a5c6 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2018 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -35,6 +35,7 @@
 #include <linux/kernel.h>
 #include <linux/gfp.h>
 #include <linux/in.h>
+#include <linux/ipv6.h>
 #include <linux/poll.h>
 #include <net/sock.h>
 
@@ -113,26 +114,63 @@ void rds_wake_sk_sleep(struct rds_sock *rs)
 static int rds_getname(struct socket *sock, struct sockaddr *uaddr,
 		       int peer)
 {
-	struct sockaddr_in *sin = (struct sockaddr_in *)uaddr;
 	struct rds_sock *rs = rds_sk_to_rs(sock->sk);
-
-	memset(sin->sin_zero, 0, sizeof(sin->sin_zero));
+	struct sockaddr_in6 *sin6;
+	struct sockaddr_in *sin;
+	int uaddr_len;
 
 	/* racey, don't care */
 	if (peer) {
-		if (!rs->rs_conn_addr)
+		if (ipv6_addr_any(&rs->rs_conn_addr))
 			return -ENOTCONN;
 
-		sin->sin_port = rs->rs_conn_port;
-		sin->sin_addr.s_addr = rs->rs_conn_addr;
+		if (ipv6_addr_v4mapped(&rs->rs_conn_addr)) {
+			sin = (struct sockaddr_in *)uaddr;
+			memset(sin->sin_zero, 0, sizeof(sin->sin_zero));
+			sin->sin_family = AF_INET;
+			sin->sin_port = rs->rs_conn_port;
+			sin->sin_addr.s_addr = rs->rs_conn_addr_v4;
+			uaddr_len = sizeof(*sin);
+		} else {
+			sin6 = (struct sockaddr_in6 *)uaddr;
+			sin6->sin6_family = AF_INET6;
+			sin6->sin6_port = rs->rs_conn_port;
+			sin6->sin6_addr = rs->rs_conn_addr;
+			sin6->sin6_flowinfo = 0;
+			/* scope_id is the same as in the bound address. */
+			sin6->sin6_scope_id = rs->rs_bound_scope_id;
+			uaddr_len = sizeof(*sin6);
+		}
 	} else {
-		sin->sin_port = rs->rs_bound_port;
-		sin->sin_addr.s_addr = rs->rs_bound_addr;
+		/* If socket is not yet bound, set the return address family
+		 * to be AF_UNSPEC (value 0) and the address size to be that
+		 * of an IPv4 address.
+		 */
+		if (ipv6_addr_any(&rs->rs_bound_addr)) {
+			sin = (struct sockaddr_in *)uaddr;
+			memset(sin, 0, sizeof(*sin));
+			sin->sin_family = AF_UNSPEC;
+			return sizeof(*sin);
+		}
+		if (ipv6_addr_v4mapped(&rs->rs_bound_addr)) {
+			sin = (struct sockaddr_in *)uaddr;
+			memset(sin->sin_zero, 0, sizeof(sin->sin_zero));
+			sin->sin_family = AF_INET;
+			sin->sin_port = rs->rs_bound_port;
+			sin->sin_addr.s_addr = rs->rs_bound_addr_v4;
+			uaddr_len = sizeof(*sin);
+		} else {
+			sin6 = (struct sockaddr_in6 *)uaddr;
+			sin6->sin6_family = AF_INET6;
+			sin6->sin6_port = rs->rs_bound_port;
+			sin6->sin6_addr = rs->rs_bound_addr;
+			sin6->sin6_flowinfo = 0;
+			sin6->sin6_scope_id = rs->rs_bound_scope_id;
+			uaddr_len = sizeof(*sin6);
+		}
 	}
 
-	sin->sin_family = AF_INET;
-
-	return sizeof(*sin);
+	return uaddr_len;
 }
 
 /*
@@ -203,11 +241,12 @@ static int rds_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 static int rds_cancel_sent_to(struct rds_sock *rs, char __user *optval,
 			      int len)
 {
+	struct sockaddr_in6 sin6;
 	struct sockaddr_in sin;
 	int ret = 0;
 
 	/* racing with another thread binding seems ok here */
-	if (rs->rs_bound_addr == 0) {
+	if (ipv6_addr_any(&rs->rs_bound_addr)) {
 		ret = -ENOTCONN; /* XXX not a great errno */
 		goto out;
 	}
@@ -215,14 +254,23 @@ static int rds_cancel_sent_to(struct rds_sock *rs, char __user *optval,
 	if (len < sizeof(struct sockaddr_in)) {
 		ret = -EINVAL;
 		goto out;
+	} else if (len < sizeof(struct sockaddr_in6)) {
+		/* Assume IPv4 */
+		if (copy_from_user(&sin, optval, sizeof(struct sockaddr_in))) {
+			ret = -EFAULT;
+			goto out;
+		}
+		ipv6_addr_set_v4mapped(sin.sin_addr.s_addr, &sin6.sin6_addr);
+		sin6.sin6_port = sin.sin_port;
+	} else {
+		if (copy_from_user(&sin6, optval,
+				   sizeof(struct sockaddr_in6))) {
+			ret = -EFAULT;
+			goto out;
+		}
 	}
 
-	if (copy_from_user(&sin, optval, sizeof(sin))) {
-		ret = -EFAULT;
-		goto out;
-	}
-
-	rds_send_drop_to(rs, &sin);
+	rds_send_drop_to(rs, &sin6);
 out:
 	return ret;
 }
@@ -435,31 +483,41 @@ static int rds_connect(struct socket *sock, struct sockaddr *uaddr,
 		       int addr_len, int flags)
 {
 	struct sock *sk = sock->sk;
-	struct sockaddr_in *sin = (struct sockaddr_in *)uaddr;
+	struct sockaddr_in *sin;
 	struct rds_sock *rs = rds_sk_to_rs(sk);
 	int ret = 0;
 
 	lock_sock(sk);
 
-	if (addr_len != sizeof(struct sockaddr_in)) {
-		ret = -EINVAL;
-		goto out;
-	}
+	switch (addr_len) {
+	case sizeof(struct sockaddr_in):
+		sin = (struct sockaddr_in *)uaddr;
+		if (sin->sin_family != AF_INET) {
+			ret = -EAFNOSUPPORT;
+			break;
+		}
+		if (sin->sin_addr.s_addr == htonl(INADDR_ANY)) {
+			ret = -EDESTADDRREQ;
+			break;
+		}
+		if (IN_MULTICAST(ntohl(sin->sin_addr.s_addr)) ||
+		    sin->sin_addr.s_addr == htonl(INADDR_BROADCAST)) {
+			ret = -EINVAL;
+			break;
+		}
+		ipv6_addr_set_v4mapped(sin->sin_addr.s_addr, &rs->rs_conn_addr);
+		rs->rs_conn_port = sin->sin_port;
+		break;
 
-	if (sin->sin_family != AF_INET) {
-		ret = -EAFNOSUPPORT;
-		goto out;
-	}
+	case sizeof(struct sockaddr_in6):
+		ret = -EPROTONOSUPPORT;
+		break;
 
-	if (sin->sin_addr.s_addr == htonl(INADDR_ANY)) {
-		ret = -EDESTADDRREQ;
-		goto out;
+	default:
+		ret = -EINVAL;
+		break;
 	}
 
-	rs->rs_conn_addr = sin->sin_addr.s_addr;
-	rs->rs_conn_port = sin->sin_port;
-
-out:
 	release_sock(sk);
 	return ret;
 }
@@ -578,8 +636,10 @@ static void rds_sock_inc_info(struct socket *sock, unsigned int len,
 		list_for_each_entry(inc, &rs->rs_recv_queue, i_item) {
 			total++;
 			if (total <= len)
-				rds_inc_info_copy(inc, iter, inc->i_saddr,
-						  rs->rs_bound_addr, 1);
+				rds_inc_info_copy(inc, iter,
+						  inc->i_saddr.s6_addr32[3],
+						  rs->rs_bound_addr_v4,
+						  1);
 		}
 
 		read_unlock(&rs->rs_recv_lock);
@@ -608,8 +668,8 @@ static void rds_sock_info(struct socket *sock, unsigned int len,
 	list_for_each_entry(rs, &rds_sock_list, rs_item) {
 		sinfo.sndbuf = rds_sk_sndbuf(rs);
 		sinfo.rcvbuf = rds_sk_rcvbuf(rs);
-		sinfo.bound_addr = rs->rs_bound_addr;
-		sinfo.connected_addr = rs->rs_conn_addr;
+		sinfo.bound_addr = rs->rs_bound_addr_v4;
+		sinfo.connected_addr = rs->rs_conn_addr_v4;
 		sinfo.bound_port = rs->rs_bound_port;
 		sinfo.connected_port = rs->rs_conn_port;
 		sinfo.inum = sock_i_ino(rds_rs_to_sk(rs));
net/rds/bind.c +63 -28

192 lines not on this page · raw message

net/rds/cong.c +13 -10
index 63da9d2..ccdff09 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007 Oracle.  All rights reserved.
+ * Copyright (c) 2007, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -101,7 +101,7 @@
 static DEFINE_SPINLOCK(rds_cong_lock);
 static struct rb_root rds_cong_tree = RB_ROOT;
 
-static struct rds_cong_map *rds_cong_tree_walk(__be32 addr,
+static struct rds_cong_map *rds_cong_tree_walk(const struct in6_addr *addr,
 					       struct rds_cong_map *insert)
 {
 	struct rb_node **p = &rds_cong_tree.rb_node;
@@ -109,12 +109,15 @@ static struct rds_cong_map *rds_cong_tree_walk(__be32 addr,
 	struct rds_cong_map *map;
 
 	while (*p) {
+		int diff;
+
 		parent = *p;
 		map = rb_entry(parent, struct rds_cong_map, m_rb_node);
 
-		if (addr < map->m_addr)
+		diff = rds_addr_cmp(addr, &map->m_addr);
+		if (diff < 0)
 			p = &(*p)->rb_left;
-		else if (addr > map->m_addr)
+		else if (diff > 0)
 			p = &(*p)->rb_right;
 		else
 			return map;
@@ -132,7 +135,7 @@ static struct rds_cong_map *rds_cong_tree_walk(__be32 addr,
  * these bitmaps in the process getting pointers to them.  The bitmaps are only
  * ever freed as the module is removed after all connections have been freed.
  */
-static struct rds_cong_map *rds_cong_from_addr(__be32 addr)
+static struct rds_cong_map *rds_cong_from_addr(const struct in6_addr *addr)
 {
 	struct rds_cong_map *map;
 	struct rds_cong_map *ret = NULL;
@@ -144,7 +147,7 @@ static struct rds_cong_map *rds_cong_from_addr(__be32 addr)
 	if (!map)
 		return NULL;
 
-	map->m_addr = addr;
+	map->m_addr = *addr;
 	init_waitqueue_head(&map->m_waitq);
 	INIT_LIST_HEAD(&map->m_conn_list);
 
@@ -171,7 +174,7 @@ static struct rds_cong_map *rds_cong_from_addr(__be32 addr)
 		kfree(map);
 	}
 
-	rdsdebug("map %p for addr %x\n", ret, be32_to_cpu(addr));
+	rdsdebug("map %p for addr %pI6c\n", ret, addr);
 
 	return ret;
 }
@@ -202,8 +205,8 @@ void rds_cong_remove_conn(struct rds_connection *conn)
 
 int rds_cong_get_maps(struct rds_connection *conn)
 {
-	conn->c_lcong = rds_cong_from_addr(conn->c_laddr);
-	conn->c_fcong = rds_cong_from_addr(conn->c_faddr);
+	conn->c_lcong = rds_cong_from_addr(&conn->c_laddr);
+	conn->c_fcong = rds_cong_from_addr(&conn->c_faddr);
 
 	if (!(conn->c_lcong && conn->c_fcong))
 		return -ENOMEM;
@@ -353,7 +356,7 @@ void rds_cong_remove_socket(struct rds_sock *rs)
 
 	/* update congestion map for now-closed port */
 	spin_lock_irqsave(&rds_cong_lock, flags);
-	map = rds_cong_tree_walk(rs->rs_bound_addr, NULL);
+	map = rds_cong_tree_walk(&rs->rs_bound_addr, NULL);
 	spin_unlock_irqrestore(&rds_cong_lock, flags);
 
 	if (map && rds_cong_test_bit(map, rs->rs_bound_port)) {
net/rds/connection.c +81 -51

255 lines not on this page · raw message

net/rds/ib.c +9 -8
index b6ad38e..c712a84 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -296,8 +296,8 @@ static int rds_ib_conn_info_visitor(struct rds_connection *conn,
 	if (conn->c_trans != &rds_ib_transport)
 		return 0;
 
-	iinfo->src_addr = conn->c_laddr;
-	iinfo->dst_addr = conn->c_faddr;
+	iinfo->src_addr = conn->c_laddr.s6_addr32[3];
+	iinfo->dst_addr = conn->c_faddr.s6_addr32[3];
 
 	memset(&iinfo->src_gid, 0, sizeof(iinfo->src_gid));
 	memset(&iinfo->dst_gid, 0, sizeof(iinfo->dst_gid));
@@ -341,7 +341,8 @@ static void rds_ib_ic_info(struct socket *sock, unsigned int len,
  * allowed to influence which paths have priority.  We could call userspace
  * asserting this policy "routing".
  */
-static int rds_ib_laddr_check(struct net *net, __be32 addr)
+static int rds_ib_laddr_check(struct net *net, const struct in6_addr *addr,
+			      __u32 scope_id)
 {
 	int ret;
 	struct rdma_cm_id *cm_id;
@@ -357,7 +358,7 @@ static int rds_ib_laddr_check(struct net *net, __be32 addr)
 
 	memset(&sin, 0, sizeof(sin));
 	sin.sin_family = AF_INET;
-	sin.sin_addr.s_addr = addr;
+	sin.sin_addr.s_addr = addr->s6_addr32[3];
 
 	/* rdma_bind_addr will only succeed for IB & iWARP devices */
 	ret = rdma_bind_addr(cm_id, (struct sockaddr *)&sin);
@@ -367,9 +368,9 @@ static int rds_ib_laddr_check(struct net *net, __be32 addr)
 	    cm_id->device->node_type != RDMA_NODE_IB_CA)
 		ret = -EADDRNOTAVAIL;
 
-	rdsdebug("addr %pI4 ret %d node type %d\n",
-		&addr, ret,
-		cm_id->device ? cm_id->device->node_type : -1);
+	rdsdebug("addr %pI6c ret %d node type %d\n",
+		 addr, ret,
+		 cm_id->device ? cm_id->device->node_type : -1);
 
 	rdma_destroy_id(cm_id);
 
net/rds/ib.h +34 -11

65 lines not on this page · raw message

net/rds/ib_cm.c +222 -78

459 lines not on this page · raw message

net/rds/ib_rdma.c +8 -7
index e678699..0ec9df0 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -100,18 +100,19 @@ static void rds_ib_remove_ipaddr(struct rds_ib_device *rds_ibdev, __be32 ipaddr)
 		kfree_rcu(to_free, rcu);
 }
 
-int rds_ib_update_ipaddr(struct rds_ib_device *rds_ibdev, __be32 ipaddr)
+int rds_ib_update_ipaddr(struct rds_ib_device *rds_ibdev,
+			 struct in6_addr *ipaddr)
 {
 	struct rds_ib_device *rds_ibdev_old;
 
-	rds_ibdev_old = rds_ib_get_device(ipaddr);
+	rds_ibdev_old = rds_ib_get_device(ipaddr->s6_addr32[3]);
 	if (!rds_ibdev_old)
-		return rds_ib_add_ipaddr(rds_ibdev, ipaddr);
+		return rds_ib_add_ipaddr(rds_ibdev, ipaddr->s6_addr32[3]);
 
 	if (rds_ibdev_old != rds_ibdev) {
-		rds_ib_remove_ipaddr(rds_ibdev_old, ipaddr);
+		rds_ib_remove_ipaddr(rds_ibdev_old, ipaddr->s6_addr32[3]);
 		rds_ib_dev_put(rds_ibdev_old);
-		return rds_ib_add_ipaddr(rds_ibdev, ipaddr);
+		return rds_ib_add_ipaddr(rds_ibdev, ipaddr->s6_addr32[3]);
 	}
 	rds_ib_dev_put(rds_ibdev_old);
 
@@ -544,7 +545,7 @@ void *rds_ib_get_mr(struct scatterlist *sg, unsigned long nents,
 	struct rds_ib_connection *ic = rs->rs_conn->c_transport_data;
 	int ret;
 
-	rds_ibdev = rds_ib_get_device(rs->rs_bound_addr);
+	rds_ibdev = rds_ib_get_device(rs->rs_bound_addr.s6_addr32[3]);
 	if (!rds_ibdev) {
 		ret = -ENODEV;
 		goto out;
net/rds/ib_recv.c +9 -9
index b4e421a..f101bed 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -266,7 +266,7 @@ static struct rds_ib_incoming *rds_ib_refill_one_inc(struct rds_ib_connection *i
 		rds_ib_stats_inc(s_ib_rx_total_incs);
 	}
 	INIT_LIST_HEAD(&ibinc->ii_frags);
-	rds_inc_init(&ibinc->ii_inc, ic->conn, ic->conn->c_faddr);
+	rds_inc_init(&ibinc->ii_inc, ic->conn, &ic->conn->c_faddr);
 
 	return ibinc;
 }
@@ -420,7 +420,7 @@ void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
 		ret = ib_post_recv(ic->i_cm_id->qp, &recv->r_wr, &failed_wr);
 		if (ret) {
 			rds_ib_conn_error(conn, "recv post on "
-			       "%pI4 returned %d, disconnecting and "
+			       "%pI6c returned %d, disconnecting and "
 			       "reconnecting\n", &conn->c_faddr,
 			       ret);
 			break;
@@ -850,7 +850,7 @@ static void rds_ib_process_recv(struct rds_connection *conn,
 
 	if (data_len < sizeof(struct rds_header)) {
 		rds_ib_conn_error(conn, "incoming message "
-		       "from %pI4 didn't include a "
+		       "from %pI6c didn't include a "
 		       "header, disconnecting and "
 		       "reconnecting\n",
 		       &conn->c_faddr);
@@ -863,7 +863,7 @@ static void rds_ib_process_recv(struct rds_connection *conn,
 	/* Validate the checksum. */
 	if (!rds_message_verify_checksum(ihdr)) {
 		rds_ib_conn_error(conn, "incoming message "
-		       "from %pI4 has corrupted header - "
+		       "from %pI6c has corrupted header - "
 		       "forcing a reconnect\n",
 		       &conn->c_faddr);
 		rds_stats_inc(s_recv_drop_bad_checksum);
@@ -943,10 +943,10 @@ static void rds_ib_process_recv(struct rds_connection *conn,
 		ic->i_recv_data_rem = 0;
 		ic->i_ibinc = NULL;
 
-		if (ibinc->ii_inc.i_hdr.h_flags == RDS_FLAG_CONG_BITMAP)
+		if (ibinc->ii_inc.i_hdr.h_flags == RDS_FLAG_CONG_BITMAP) {
 			rds_ib_cong_recv(conn, ibinc);
-		else {
-			rds_recv_incoming(conn, conn->c_faddr, conn->c_laddr,
+		} else {
+			rds_recv_incoming(conn, &conn->c_faddr, &conn->c_laddr,
 					  &ibinc->ii_inc, GFP_ATOMIC);
 			state->ack_next = be64_to_cpu(hdr->h_sequence);
 			state->ack_next_valid = 1;
@@ -990,7 +990,7 @@ void rds_ib_recv_cqe_handler(struct rds_ib_connection *ic,
 	} else {
 		/* We expect errors as the qp is drained during shutdown */
 		if (rds_conn_up(conn) || rds_conn_connecting(conn))
-			rds_ib_conn_error(conn, "recv completion on <%pI4,%pI4> had status %u (%s), disconnecting and reconnecting\n",
+			rds_ib_conn_error(conn, "recv completion on <%pI6c,%pI6c> had status %u (%s), disconnecting and reconnecting\n",
 					  &conn->c_laddr, &conn->c_faddr,
 					  wc->status,
 					  ib_wc_status_msg(wc->status));
net/rds/ib_send.c +5 -5
index 8557a1c..c4cdfe49 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -305,7 +305,7 @@ void rds_ib_send_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
 
 	/* We expect errors as the qp is drained during shutdown */
 	if (wc->status != IB_WC_SUCCESS && rds_conn_up(conn)) {
-		rds_ib_conn_error(conn, "send completion on <%pI4,%pI4> had status %u (%s), disconnecting and reconnecting\n",
+		rds_ib_conn_error(conn, "send completion on <%pI6c,%pI6c> had status %u (%s), disconnecting and reconnecting\n",
 				  &conn->c_laddr, &conn->c_faddr, wc->status,
 				  ib_wc_status_msg(wc->status));
 	}
@@ -730,7 +730,7 @@ int rds_ib_xmit(struct rds_connection *conn, struct rds_message *rm,
 		 first, &first->s_wr, ret, failed_wr);
 	BUG_ON(failed_wr != &first->s_wr);
 	if (ret) {
-		printk(KERN_WARNING "RDS/IB: ib_post_send to %pI4 "
+		printk(KERN_WARNING "RDS/IB: ib_post_send to %pI6c "
 		       "returned %d\n", &conn->c_faddr, ret);
 		rds_ib_ring_unalloc(&ic->i_send_ring, work_alloc);
 		rds_ib_sub_signaled(ic, nr_sig);
@@ -827,7 +827,7 @@ int rds_ib_xmit_atomic(struct rds_connection *conn, struct rm_atomic_op *op)
 		 send, &send->s_atomic_wr, ret, failed_wr);
 	BUG_ON(failed_wr != &send->s_atomic_wr.wr);
 	if (ret) {
-		printk(KERN_WARNING "RDS/IB: atomic ib_post_send to %pI4 "
+		printk(KERN_WARNING "RDS/IB: atomic ib_post_send to %pI6c "
 		       "returned %d\n", &conn->c_faddr, ret);
 		rds_ib_ring_unalloc(&ic->i_send_ring, work_alloc);
 		rds_ib_sub_signaled(ic, nr_sig);
@@ -967,7 +967,7 @@ int rds_ib_xmit_rdma(struct rds_connection *conn, struct rm_rdma_op *op)
 		 first, &first->s_rdma_wr.wr, ret, failed_wr);
 	BUG_ON(failed_wr != &first->s_rdma_wr.wr);
 	if (ret) {
-		printk(KERN_WARNING "RDS/IB: rdma ib_post_send to %pI4 "
+		printk(KERN_WARNING "RDS/IB: rdma ib_post_send to %pI6c "
 		       "returned %d\n", &conn->c_faddr, ret);
 		rds_ib_ring_unalloc(&ic->i_send_ring, work_alloc);
 		rds_ib_sub_signaled(ic, nr_sig);
net/rds/loop.c +4 -3
index dac6218..0dca895 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -33,6 +33,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/in.h>
+#include <linux/ipv6.h>
 
 #include "rds_single_path.h"
 #include "rds.h"
@@ -75,11 +76,11 @@ static int rds_loop_xmit(struct rds_connection *conn, struct rds_message *rm,
 
 	BUG_ON(hdr_off || sg || off);
 
-	rds_inc_init(&rm->m_inc, conn, conn->c_laddr);
+	rds_inc_init(&rm->m_inc, conn, &conn->c_laddr);
 	/* For the embedded inc. Matching put is in loop_inc_free() */
 	rds_message_addref(rm);
 
-	rds_recv_incoming(conn, conn->c_laddr, conn->c_faddr, &rm->m_inc,
+	rds_recv_incoming(conn, &conn->c_laddr, &conn->c_faddr, &rm->m_inc,
 			  GFP_KERNEL);
 
 	rds_send_drop_acked(conn, be64_to_cpu(rm->m_inc.i_hdr.h_sequence),
net/rds/rdma.c +3 -3
index 634cfcb..7b39980 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007 Oracle.  All rights reserved.
+ * Copyright (c) 2007, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -183,7 +183,7 @@ static int __rds_rdma_map(struct rds_sock *rs, struct rds_get_mr_args *args,
 	long i;
 	int ret;
 
-	if (rs->rs_bound_addr == 0 || !rs->rs_transport) {
+	if (ipv6_addr_any(&rs->rs_bound_addr) || !rs->rs_transport) {
 		ret = -ENOTCONN; /* XXX not a great errno */
 		goto out;
 	}
@@ -574,7 +574,7 @@ int rds_cmsg_rdma_args(struct rds_sock *rs, struct rds_message *rm,
 
 	args = CMSG_DATA(cmsg);
 
-	if (rs->rs_bound_addr == 0) {
+	if (ipv6_addr_any(&rs->rs_bound_addr)) {
 		ret = -ENOTCONN; /* XXX not a great errno */
 		goto out_ret;
 	}
net/rds/rdma_transport.c +40 -16

122 lines not on this page · raw message

net/rds/rds.h +46 -23

162 lines not on this page · raw message

net/rds/recv.c +34 -17

126 lines not on this page · raw message

net/rds/send.c +52 -15

125 lines not on this page · raw message

net/rds/tcp.c +29 -3
index 351a284..dadb337 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -37,6 +37,8 @@
 #include <net/tcp.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
+#include <net/tcp.h>
+#include <net/addrconf.h>
 
 #include "rds.h"
 #include "tcp.h"
@@ -262,9 +264,33 @@ static void rds_tcp_tc_info(struct socket *rds_sock, unsigned int len,
 	spin_unlock_irqrestore(&rds_tcp_tc_list_lock, flags);
 }
 
-static int rds_tcp_laddr_check(struct net *net, __be32 addr)
+static int rds_tcp_laddr_check(struct net *net, const struct in6_addr *addr,
+			       __u32 scope_id)
 {
-	if (inet_addr_type(net, addr) == RTN_LOCAL)
+	struct net_device *dev = NULL;
+	int ret;
+
+	if (ipv6_addr_v4mapped(addr)) {
+		if (inet_addr_type(net, addr->s6_addr32[3]) == RTN_LOCAL)
+			return 0;
+		return -EADDRNOTAVAIL;
+	}
+
+	/* If the scope_id is specified, check only those addresses
+	 * hosted on the specified interface.
+	 */
+	if (scope_id != 0) {
+		rcu_read_lock();
+		dev = dev_get_by_index_rcu(net, scope_id);
+		/* scope_id is not valid... */
+		if (!dev) {
+			rcu_read_unlock();
+			return -EADDRNOTAVAIL;
+		}
+		rcu_read_unlock();
+	}
+	ret = ipv6_chk_addr(net, addr, dev, 0);
+	if (ret)
 		return 0;
 	return -EADDRNOTAVAIL;
 }
net/rds/tcp_connect.c +20 -14
index d999e70..231ae92 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -66,7 +66,8 @@ void rds_tcp_state_change(struct sock *sk)
 		 * RDS connection as RDS_CONN_UP until the reconnect,
 		 * to avoid RDS datagram loss.
 		 */
-		if (!IS_CANONICAL(cp->cp_conn->c_laddr, cp->cp_conn->c_faddr) &&
+		if (rds_addr_cmp(&cp->cp_conn->c_laddr,
+				 &cp->cp_conn->c_faddr) >= 0 &&
 		    rds_conn_path_transition(cp, RDS_CONN_CONNECTING,
 					     RDS_CONN_ERROR)) {
 			rds_conn_path_drop(cp, false);
@@ -88,7 +89,9 @@ void rds_tcp_state_change(struct sock *sk)
 int rds_tcp_conn_path_connect(struct rds_conn_path *cp)
 {
 	struct socket *sock = NULL;
-	struct sockaddr_in src, dest;
+	struct sockaddr_in sin;
+	struct sockaddr *addr;
+	int addrlen;
 	int ret;
 	struct rds_connection *conn = cp->cp_conn;
 	struct rds_tcp_connection *tc = cp->cp_transport_data;
@@ -112,30 +115,33 @@ int rds_tcp_conn_path_connect(struct rds_conn_path *cp)
 
 	rds_tcp_tune(sock);
 
-	src.sin_family = AF_INET;
-	src.sin_addr.s_addr = (__force u32)conn->c_laddr;
-	src.sin_port = (__force u16)htons(0);
+	sin.sin_family = AF_INET;
+	sin.sin_addr.s_addr = conn->c_laddr.s6_addr32[3];
+	sin.sin_port = 0;
+	addr = (struct sockaddr *)&sin;
+	addrlen = sizeof(sin);
 
-	ret = sock->ops->bind(sock, (struct sockaddr *)&src, sizeof(src));
+	ret = sock->ops->bind(sock, addr, addrlen);
 	if (ret) {
-		rdsdebug("bind failed with %d at address %pI4\n",
+		rdsdebug("bind failed with %d at address %pI6c\n",
 			 ret, &conn->c_laddr);
 		goto out;
 	}
 
-	dest.sin_family = AF_INET;
-	dest.sin_addr.s_addr = (__force u32)conn->c_faddr;
-	dest.sin_port = (__force u16)htons(RDS_TCP_PORT);
+	sin.sin_family = AF_INET;
+	sin.sin_addr.s_addr = conn->c_faddr.s6_addr32[3];
+	sin.sin_port = htons(RDS_TCP_PORT);
+	addr = (struct sockaddr *)&sin;
+	addrlen = sizeof(sin);
 
 	/*
 	 * once we call connect() we can start getting callbacks and they
 	 * own the socket
 	 */
 	rds_tcp_set_callbacks(sock, cp);
-	ret = sock->ops->connect(sock, (struct sockaddr *)&dest, sizeof(dest),
-				 O_NONBLOCK);
+	ret = sock->ops->connect(sock, addr, addrlen, O_NONBLOCK);
 
-	rdsdebug("connect to address %pI4 returned %d\n", &conn->c_faddr, ret);
+	rdsdebug("connect to address %pI6c returned %d\n", &conn->c_faddr, ret);
 	if (ret == -EINPROGRESS)
 		ret = 0;
 	if (ret == 0) {
net/rds/tcp_listen.c +10 -8
index 2257118..4fdf5b3 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006, 2018 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2018 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -83,13 +83,12 @@ int rds_tcp_keepalive(struct socket *sock)
 struct rds_tcp_connection *rds_tcp_accept_one_path(struct rds_connection *conn)
 {
 	int i;
-	bool peer_is_smaller = IS_CANONICAL(conn->c_faddr, conn->c_laddr);
 	int npaths = max_t(int, 1, conn->c_npaths);
 
 	/* for mprds, all paths MUST be initiated by the peer
 	 * with the smaller address.
 	 */
-	if (!peer_is_smaller) {
+	if (rds_addr_cmp(&conn->c_faddr, &conn->c_laddr) >= 0) {
 		/* Make sure we initiate at least one path if this
 		 * has not already been done; rds_start_mprds() will
 		 * take care of additional paths, if necessary.
@@ -164,13 +163,16 @@ int rds_tcp_accept_one(struct socket *sock)
 
 	inet = inet_sk(new_sock->sk);
 
-	rdsdebug("accepted tcp %pI4:%u -> %pI4:%u\n",
-		 &inet->inet_saddr, ntohs(inet->inet_sport),
-		 &inet->inet_daddr, ntohs(inet->inet_dport));
+	rdsdebug("accepted tcp %pI6c:%u -> %pI6c:%u\n",
+		 &new_sock->sk->sk_v6_rcv_saddr, ntohs(inet->inet_sport),
+		 &new_sock->sk->sk_v6_daddr, ntohs(inet->inet_dport));
 
 	conn = rds_conn_create(sock_net(sock->sk),
-			       inet->inet_saddr, inet->inet_daddr,
-			       &rds_tcp_transport, GFP_KERNEL);
+			       &new_sock->sk->sk_v6_rcv_saddr,
+			       &new_sock->sk->sk_v6_daddr,
+			       &rds_tcp_transport, GFP_KERNEL,
+			       new_sock->sk->sk_bound_dev_if);
+
 	if (IS_ERR(conn)) {
 		ret = PTR_ERR(conn);
 		goto out;
net/rds/tcp_recv.c +5 -4
index b9fbd2e..42c5ff1 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -179,7 +179,7 @@ static int rds_tcp_data_recv(read_descriptor_t *desc, struct sk_buff *skb,
 			tc->t_tinc = tinc;
 			rdsdebug("alloced tinc %p\n", tinc);
 			rds_inc_path_init(&tinc->ti_inc, cp,
-					  cp->cp_conn->c_faddr);
+					  &cp->cp_conn->c_faddr);
 			tinc->ti_inc.i_rx_lat_trace[RDS_MSG_RX_HDR] =
 					local_clock();
 
@@ -239,8 +239,9 @@ static int rds_tcp_data_recv(read_descriptor_t *desc, struct sk_buff *skb,
 			if (tinc->ti_inc.i_hdr.h_flags == RDS_FLAG_CONG_BITMAP)
 				rds_tcp_cong_recv(conn, tinc);
 			else
-				rds_recv_incoming(conn, conn->c_faddr,
-						  conn->c_laddr, &tinc->ti_inc,
+				rds_recv_incoming(conn, &conn->c_faddr,
+						  &conn->c_laddr,
+						  &tinc->ti_inc,
 						  arg->gfp);
 
 			tc->t_tinc_hdr_rem = sizeof(struct rds_header);
net/rds/tcp_send.c +2 -2
index 7df869d..78a2554 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -153,7 +153,7 @@ int rds_tcp_xmit(struct rds_connection *conn, struct rds_message *rm,
 			 * an incoming RST.
 			 */
 			if (rds_conn_path_up(cp)) {
-				pr_warn("RDS/tcp: send to %pI4 on cp [%d]"
+				pr_warn("RDS/tcp: send to %pI6c on cp [%d]"
 					"returned %d, "
 					"disconnecting and reconnecting\n",
 					&conn->c_faddr, cp->cp_index, ret);
net/rds/threads.c +58 -11

108 lines not on this page · raw message

net/rds/transport.c +11 -4
index 0b188dd..c9788db 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -33,6 +33,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/in.h>
+#include <linux/ipv6.h>
 
 #include "rds.h"
 #include "loop.h"
@@ -75,20 +76,26 @@ void rds_trans_put(struct rds_transport *trans)
 		module_put(trans->t_owner);
 }
 
-struct rds_transport *rds_trans_get_preferred(struct net *net, __be32 addr)
+struct rds_transport *rds_trans_get_preferred(struct net *net,
+					      const struct in6_addr *addr,
+					      __u32 scope_id)
 {
 	struct rds_transport *ret = NULL;
 	struct rds_transport *trans;
 	unsigned int i;
 
-	if (IN_LOOPBACK(ntohl(addr)))
+	if (ipv6_addr_v4mapped(addr)) {
+		if (*(u_int8_t *)&addr->s6_addr32[3] == IN_LOOPBACKNET)
+			return &rds_loop_transport;
+	} else if (ipv6_addr_loopback(addr)) {
 		return &rds_loop_transport;
+	}
 
 	down_read(&rds_trans_sem);
 	for (i = 0; i < RDS_TRANS_COUNT; i++) {
 		trans = transports[i];
 
-		if (trans && (trans->laddr_check(net, addr) == 0) &&
+		if (trans && (trans->laddr_check(net, addr, scope_id) == 0) &&
 		    (!trans->t_owner || try_module_get(trans->t_owner))) {
 			ret = trans;
 			break;
Signature · 1 line
1.8.3.1
maintainer
#2 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

From: Ka-Cheong Poon <[email protected]>
Date: Wed, 27 Jun 2018 03:23:27 -0700

Quoted 10 lines

This patch changes the internal representation of an IP address to use struct in6_addr. IPv4 address is stored as an IPv4 mapped address. All the functions which take an IP address as argument are also changed to use struct in6_addr. But RDS socket layer is not modified such that it still does not accept IPv6 address from an application. And RDS layer does not accept nor initiate IPv6 connections.

v2: Fixed sparse warnings.

I really don't like this.

An ipv4 mapped ipv6 address is not the same as an ipv4 address.

You are effectively preventing the use of ipv6 connections using ipv4 mapped addresses.

Also, assuming the sockaddr type based upon size is wrong. You have to check the family field, then you can decide to interpret the rest of the sockaddr in one way or another and also validate it's length.

#3 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

Quoted 12 lines · David Miller

From: Ka-Cheong Poon <[email protected]>
Date: Wed, 27 Jun 2018 03:23:27 -0700

Quoted 9 lines

This patch changes the internal representation of an IP address to use struct in6_addr. IPv4 address is stored as an IPv4 mapped address. All the functions which take an IP address as argument are also changed to use struct in6_addr. But RDS socket layer is not modified such that it still does not accept IPv6 address from an application. And RDS layer does not accept nor initiate IPv6 connections.

v2: Fixed sparse warnings.

Quoted 8 lines
Quoted 1 line

I really don't like this.

An ipv4 mapped ipv6 address is not the same as an ipv4 address.

You are effectively preventing the use of ipv6 connections using ipv4 mapped addresses.

Could you please clarify what is meant by an IPv6 connections using IPv4 mapped address? An IPv6 packet cannot use an IPv4 mapped address as source or destination address. Do you mean an app uses an IPv4 mapped address in a struct sockaddr_in6 to set up an IPv4 connection?

Please note that this patch is patch #1. This patch alone does not support RDS/IPv6. Hence this patch has checks to prevent an app to use IPv6 address. Those checks will be removed in patch #2.

Quoted 5 lines

Also, assuming the sockaddr type based upon size is wrong. You have to check the family field, then you can decide to interpret the rest of the sockaddr in one way or another and also validate it's length.

Signature · 2 lines
K. Poon
[email protected]
#4 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

In reply to Ka-Cheong Poon
Quoted 57 lines · Ka-Cheong Poon

This patch changes the internal representation of an IP address to use struct in6_addr. IPv4 address is stored as an IPv4 mapped address. All the functions which take an IP address as argument are also changed to use struct in6_addr. But RDS socket layer is not modified such that it still does not accept IPv6 address from an application. And RDS layer does not accept nor initiate IPv6 connections.

v2: Fixed sparse warnings.

---
  net/rds/af_rds.c         | 138 ++++++++++++++++------
  net/rds/bind.c           |  91 +++++++++-----
  net/rds/cong.c           |  23 ++--
  net/rds/connection.c     | 132 +++++++++++++--------
  net/rds/ib.c             |  17 +--
  net/rds/ib.h             |  45 +++++--
  net/rds/ib_cm.c          | 300 +++++++++++++++++++++++++++++++++++------------
  net/rds/ib_rdma.c        |  15 +--
  net/rds/ib_recv.c        |  18 +--
  net/rds/ib_send.c        |  10 +-
  net/rds/loop.c           |   7 +-
  net/rds/rdma.c           |   6 +-
  net/rds/rdma_transport.c |  56 ++++++---
  net/rds/rds.h            |  69 +++++++----
  net/rds/recv.c           |  51 +++++---
  net/rds/send.c           |  67 ++++++++---
  net/rds/tcp.c            |  32 ++++-
  net/rds/tcp_connect.c    |  34 +++---
  net/rds/tcp_listen.c     |  18 +--
  net/rds/tcp_recv.c       |   9 +-
  net/rds/tcp_send.c       |   4 +-
  net/rds/threads.c        |  69 +++++++++--
  net/rds/transport.c      |  15 ++-
  23 files changed, 857 insertions(+), 369 deletions(-)
net/rds/bind.c +2 -1
index 5aa3a64..3822886 100644
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2018 Oracle and/or its affiliates. All rights reserved.
   *
   * This software is available to you under a choice of one of two
   * licenses.  You may choose to be licensed under the terms of the GNU
@@ -33,6 +33,7 @@
  #include <linux/kernel.h>
  #include <net/sock.h>
  #include <linux/in.h>
+#include <linux/ipv6.h>
  #include <linux/if_arp.h>
  #include <linux/jhash.h>
  #include <linux/ratelimit.h>

net/rds/bind.c line 46

Quoted 5 lines
net/rds/bind.c +1 -1
@@ -42,42 +43,58 @@
 
  static const struct rhashtable_params ht_parms = {
  	.nelem_hint = 768,
-	.key_len = sizeof(u64),
+	.key_len = RDS_BOUND_KEY_LEN,

Do we really need the scope id to be part of the key ? With link local/global, do you see any collisions. Please educate me on the actual usecase. This can avoid bunch of changes and hence the question.

net/rds/bind.c static int rds_add_bound(struct rds_sock *rs, __be32 addr, __be16 *port) · line 135

Quoted 6 lines
net/rds/bind.c +1 -1
@@ -114,7 +132,7 @@ static int rds_add_bound(struct rds_sock *rs, __be32 addr, __be16 *port)
  			  rs, &addr, (int)ntohs(*port));
  			break;
  		} else {
-			rs->rs_bound_addr = 0;
+			rs->rs_bound_addr = in6addr_any;

Can you elaborate why 0 is not ok ?

Quoted 24 lines
net/rds/bind.c +3 -3
  			rds_sock_put(rs);
  			ret = -ENOMEM;
  			break;
@@ -127,44 +145,61 @@ static int rds_add_bound(struct rds_sock *rs, __be32 addr, __be16 *port)
  void rds_remove_bound(struct rds_sock *rs)
  {
 
-	if (!rs->rs_bound_addr)
+	if (ipv6_addr_any(&rs->rs_bound_addr))
  		return;
 
-	rdsdebug("rs %p unbinding from %pI4:%d\n",
+	rdsdebug("rs %p unbinding from %pI6c:%d\n",
  		 rs, &rs->rs_bound_addr,
  		 ntohs(rs->rs_bound_port));
 
  	rhashtable_remove_fast(&bind_hash_table, &rs->rs_bound_node, ht_parms);
  	rds_sock_put(rs);
-	rs->rs_bound_addr = 0;
+	rs->rs_bound_addr = in6addr_any;
  }
 
  int rds_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
  {

net/rds/bind.c static int rds_add_bound(struct rds_sock *rs, __be32 addr, __be16 *port) · line 170

Quoted 10 lines
net/rds/bind.c +4 -1
  	struct sock *sk = sock->sk;
-	struct sockaddr_in *sin = (struct sockaddr_in *)uaddr;
  	struct rds_sock *rs = rds_sk_to_rs(sk);
+	struct in6_addr v6addr, *binding_addr;
  	struct rds_transport *trans;
+	__u32 scope_id = 0;
  	int ret = 0;
+	__be16 port;
 
+	/* We only allow an RDS socket to be bound to and IPv4 address. IPv6

s/'bound to and IPv4'/'bound to an IPv4'

Quoted 17 lines
net/rds/bind.c +16 -0
+	 * address support will be added later.
+	 */
+	if (addr_len == sizeof(struct sockaddr_in)) {
+		struct sockaddr_in *sin = (struct sockaddr_in *)uaddr;
+
+		if (sin->sin_family != AF_INET ||
+		    sin->sin_addr.s_addr == htonl(INADDR_ANY))
+			return -EINVAL;
+		ipv6_addr_set_v4mapped(sin->sin_addr.s_addr, &v6addr);
+		binding_addr = &v6addr;
+		port = sin->sin_port;
+	} else if (addr_len == sizeof(struct sockaddr_in6)) {
+		return -EPROTONOSUPPORT;
+	} else {
+		return -EINVAL;
+	}
  	lock_sock(sk);

net/rds/bind.c static int rds_add_bound(struct rds_sock *rs, __be32 addr, __be16 *port) · line 193

Quoted 10 lines
net/rds/bind.c +2 -4
 
-	if (addr_len != sizeof(struct sockaddr_in) ||
-	    sin->sin_family != AF_INET ||
-	    rs->rs_bound_addr ||
-	    sin->sin_addr.s_addr == htonl(INADDR_ANY)) {
+	/* RDS socket does not allow re-binding. */
+	if (!ipv6_addr_any(&rs->rs_bound_addr)) {
  		ret = -EINVAL;
  		goto out;
  	}

Seems new behavior you are adding. The statement itself is true but if we return silently for already bind address, its ok. May be am missing something above.

Quoted 8 lines
net/rds/connection.c +0 -0
index abef75d..ca72563 100644
@@ -142,9 +151,12 @@ static void __rds_conn_path_init(struct rds_connection *conn,
   * are torn down as the module is removed, if ever.
   */

net/rds/connection.c static void __rds_conn_path_init(struct rds_connection *conn, · line 159

Quoted 10 lines
net/rds/connection.c +6 -3
  static struct rds_connection *__rds_conn_create(struct net *net,
-						__be32 laddr, __be32 faddr,
-				       struct rds_transport *trans, gfp_t gfp,
-				       int is_outgoing)
+						const struct in6_addr *laddr,
+						const struct in6_addr *faddr,
+						struct rds_transport *trans,
+						gfp_t gfp,
+						int is_outgoing,
+						int dev_if)

Am just wondering if we can handle local link address case differently instead of pushing the interface index everywhere. Did you think about any alternative. This can also avoid bunch of changes again and hence the question. Am trying to see if we can minimize the changes to absolute must have to support IPv6.

Quoted 93 lines
net/rds/ib.c +9 -8
index b6ad38e..c712a84 100644
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
   *
   * This software is available to you under a choice of one of two
   * licenses.  You may choose to be licensed under the terms of the GNU
@@ -296,8 +296,8 @@ static int rds_ib_conn_info_visitor(struct rds_connection *conn,
  	if (conn->c_trans != &rds_ib_transport)
  		return 0;
 
-	iinfo->src_addr = conn->c_laddr;
-	iinfo->dst_addr = conn->c_faddr;
+	iinfo->src_addr = conn->c_laddr.s6_addr32[3];
+	iinfo->dst_addr = conn->c_faddr.s6_addr32[3];
 
  	memset(&iinfo->src_gid, 0, sizeof(iinfo->src_gid));
  	memset(&iinfo->dst_gid, 0, sizeof(iinfo->dst_gid));
@@ -341,7 +341,8 @@ static void rds_ib_ic_info(struct socket *sock, unsigned int len,
   * allowed to influence which paths have priority.  We could call userspace
   * asserting this policy "routing".
   */
-static int rds_ib_laddr_check(struct net *net, __be32 addr)
+static int rds_ib_laddr_check(struct net *net, const struct in6_addr *addr,
+			      __u32 scope_id)
  {
  	int ret;
  	struct rdma_cm_id *cm_id;
@@ -357,7 +358,7 @@ static int rds_ib_laddr_check(struct net *net, __be32 addr)
 
  	memset(&sin, 0, sizeof(sin));
  	sin.sin_family = AF_INET;
-	sin.sin_addr.s_addr = addr;
+	sin.sin_addr.s_addr = addr->s6_addr32[3];
 
  	/* rdma_bind_addr will only succeed for IB & iWARP devices */
  	ret = rdma_bind_addr(cm_id, (struct sockaddr *)&sin);
@@ -367,9 +368,9 @@ static int rds_ib_laddr_check(struct net *net, __be32 addr)
  	    cm_id->device->node_type != RDMA_NODE_IB_CA)
  		ret = -EADDRNOTAVAIL;
 
-	rdsdebug("addr %pI4 ret %d node type %d\n",
-		&addr, ret,
-		cm_id->device ? cm_id->device->node_type : -1);
+	rdsdebug("addr %pI6c ret %d node type %d\n",
+		 addr, ret,
+		 cm_id->device ? cm_id->device->node_type : -1);
 
  	rdma_destroy_id(cm_id);
 
net/rds/ib.h +21 -8
index a6f4d7d..12f96b3 100644
@@ -57,16 +57,38 @@ struct rds_ib_refill_cache {
  	struct list_head	 *ready;
  };
 
+struct rds_ib_conn_priv_cmn {
+	u8			ricpc_protocol_major;
+	u8			ricpc_protocol_minor;
+	__be16			ricpc_protocol_minor_mask;	/* bitmask */
+	__be32			ricpc_reserved1;
+	__be64			ricpc_ack_seq;
+	__be32			ricpc_credit;	/* non-zero enables flow ctl */
+};
+
  struct rds_ib_connect_private {
  	/* Add new fields at the end, and don't permute existing fields. */
-	__be32			dp_saddr;
-	__be32			dp_daddr;
-	u8			dp_protocol_major;
-	u8			dp_protocol_minor;
-	__be16			dp_protocol_minor_mask; /* bitmask */
-	__be32			dp_reserved1;
-	__be64			dp_ack_seq;
-	__be32			dp_credit;		/* non-zero enables flow ctl */
+	__be32				dp_saddr;
+	__be32				dp_daddr;
+	struct rds_ib_conn_priv_cmn	dp_cmn;
+};
+
+struct rds6_ib_connect_private {
+	/* Add new fields at the end, and don't permute existing fields. */
+	struct in6_addr			dp_saddr;
+	struct in6_addr			dp_daddr;
+	struct rds_ib_conn_priv_cmn	dp_cmn;
+};
+

net/rds/ib.h struct rds_ib_refill_cache { · line 92

Quoted 10 lines
net/rds/ib.h +9 -0
+#define dp_protocol_major	dp_cmn.ricpc_protocol_major
+#define dp_protocol_minor	dp_cmn.ricpc_protocol_minor
+#define dp_protocol_minor_mask	dp_cmn.ricpc_protocol_minor_mask
+#define dp_ack_seq		dp_cmn.ricpc_ack_seq
+#define dp_credit		dp_cmn.ricpc_credit
+
+union rds_ib_conn_priv {
+	struct rds_ib_connect_private	ricp_v4;
+	struct rds6_ib_connect_private	ricp_v6;
  };

This change was invetiable. Just add a comment saying the priviate data size for v6 vs v4 is different. Also for IPv6 priviate data, I suggest add some resrve fields for any future extensions so that things can be added without breaking wire protocol. With IPv4, we ended up in bit of mess.

Quoted 10 lines
net/rds/ib_cm.c +1 -1
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2018 Oracle and/or its affiliates. All rights reserved.
   *
   * This software is available to you under a choice of one of two
   * licenses.  You may choose to be licensed under the terms of the GNU

net/rds/ib_cm.c line 44

Quoted 10 lines
net/rds/ib_cm.c +2 -0
@@ -35,10 +35,12 @@
  #include <linux/slab.h>
  #include <linux/vmalloc.h>
  #include <linux/ratelimit.h>
+#include <net/addrconf.h>
 
  #include "rds_single_path.h"
  #include "rds.h"
  #include "ib.h"
+#include "tcp.h"
 

Hmm. Why do you need to include tcp header in ib transport code ? If there is any common function just move to core common file and use it.

net/rds/rdma_transport.c static int rds_rdma_listen_init(void) · line 205

Quoted 12 lines
net/rds/rdma_transport.c +6 -0
index fc59821..aef73e7 100644
 
 
+/* Initialize the RDS RDMA listeners.  We create two listeners for
+ * compatibility reason.  The one on RDS_PORT is used for IPv4
+ * requests only.  The one on RDS_TCP_PORT is used for IPv6 requests
+ * only.  So only IPv6 enabled RDS module will communicate using this
+ * port.
+ */

You did above ti facilitate both v4 and v6 connections to co-exist together ? Even though potentially there is no practical usecase, its nice to have this possibility.

Quoted 10 lines
net/rds/rds.h +1 -0
index f2272fb..859808a 100644
@@ -10,6 +10,7 @@
  #include <linux/rds.h>
  #include <linux/rhashtable.h>
  #include <linux/refcount.h>
+#include <linux/in6.h>
 

[...]

net/rds/rds.h struct rds_transport { · line 526

Quoted 7 lines
net/rds/rds.h +2 -1
@@ -519,7 +522,8 @@ struct rds_transport {
  				t_mp_capable:1;
  	unsigned int		t_type;
 
-	int (*laddr_check)(struct net *net, __be32 addr);
+	int (*laddr_check)(struct net *net, const struct in6_addr *addr,
+			   __u32 scope_id);

I already asked somehwre above if we can avoid scope_id change.

Quoted 59 lines
net/rds/send.c +11 -7
index 94c7f74..6ed2e92 100644
@@ -709,7 +709,7 @@ void rds_send_drop_acked(struct rds_connection *conn, u64 ack,
  }
  EXPORT_SYMBOL_GPL(rds_send_drop_acked);
 
-void rds_send_drop_to(struct rds_sock *rs, struct sockaddr_in *dest)
+void rds_send_drop_to(struct rds_sock *rs, struct sockaddr_in6 *dest)
  {
  	struct rds_message *rm, *tmp;
  	struct rds_connection *conn;
@@ -721,8 +721,9 @@ void rds_send_drop_to(struct rds_sock *rs, struct sockaddr_in *dest)
  	spin_lock_irqsave(&rs->rs_lock, flags);
 
  	list_for_each_entry_safe(rm, tmp, &rs->rs_send_queue, m_sock_item) {
-		if (dest && (dest->sin_addr.s_addr != rm->m_daddr ||
-			     dest->sin_port != rm->m_inc.i_hdr.h_dport))
+		if (dest &&
+		    (!ipv6_addr_equal(&dest->sin6_addr, &rm->m_daddr) ||
+		     dest->sin6_port != rm->m_inc.i_hdr.h_dport))
  			continue;
 
  		list_move(&rm->m_sock_item, &list);
@@ -1059,8 +1060,8 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
  {
  	struct sock *sk = sock->sk;
  	struct rds_sock *rs = rds_sk_to_rs(sk);
+	DECLARE_SOCKADDR(struct sockaddr_in6 *, sin6, msg->msg_name);
  	DECLARE_SOCKADDR(struct sockaddr_in *, usin, msg->msg_name);
-	__be32 daddr;
  	__be16 dport;
  	struct rds_message *rm = NULL;
  	struct rds_connection *conn;
@@ -1069,10 +1070,13 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
  	int nonblock = msg->msg_flags & MSG_DONTWAIT;
  	long timeo = sock_sndtimeo(sk, nonblock);
  	struct rds_conn_path *cpath;
+	struct in6_addr daddr;
+	__u32 scope_id = 0;
  	size_t total_payload_len = payload_len, rdma_payload_len = 0;
  	bool zcopy = ((msg->msg_flags & MSG_ZEROCOPY) &&
  		      sock_flag(rds_rs_to_sk(rs), SOCK_ZEROCOPY));
  	int num_sgs = ceil(payload_len, PAGE_SIZE);
+	int namelen;
 
  	/* Mirror Linux UDP mirror of BSD error message compatibility */
  	/* XXX: Perhaps MSG_MORE someday */
@@ -1081,27 +1085,59 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
  		goto out;
  	}
 
-	if (msg->msg_namelen) {
-		/* XXX fail non-unicast destination IPs? */
-		if (msg->msg_namelen < sizeof(*usin) || usin->sin_family != AF_INET) {
+	namelen = msg->msg_namelen;
+	if (namelen != 0) {
+		if (namelen < sizeof(*usin)) {

net/rds/send.c int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len) · line 1100

Quoted 10 lines
net/rds/send.c +10 -0
+			ret = -EINVAL;
+			goto out;
+		}
+		switch (namelen) {
+		case sizeof(*usin):
+			if (usin->sin_family != AF_INET ||
+			    usin->sin_addr.s_addr == htonl(INADDR_ANY) ||
+			    usin->sin_addr.s_addr == htonl(INADDR_BROADCAST) ||
+			    IN_MULTICAST(ntohl(usin->sin_addr.s_addr))) {
+				ret = -EINVAL;

The idea was to fail non-unicast IP someday but can you not add this change as part of v6 series. We can look at it later unless you need this change for v6. Again the question is mainly to add only necessary check and leave the existing behavior as is so please re-check all below checks too.

net/rds/transport.c

Quoted 6 lines
net/rds/transport.c +1 -0
index 0b188dd..c9788db 100644
 
+	if (ipv6_addr_v4mapped(addr)) {

Dave already commented on ipv6_addr_v4mapped(). Apart from some of those comments questions, rest of the patch looks really good and nicely done. Will also look at your subsequent two patches and try to send you comments in next few days.

Thanks for getting v6 support going Ka-Cheong !!

Regards,
Santosh

#5 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

Quoted 12 lines · Santosh Shilimkar

net/rds/bind.c line 45

Quoted 8 lines
net/rds/bind.c +1 -1
@@ -42,42 +43,58 @@
   static const struct rhashtable_params ht_parms = {
       .nelem_hint = 768,
-    .key_len = sizeof(u64),
+    .key_len = RDS_BOUND_KEY_LEN,

Do we really need the scope id to be part of the key ? With link local/global, do you see any collisions. Please educate me on the actual usecase. This can avoid bunch of changes and hence the question.

Yes, because link local address is not unique. The same address can be in two different links. The scope ID is used to differentiate that.

Quoted 9 lines

net/rds/bind.c static int rds_add_bound(struct rds_sock *rs, · line 136

Quoted 8 lines
net/rds/bind.c +1 -1
@@ -114,7 +132,7 @@ static int rds_add_bound(struct rds_sock *rs, 
 __be32 addr, __be16 *port)
                 rs, &addr, (int)ntohs(*port));
               break;
           } else {
-            rs->rs_bound_addr = 0;
+            rs->rs_bound_addr = in6addr_any;

Can you elaborate why 0 is not ok ?

The type of rs_bound_addr is struct in6_addr. So 0 cannot be used.

Quoted 11 lines
Quoted 11 lines
net/rds/bind.c +0 -0
   int rds_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
   {

      struct sock *sk = sock->sk;
-    struct sockaddr_in *sin = (struct sockaddr_in *)uaddr; struct rds_sock *rs = rds_sk_to_rs(sk);
+    struct in6_addr v6addr, *binding_addr;
      struct rds_transport *trans;
+    __u32 scope_id = 0;
      int ret = 0;
+    __be16 port;
+    /* We only allow an RDS socket to be bound to and IPv4 address. IPv6

Quoted 1 line

s/'bound to and IPv4'/'bound to an IPv4'

Changed. But this comment will be removed in patch #2 anyway.

Quoted 26 lines
Quoted 18 lines

+     * address support will be added later.
+     */
+    if (addr_len == sizeof(struct sockaddr_in)) {
+        struct sockaddr_in *sin = (struct sockaddr_in *)uaddr; +
+        if (sin->sin_family != AF_INET ||
+            sin->sin_addr.s_addr == htonl(INADDR_ANY)) +            return -EINVAL;
+        ipv6_addr_set_v4mapped(sin->sin_addr.s_addr, &v6addr); +        binding_addr = &v6addr;
+        port = sin->sin_port;
+    } else if (addr_len == sizeof(struct sockaddr_in6)) { +        return -EPROTONOSUPPORT;
+    } else {
+        return -EINVAL;
+    }
      lock_sock(sk);
-    if (addr_len != sizeof(struct sockaddr_in) ||

Quoted 8 lines

-        sin->sin_family != AF_INET ||
-        rs->rs_bound_addr ||
-        sin->sin_addr.s_addr == htonl(INADDR_ANY)) { +    /* RDS socket does not allow re-binding. */ +    if (!ipv6_addr_any(&rs->rs_bound_addr)) { ret = -EINVAL;
          goto out;
      }

Quoted 3 lines

Seems new behavior you are adding. The statement itself is true but if we return silently for already bind address, its ok. May be am missing something above.

This is the existing behavior. The old code checks if rs_bound_addr is non-zero or not. If it is non-zero, it returns an error.

Quoted 16 lines
Quoted 16 lines
net/rds/connection.c +3 -3
index abef75d..ca72563 100644
@@ -142,9 +151,12 @@ static void __rds_conn_path_init(struct 
 rds_connection *conn,
    * are torn down as the module is removed, if ever.
    */
   static struct rds_connection *__rds_conn_create(struct net *net,
-                        __be32 laddr, __be32 faddr,
-                       struct rds_transport *trans, gfp_t gfp,
-                       int is_outgoing)
+                        const struct in6_addr *laddr,
+                        const struct in6_addr *faddr,
+                        struct rds_transport *trans,
Quoted 8 lines

net/rds/connection.c static void __rds_conn_path_init(struct rds_connection *conn, · line 159

Quoted 3 lines
net/rds/connection.c +3 -0
+                        gfp_t gfp,
+                        int is_outgoing,
+                        int dev_if)

Am just wondering if we can handle local link address case differently instead of pushing the interface index everywhere. Did you think about any alternative. This can also avoid bunch of changes again and hence the question. Am trying to see if we can minimize the changes to absolute must have to support IPv6.

If link local address is supported, scoped ID must be used. Unless we remove the support of link local address, we need to keep scope ID.

Quoted 9 lines
Quoted 9 lines
net/rds/ib.h +3 -0
index a6f4d7d..12f96b3 100644
 
+union rds_ib_conn_priv {
+    struct rds_ib_connect_private    ricp_v4;
+    struct rds6_ib_connect_private    ricp_v6;

  };

Quoted 5 lines

This change was invetiable. Just add a comment saying the priviate data size for v6 vs v4 is  different. Also for IPv6 priviate data, I suggest add some resrve fields for any future extensions so that things can be added without breaking wire protocol. With IPv4, we ended up in bit of mess.

Will add some comments. Unfortunately the IB private data exchange has only a limited space. I don't think there is any more space left for reserved field. I think we should think of a different way to handle extensions in future.

Quoted 15 lines
Quoted 15 lines
net/rds/ib_cm.c +1 -1
@@ -1,5 +1,5 @@
   /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2018 Oracle and/or its affiliates. All rights 

reserved.
   *
   * This software is available to you under a choice of one of two
   * licenses.  You may choose to be licensed under the terms of the GNU

net/rds/ib_cm.c +1 -0
@@ -35,10 +35,12 @@
   #include <linux/slab.h>
   #include <linux/vmalloc.h>
   #include <linux/ratelimit.h>
+#include <net/addrconf.h>
Quoted 8 lines

net/rds/ib_cm.c line 44

Quoted 5 lines
net/rds/ib_cm.c +1 -0
   #include "rds_single_path.h"
   #include "rds.h"
   #include "ib.h"
+#include "tcp.h"
 

Hmm. Why do you need to include tcp header in ib transport code ? If there is any common function just move to core common file and use it.

I think it can be removed as it is left over from earlier changes when the IB IPv6 listener port was RDS_TCP_PORT. Now all the port definitions are in rds.h.

Quoted 11 lines

net/rds/rdma_transport.c static int rds_rdma_listen_init(void) · line 205

Quoted 11 lines
net/rds/rdma_transport.c +6 -0
index fc59821..aef73e7 100644
 
+/* Initialize the RDS RDMA listeners.  We create two listeners for
+ * compatibility reason.  The one on RDS_PORT is used for IPv4
+ * requests only.  The one on RDS_TCP_PORT is used for IPv6 requests
+ * only.  So only IPv6 enabled RDS module will communicate using this
+ * port.
+ */
Quoted 3 lines

You did above ti facilitate both v4 and v6 connections to co-exist together ? Even though potentially there is no practical usecase, its nice to have this possibility.

Yes, as there is no way to differentiate the IB private data exchange. So another port is used for IPv6 IB private exchange. Doing this allow both IPv4 and IPv6 connections to coexist.

Quoted 23 lines
Quoted 23 lines
net/rds/send.c +10 -3
index 94c7f74..6ed2e92 100644
@@ -1081,27 +1085,59 @@ int rds_sendmsg(struct socket *sock, struct 
 msghdr *msg, size_t payload_len)
           goto out;
       }
-    if (msg->msg_namelen) {
-        /* XXX fail non-unicast destination IPs? */
-        if (msg->msg_namelen < sizeof(*usin) || usin->sin_family != 
 AF_INET) {
+    namelen = msg->msg_namelen;
+    if (namelen != 0) {
+        if (namelen < sizeof(*usin)) {
+            ret = -EINVAL;
+            goto out;
+        }
+        switch (namelen) {
+        case sizeof(*usin):
+            if (usin->sin_family != AF_INET ||
+                usin->sin_addr.s_addr == htonl(INADDR_ANY) ||
Quoted 8 lines

net/rds/send.c int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len) · line 1100

Quoted 3 lines
net/rds/send.c +3 -0
+                usin->sin_addr.s_addr == htonl(INADDR_BROADCAST) ||
+                IN_MULTICAST(ntohl(usin->sin_addr.s_addr))) {
+                ret = -EINVAL;

The idea was to fail non-unicast IP someday but can you not add this change as part of v6 series. We can look at it later unless you need this change for v6. Again the question is mainly to add only necessary check and leave the existing behavior as is so please re-check all below checks too.

This is to match the same IPv6 check. In IPv6 code, it checks if the address is a unicast address. I can remove the IPv4 checks but if an app does send to a multicast address, the connection will be stuck forever.

Quoted 5 lines
Quoted 4 lines
net/rds/transport.c +0 -0
index 0b188dd..c9788db 100644
Quoted 7 lines
Quoted 1 line

+    if (ipv6_addr_v4mapped(addr)) {

Dave already commented on ipv6_addr_v4mapped(). Apart from some of those comments questions, rest of the patch looks really good and nicely done. Will also look at your subsequent two patches and try to send you comments in next few days.

Do you mean using mapped address to create IPv4 connections? I already have it in my work space. Will be in v3.

Signature · 2 lines
K. Poon
[email protected]
#6 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

Quoted 7 lines · Ka-Cheong Poon
Quoted 3 lines

Hmm. Why do you need to include tcp header in ib transport code ? If there is any common function just move to core common file and use it.

I think it can be removed as it is left over from earlier changes when the IB IPv6 listener port was RDS_TCP_PORT. Now all the port definitions are in rds.h.

Transport specific information such as port numbers should not be smeared into the common rds-module. Please fix that.

If IB is also piggybacking on port 16385 (why?), please use your own definitions for it in IB specific header files.

Santosh, David, I have to NACK this if it is not changed.

--Sowmini

#7 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

Quoted 9 lines · Sowmini Varadhan
Quoted 7 lines · Ka-Cheong Poon
Quoted 3 lines

Hmm. Why do you need to include tcp header in ib transport code ? If there is any common function just move to core common file and use it.

I think it can be removed as it is left over from earlier changes when the IB IPv6 listener port was RDS_TCP_PORT. Now all the port definitions are in rds.h.

Quoted 5 lines

Transport specific information such as port numbers should not be smeared into the common rds-module. Please fix that.

If IB is also piggybacking on port 16385 (why?), please use your own definitions for it in IB specific header files.

As mentioned in a previous mail, it is unclear why the port number is transport specific. Most Internet services use the same port number running over TCP/UDP as shown in the IANA database. And the IANA RDS registration is the same. What is the rationale of having a transport specific number in the RDS implementation?

Signature · 2 lines
K. Poon
[email protected]
#8 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

Quoted 7 lines · Ka-Cheong Poon

As mentioned in a previous mail, it is unclear why the port number is transport specific. Most Internet services use the same port number running over TCP/UDP as shown in the IANA database. And the IANA RDS registration is the same. What is the rationale of having a transport specific number in the RDS implementation?

because not every transport may need a port number.

e.g., "RDS over pigeon carrier" may not need a port number.

in a different design (e.g., KCM) the listen port is configurable with sysctl

Some may need more than one, e.g., rds_rdma, evidently.

What is the problem with using the unused 18635 for the RDS_CM_PORT?

#9 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

Hi Ka-Cheong,

Quoted 11 lines · Sowmini Varadhan
Quoted 7 lines · Ka-Cheong Poon

As mentioned in a previous mail, it is unclear why the port number is transport specific. Most Internet services use the same port number running over TCP/UDP as shown in the IANA database. And the IANA RDS registration is the same. What is the rationale of having a transport specific number in the RDS implementation?

because not every transport may need a port number.

Lets keep separate port for RDMA and TCP transport. This has been already useful for wireshark dissector and can also help for eBPF like external tooling. The fragment format and re-assembly is different across transports.

I do see your point and also agree that port number isn't transport specific and in case we need to add another transport, what port to use. But may be till then lets keep the existing behavior. As such this port switch is not related to IPv6 support as such so lets deal with it separately.

Regards,
Santosh

#10 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

Quoted 12 lines · Santosh Shilimkar

Hi Ka-Cheong,

Quoted 9 lines · Sowmini Varadhan
Quoted 7 lines · Ka-Cheong Poon

As mentioned in a previous mail, it is unclear why the port number is transport specific.  Most Internet services use the same port number running over TCP/UDP as shown in the IANA database.  And the IANA RDS registration is the same.  What is the rationale of having a transport specific number in the RDS implementation?

Quoted 6 lines
Quoted 2 lines

because not every transport may need a port number.

Lets keep separate port for RDMA and TCP transport. This has been already useful for wireshark dissector and can also help for eBPF like external tooling. The fragment format and re-assembly is different across transports.

But does it have anything to do with the fact that a #define is in the rds.h file?

Quoted 5 lines

I do see your point and also agree that port number isn't transport specific and in case we need to add another transport, what port to use. But may be till then lets keep the existing behavior. As such this port switch is not related to IPv6 support as such so lets deal with it separately.

As I mentioned in a previous mail, I can move the #define if it makes folks happy. But the number cannot be changed as it is already being used.

Signature · 2 lines
K. Poon
[email protected]
#11 lore raw

Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6_addr

In reply to Ka-Cheong Poon
Quoted 30 lines · Ka-Cheong Poon
Quoted 16 lines · Santosh Shilimkar
Quoted 16 lines
net/rds/connection.c +3 -3
index abef75d..ca72563 100644
@@ -142,9 +151,12 @@ static void __rds_conn_path_init(struct 
 rds_connection *conn,
    * are torn down as the module is removed, if ever.
    */
   static struct rds_connection *__rds_conn_create(struct net *net,
-                        __be32 laddr, __be32 faddr,
-                       struct rds_transport *trans, gfp_t gfp,
-                       int is_outgoing)
+                        const struct in6_addr *laddr,
+                        const struct in6_addr *faddr,
+                        struct rds_transport *trans,
Quoted 8 lines

net/rds/connection.c static void __rds_conn_path_init(struct rds_connection *conn, · line 159

Quoted 3 lines
net/rds/connection.c +3 -0
+                        gfp_t gfp,
+                        int is_outgoing,
+                        int dev_if)

Am just wondering if we can handle local link address case differently instead of pushing the interface index everywhere. Did you think about any alternative. This can also avoid bunch of changes again and hence the question. Am trying to see if we can minimize the changes to absolute must have to support IPv6.

Quoted 4 lines

If link local address is supported, scoped ID must be used. Unless we remove the support of link local address, we need to keep scope ID.

Ok.

Quoted 17 lines
Quoted 10 lines
Quoted 10 lines
net/rds/ib.h +3 -0
index a6f4d7d..12f96b3 100644
 
+union rds_ib_conn_priv {
+    struct rds_ib_connect_private    ricp_v4;
+    struct rds6_ib_connect_private    ricp_v6;

  };

Quoted 5 lines

This change was invetiable. Just add a comment saying the priviate data size for v6 vs v4 is  different. Also for IPv6 priviate data, I suggest add some resrve fields for any future extensions so that things can be added without breaking wire protocol. With IPv4, we ended up in bit of mess.

Quoted 5 lines

Will add some comments.  Unfortunately the IB private data exchange has only a limited space.  I don't think there is any more space left for reserved field.  I think we should think of a different way to handle extensions in future.

There is enough space. You can send upto 512 bytes iirc. Please add some reserve fields and ping me if you run into issues.

Quoted 33 lines
Quoted 23 lines
Quoted 23 lines
net/rds/send.c +10 -3
index 94c7f74..6ed2e92 100644
@@ -1081,27 +1085,59 @@ int rds_sendmsg(struct socket *sock, struct 
 msghdr *msg, size_t payload_len)
           goto out;
       }
-    if (msg->msg_namelen) {
-        /* XXX fail non-unicast destination IPs? */
-        if (msg->msg_namelen < sizeof(*usin) || usin->sin_family != 
 AF_INET) {
+    namelen = msg->msg_namelen;
+    if (namelen != 0) {
+        if (namelen < sizeof(*usin)) {
+            ret = -EINVAL;
+            goto out;
+        }
+        switch (namelen) {
+        case sizeof(*usin):
+            if (usin->sin_family != AF_INET ||
+                usin->sin_addr.s_addr == htonl(INADDR_ANY) ||
Quoted 8 lines

net/rds/send.c int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len) · line 1100

Quoted 3 lines
net/rds/send.c +3 -0
+                usin->sin_addr.s_addr == htonl(INADDR_BROADCAST) ||
+                IN_MULTICAST(ntohl(usin->sin_addr.s_addr))) {
+                ret = -EINVAL;

The idea was to fail non-unicast IP someday but can you not add this change as part of v6 series. We can look at it later unless you need this change for v6. Again the question is mainly to add only necessary check and leave the existing behavior as is so please re-check all below checks too.

Quoted 5 lines

This is to match the same IPv6 check.  In IPv6 code, it checks if the address is a unicast address.  I can remove the IPv4 checks but if an app does send to a multicast address, the connection will be stuck forever.

OK. Lets keep it then.

Quoted 15 lines
Quoted 6 lines
Quoted 5 lines
net/rds/transport.c +0 -0
index 0b188dd..c9788db 100644
Quoted 7 lines
Quoted 1 line

+    if (ipv6_addr_v4mapped(addr)) {

Dave already commented on ipv6_addr_v4mapped(). Apart from some of those comments questions, rest of the patch looks really good and nicely done. Will also look at your subsequent two patches and try to send you comments in next few days.

Quoted 2 lines

Do you mean using mapped address to create IPv4 connections? I already have it in my work space.  Will be in v3.

yeah. Thanks !!

#12 lore raw

PATCH v2 2/3 rds: Enable RDS IPv6 support

In reply to Ka-Cheong Poon

This patch enables RDS to use IPv6 addresses. For RDS/TCP, the listener is now an IPv6 endpoint which accepts both IPv4 and IPv6 connection requests. RDS/RDMA/IB uses a private data (struct rds_ib_connect_private) exchange between endpoints at RDS connection establishment time to support RDMA. This private data exchange uses a 32 bit integer to represent an IP address. This needs to be changed in order to support IPv6. A new private data struct rds6_ib_connect_private is introduced to handle this. To ensure backward compatibility, an IPv6 capable RDS stack uses another RDMA listener port (RDS_CM_PORT) to accept IPv6 connection. And it continues to use the original RDS_PORT for IPv4 RDS connections. When it needs to communicate with an IPv6 peer, it uses the RDS_CM_PORT to send the connection set up request.

v2: Fixed bound and peer address scope mismatched issue.
Added back rds_connect() IPv6 changes.

---
 net/rds/af_rds.c         | 28 +++++++++++++++++++++++-
 net/rds/bind.c           | 34 +++++++++++++++++++++++++-----
 net/rds/connection.c     | 43 ++++++++++++++++++++++++-------------
 net/rds/ib.c             | 55 +++++++++++++++++++++++++++++++++++++++++-------
 net/rds/ib_cm.c          | 13 ++++++------
 net/rds/rdma_transport.c | 32 ++++++++++++++++++++++++++--
 net/rds/rdma_transport.h |  2 ++
 net/rds/rds.h            | 12 ++++++-----
 net/rds/send.c           | 32 ++++++++++++++++++++++++++--
 net/rds/tcp.c            | 54 +++++++++++++++++++++++++++++------------------
 net/rds/tcp.h            |  4 +---
 net/rds/tcp_connect.c    | 54 ++++++++++++++++++++++++++++++++++++-----------
 net/rds/tcp_listen.c     | 40 +++++++++++++++++++++++++++--------
 13 files changed, 315 insertions(+), 88 deletions(-)
net/rds/af_rds.c +27 -1
index fc1a5c6..8ce2d92 100644
@@ -484,7 +484,9 @@ static int rds_connect(struct socket *sock, struct sockaddr *uaddr,
 {
 	struct sock *sk = sock->sk;
 	struct sockaddr_in *sin;
+	struct sockaddr_in6 *sin6;
 	struct rds_sock *rs = rds_sk_to_rs(sk);
+	int addr_type;
 	int ret = 0;
 
 	lock_sock(sk);
@@ -510,7 +512,31 @@ static int rds_connect(struct socket *sock, struct sockaddr *uaddr,
 		break;
 
 	case sizeof(struct sockaddr_in6):
-		ret = -EPROTONOSUPPORT;
+		sin6 = (struct sockaddr_in6 *)uaddr;
+		if (sin6->sin6_family != AF_INET6) {
+			ret = -EAFNOSUPPORT;
+			break;
+		}
+		addr_type = ipv6_addr_type(&sin6->sin6_addr);
+		if (!(addr_type & IPV6_ADDR_UNICAST)) {
+			ret = -EPROTOTYPE;
+			break;
+		}
+		if (addr_type & IPV6_ADDR_LINKLOCAL) {
+			if (sin6->sin6_scope_id == 0 ||
+			    (!ipv6_addr_any(&rs->rs_bound_addr) &&
+			     sin6->sin6_scope_id != rs->rs_bound_scope_id)) {
+				ret = -EINVAL;
+				break;
+			}
+			/* Remember the connected address scope ID.  It will
+			 * be checked against the binding local address when
+			 * the socket is bound.
+			 */
+			rs->rs_bound_scope_id = sin6->sin6_scope_id;
+		}
+		rs->rs_conn_addr = sin6->sin6_addr;
+		rs->rs_conn_port = sin6->sin6_port;
 		break;
 
 	default:
net/rds/bind.c +29 -5
index 3822886..6e6e4ea 100644
@@ -127,9 +127,10 @@ static int rds_add_bound(struct rds_sock *rs, const struct in6_addr *addr,
 		if (!rhashtable_insert_fast(&bind_hash_table,
 					    &rs->rs_bound_node, ht_parms)) {
 			*port = rs->rs_bound_port;
+			rs->rs_bound_scope_id = scope_id;
 			ret = 0;
-			rdsdebug("rs %p binding to %pI4:%d\n",
-			  rs, &addr, (int)ntohs(*port));
+			rdsdebug("rs %p binding to %pI6c:%d\n",
+				 rs, addr, (int)ntohs(*port));
 			break;
 		} else {
 			rs->rs_bound_addr = in6addr_any;
@@ -164,11 +165,12 @@ int rds_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	struct in6_addr v6addr, *binding_addr;
 	struct rds_transport *trans;
 	__u32 scope_id = 0;
+	int addr_type;
 	int ret = 0;
 	__be16 port;
 
-	/* We only allow an RDS socket to be bound to and IPv4 address. IPv6
-	 * address support will be added later.
+	/* We allow an RDS socket to be bound to either IPv4 or IPv6
+	 * address.
 	 */
 	if (addr_len == sizeof(struct sockaddr_in)) {
 		struct sockaddr_in *sin = (struct sockaddr_in *)uaddr;
@@ -180,7 +182,21 @@ int rds_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 		binding_addr = &v6addr;
 		port = sin->sin_port;
 	} else if (addr_len == sizeof(struct sockaddr_in6)) {
-		return -EPROTONOSUPPORT;
+		struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)uaddr;
+
+		addr_type = ipv6_addr_type(&sin6->sin6_addr);
+		if (sin6->sin6_family != AF_INET6 ||
+		    !(addr_type & IPV6_ADDR_UNICAST)) {
+			return -EINVAL;
+		}
+		/* The scope ID must be specified for link local address. */
+		if (addr_type & IPV6_ADDR_LINKLOCAL) {
+			if (sin6->sin6_scope_id == 0)
+				return -EINVAL;
+			scope_id = sin6->sin6_scope_id;
+		}
+		binding_addr = &sin6->sin6_addr;
+		port = sin6->sin6_port;
 	} else {
 		return -EINVAL;
 	}
@@ -191,6 +207,14 @@ int rds_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 		ret = -EINVAL;
 		goto out;
 	}
+	/* Socket is connected.  The binding address should have the same
+	 * scope ID as the connected address.
+	 */
+	if (!ipv6_addr_any(&rs->rs_conn_addr) &&
+	    scope_id != rs->rs_bound_scope_id) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = rds_add_bound(rs, binding_addr, &port, scope_id);
 	if (ret)
net/rds/connection.c +28 -15

73 lines not on this page · raw message

net/rds/ib.c +47 -8

99 lines not on this page · raw message

net/rds/ib_cm.c +7 -6
index 5b8b181..250be1c 100644
@@ -40,7 +40,6 @@
 #include "rds_single_path.h"
 #include "rds.h"
 #include "ib.h"
-#include "tcp.h"
 
 /*
  * Set the selected protocol version
@@ -679,7 +678,7 @@ static u32 rds_ib_protocol_compatible(struct rdma_cm_event *event, bool isv6)
 	return version;
 }
 
-/* Given an IPv6 address, find the IB net_device which hosts that address and
+/* Given an IPv6 address, find the net_device which hosts that address and
  * return its index.  This is used by the rds_ib_cm_handle_connect() code to
  * find the interface index of where an incoming request comes from when
  * the request is using a link local address.
@@ -696,8 +695,7 @@ static u32 __rds_find_ifindex(struct net *net, const struct in6_addr *addr)
 
 	rcu_read_lock();
 	for_each_netdev_rcu(net, dev) {
-		if (dev->type == ARPHRD_INFINIBAND &&
-		    ipv6_chk_addr(net, addr, dev, 0)) {
+		if (ipv6_chk_addr(net, addr, dev, 0)) {
 			idx = dev->ifindex;
 			break;
 		}
@@ -887,7 +885,10 @@ int rds_ib_conn_path_connect(struct rds_conn_path *cp)
 
 	/* XXX I wonder what affect the port space has */
 	/* delegate cm event handler to rdma_transport */
-	handler = rds_rdma_cm_event_handler;
+	if (conn->c_isv6)
+		handler = rds6_rdma_cm_event_handler;
+	else
+		handler = rds_rdma_cm_event_handler;
 	ic->i_cm_id = rdma_create_id(&init_net, handler, conn,
 				     RDMA_PS_TCP, IB_QPT_RC);
 	if (IS_ERR(ic->i_cm_id)) {
@@ -923,7 +924,7 @@ int rds_ib_conn_path_connect(struct rds_conn_path *cp)
 		sin6 = (struct sockaddr_in6 *)&dest;
 		sin6->sin6_family = AF_INET6;
 		sin6->sin6_addr = conn->c_faddr;
-		sin6->sin6_port = htons(RDS_TCP_PORT);
+		sin6->sin6_port = htons(RDS_CM_PORT);
 		sin6->sin6_scope_id = conn->c_dev_if;
 	}
 
net/rds/rdma_transport.c +30 -2
index aef73e7..bd67e55 100644
@@ -37,7 +37,9 @@
 #include "rdma_transport.h"
 #include "ib.h"
 
+/* Global IPv4 and IPv6 RDS RDMA listener cm_id */
 static struct rdma_cm_id *rds_rdma_listen_id;
+static struct rdma_cm_id *rds6_rdma_listen_id;
 
 static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
 					 struct rdma_cm_event *event,
@@ -153,6 +155,12 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
 	return rds_rdma_cm_event_handler_cmn(cm_id, event, false);
 }
 
+int rds6_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
+			       struct rdma_cm_event *event)
+{
+	return rds_rdma_cm_event_handler_cmn(cm_id, event, true);
+}
+
 static int rds_rdma_listen_init_common(rdma_cm_event_handler handler,
 				       struct sockaddr *sa,
 				       struct rdma_cm_id **ret_cm_id)
@@ -199,13 +207,14 @@ static int rds_rdma_listen_init_common(rdma_cm_event_handler handler,
 
 /* Initialize the RDS RDMA listeners.  We create two listeners for
  * compatibility reason.  The one on RDS_PORT is used for IPv4
- * requests only.  The one on RDS_TCP_PORT is used for IPv6 requests
+ * requests only.  The one on RDS_CM_PORT is used for IPv6 requests
  * only.  So only IPv6 enabled RDS module will communicate using this
  * port.
  */
 static int rds_rdma_listen_init(void)
 {
 	int ret;
+	struct sockaddr_in6 sin6;
 	struct sockaddr_in sin;
 
 	sin.sin_family = PF_INET;
@@ -214,7 +223,21 @@ static int rds_rdma_listen_init(void)
 	ret = rds_rdma_listen_init_common(rds_rdma_cm_event_handler,
 					  (struct sockaddr *)&sin,
 					  &rds_rdma_listen_id);
-	return ret;
+	if (ret != 0)
+		return ret;
+
+	sin6.sin6_family = PF_INET6;
+	sin6.sin6_addr = in6addr_any;
+	sin6.sin6_port = htons(RDS_CM_PORT);
+	sin6.sin6_scope_id = 0;
+	sin6.sin6_flowinfo = 0;
+	ret = rds_rdma_listen_init_common(rds6_rdma_cm_event_handler,
+					  (struct sockaddr *)&sin6,
+					  &rds6_rdma_listen_id);
+	/* Keep going even when IPv6 is not enabled in the system. */
+	if (ret != 0)
+		rdsdebug("Cannot set up IPv6 RDMA listener\n");
+	return 0;
 }
 
 static void rds_rdma_listen_stop(void)
@@ -224,6 +247,11 @@ static void rds_rdma_listen_stop(void)
 		rdma_destroy_id(rds_rdma_listen_id);
 		rds_rdma_listen_id = NULL;
 	}
+	if (rds6_rdma_listen_id) {
+		rdsdebug("cm %p\n", rds6_rdma_listen_id);
+		rdma_destroy_id(rds6_rdma_listen_id);
+		rds6_rdma_listen_id = NULL;
+	}
 }
 
 static int rds_rdma_init(void)
net/rds/rdma_transport.h +2 -0
index d309c44..bc3c639 100644
@@ -11,6 +11,8 @@
 int rds_rdma_conn_connect(struct rds_connection *conn);
 int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
 			      struct rdma_cm_event *event);
+int rds6_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
+			       struct rdma_cm_event *event);
 
 /* from ib.c */
 extern struct rds_transport rds_ib_transport;
net/rds/rds.h +7 -5
index 859808a..f5f99d1 100644
@@ -24,13 +24,15 @@
 #define RDS_PROTOCOL_MINOR(v)	((v) & 255)
 #define RDS_PROTOCOL(maj, min)	(((maj) << 8) | min)
 
-/*
- * XXX randomly chosen, but at least seems to be unused:
- * #               18464-18768 Unassigned
- * We should do better.  We want a reserved port to discourage unpriv'ed
- * userspace from listening.
+/* The following ports, 16385, 18634, 18635, are registered with IANA as
+ * the ports to be used for RDS over TCP and UDP.  18634 is the historical
+ * value used for the RDMA_CM listener port.  RDS/TCP uses port 16385.  After
+ * IPv6 work, RDMA_CM also uses 16385 as the listener port.  18634 is kept
+ * to ensure compatibility with older RDS modules.
  */
 #define RDS_PORT	18634
+#define RDS_CM_PORT	16385
+#define RDS_TCP_PORT	RDS_CM_PORT
 
 #ifdef ATOMIC64_INIT
 #define KERNEL_HAS_ATOMIC64
net/rds/send.c +30 -2
index 6ed2e92..c0e4f0b 100644
@@ -1105,8 +1105,28 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 			break;
 
 		case sizeof(*sin6): {
-			ret = -EPROTONOSUPPORT;
-			goto out;
+			int addr_type;
+
+			if (sin6->sin6_family != AF_INET6) {
+				ret = -EINVAL;
+				goto out;
+			}
+			addr_type = ipv6_addr_type(&sin6->sin6_addr);
+			if (!(addr_type & IPV6_ADDR_UNICAST)) {
+				ret = -EINVAL;
+				goto out;
+			}
+			if (addr_type & IPV6_ADDR_LINKLOCAL) {
+				if (sin6->sin6_scope_id == 0) {
+					ret = -EINVAL;
+					goto out;
+				}
+				scope_id = sin6->sin6_scope_id;
+			}
+
+			daddr = sin6->sin6_addr;
+			dport = sin6->sin6_port;
+			break;
 		}
 
 		default:
@@ -1138,6 +1158,14 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 			ret = -EOPNOTSUPP;
 			goto out;
 		}
+		/* If the socket is already bound to a link local address,
+		 * it can only send to peers on the same link.
+		 */
+		if (scope_id != rs->rs_bound_scope_id) {
+			release_sock(sk);
+			ret = -EINVAL;
+			goto out;
+		}
 	}
 	release_sock(sk);
 
net/rds/tcp.c +34 -20

103 lines not on this page · raw message

net/rds/tcp.h +1 -3
index c6fa080..6a948c1 100644
@@ -2,8 +2,6 @@
 #ifndef _RDS_TCP_H
 #define _RDS_TCP_H
 
-#define RDS_TCP_PORT	16385
-
 struct rds_tcp_incoming {
 	struct rds_incoming	ti_inc;
 	struct sk_buff_head	ti_skb_list;
@@ -67,7 +65,7 @@ void rds_tcp_restore_callbacks(struct socket *sock,
 void rds_tcp_state_change(struct sock *sk);
 
 /* tcp_listen.c */
-struct socket *rds_tcp_listen_init(struct net *);
+struct socket *rds_tcp_listen_init(struct net *net, bool isv6);
 void rds_tcp_listen_stop(struct socket *sock, struct work_struct *acceptor);
 void rds_tcp_listen_data_ready(struct sock *sk);
 int rds_tcp_accept_one(struct socket *sock);
net/rds/tcp_connect.c +42 -12

80 lines not on this page · raw message

net/rds/tcp_listen.c +31 -9
index 4fdf5b3..0f996e4 100644
@@ -256,15 +256,22 @@ void rds_tcp_listen_data_ready(struct sock *sk)
 		ready(sk);
 }
 
-struct socket *rds_tcp_listen_init(struct net *net)
+struct socket *rds_tcp_listen_init(struct net *net, bool isv6)
 {
-	struct sockaddr_in sin;
 	struct socket *sock = NULL;
+	struct sockaddr_storage ss;
+	struct sockaddr_in6 *sin6;
+	struct sockaddr_in *sin;
+	int addr_len;
 	int ret;
 
-	ret = sock_create_kern(net, PF_INET, SOCK_STREAM, IPPROTO_TCP, &sock);
-	if (ret < 0)
+	ret = sock_create_kern(net, isv6 ? PF_INET6 : PF_INET, SOCK_STREAM,
+			       IPPROTO_TCP, &sock);
+	if (ret < 0) {
+		rdsdebug("could not create %s listener socket: %d\n",
+			 isv6 ? "IPv6" : "IPv4", ret);
 		goto out;
+	}
 
 	sock->sk->sk_reuse = SK_CAN_REUSE;
 	rds_tcp_nonagle(sock);
@@ -274,13 +281,28 @@ struct socket *rds_tcp_listen_init(struct net *net)
 	sock->sk->sk_data_ready = rds_tcp_listen_data_ready;
 	write_unlock_bh(&sock->sk->sk_callback_lock);
 
-	sin.sin_family = PF_INET;
-	sin.sin_addr.s_addr = (__force u32)htonl(INADDR_ANY);
-	sin.sin_port = (__force u16)htons(RDS_TCP_PORT);
+	if (isv6) {
+		sin6 = (struct sockaddr_in6 *)&ss;
+		sin6->sin6_family = PF_INET6;
+		sin6->sin6_addr = in6addr_any;
+		sin6->sin6_port = (__force u16)htons(RDS_TCP_PORT);
+		sin6->sin6_scope_id = 0;
+		sin6->sin6_flowinfo = 0;
+		addr_len = sizeof(*sin6);
+	} else {
+		sin = (struct sockaddr_in *)&ss;
+		sin->sin_family = PF_INET;
+		sin->sin_addr.s_addr = INADDR_ANY;
+		sin->sin_port = (__force u16)htons(RDS_TCP_PORT);
+		addr_len = sizeof(*sin);
+	}
 
-	ret = sock->ops->bind(sock, (struct sockaddr *)&sin, sizeof(sin));
-	if (ret < 0)
+	ret = sock->ops->bind(sock, (struct sockaddr *)&ss, addr_len);
+	if (ret < 0) {
+		rdsdebug("could not bind %s listener socket: %d\n",
+			 isv6 ? "IPv6" : "IPv4", ret);
 		goto out;
+	}
 
 	ret = sock->ops->listen(sock, 64);
 	if (ret < 0)
Signature · 1 line
1.8.3.1
#13 lore raw

PATCH v2 3/3 rds: Extend RDS API for IPv6 support

In reply to Ka-Cheong Poon

There are many data structures (RDS socket options) used by RDS apps which use a 32 bit integer to store IP address. To support IPv6, struct in6_addr needs to be used. To ensure backward compatibility, a new data structure is introduced for each of those data structures which use a 32 bit integer to represent an IP address. And new socket options are introduced to use those new structures. This means that existing apps should work without a problem with the new RDS module. For apps which want to use IPv6, those new data structures and socket options can be used. IPv4 mapped address is used to represent IPv4 address in the new data structures.

---
 include/uapi/linux/rds.h |  71 +++++++++++++++++++++++++++++++--
 net/rds/connection.c     | 100 +++++++++++++++++++++++++++++++++++++++++++----
 net/rds/ib.c             |  52 ++++++++++++++++++++++++
 net/rds/ib_mr.h          |   2 +
 net/rds/ib_rdma.c        |  11 +++++-
 net/rds/rds.h            |   4 ++
 net/rds/recv.c           |  25 ++++++++++++
 net/rds/tcp.c            |  44 +++++++++++++++++++++
 8 files changed, 298 insertions(+), 11 deletions(-)
include/uapi/linux/rds.h +68 -3
index 20c6bd0..518d40f 100644
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR Linux-OpenIB) */
 /*
- * Copyright (c) 2008 Oracle.  All rights reserved.
+ * Copyright (c) 2008, 2018 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -52,7 +52,7 @@
 #define RDS_RECVERR			5
 #define RDS_CONG_MONITOR		6
 #define RDS_GET_MR_FOR_DEST		7
-#define SO_RDS_TRANSPORT		8
+#define SO_RDS_TRANSPORT		9
 
 /* Socket option to tap receive path latency
  *	SO_RDS: SO_RDS_MSG_RXPATH_LATENCY
@@ -118,7 +118,17 @@
 #define RDS_INFO_IB_CONNECTIONS		10008
 #define RDS_INFO_CONNECTION_STATS	10009
 #define RDS_INFO_IWARP_CONNECTIONS	10010
-#define RDS_INFO_LAST			10010
+
+/* PF_RDS6 options */
+#define RDS6_INFO_CONNECTIONS		10011
+#define RDS6_INFO_SEND_MESSAGES		10012
+#define RDS6_INFO_RETRANS_MESSAGES	10013
+#define RDS6_INFO_RECV_MESSAGES		10014
+#define RDS6_INFO_SOCKETS		10015
+#define RDS6_INFO_TCP_SOCKETS		10016
+#define RDS6_INFO_IB_CONNECTIONS	10017
+
+#define RDS_INFO_LAST			10017
 
 struct rds_info_counter {
 	__u8	name[32];
@@ -140,6 +150,15 @@ struct rds_info_connection {
 	__u8		flags;
 } __attribute__((packed));
 
+struct rds6_info_connection {
+	__u64		next_tx_seq;
+	__u64		next_rx_seq;
+	struct in6_addr	laddr;
+	struct in6_addr	faddr;
+	__u8		transport[TRANSNAMSIZ];		/* null term ascii */
+	__u8		flags;
+} __attribute__((packed));
+
 #define RDS_INFO_MESSAGE_FLAG_ACK               0x01
 #define RDS_INFO_MESSAGE_FLAG_FAST_ACK          0x02
 
@@ -153,6 +172,17 @@ struct rds_info_message {
 	__u8		flags;
 } __attribute__((packed));
 
+struct rds6_info_message {
+	__u64	seq;
+	__u32	len;
+	struct in6_addr	laddr;
+	struct in6_addr	faddr;
+	__be16		lport;
+	__be16		fport;
+	__u8		flags;
+	__u8		tos;
+} __attribute__((packed));
+
 struct rds_info_socket {
 	__u32		sndbuf;
 	__be32		bound_addr;
@@ -163,6 +193,16 @@ struct rds_info_socket {
 	__u64		inum;
 } __attribute__((packed));
 
+struct rds6_info_socket {
+	__u32		sndbuf;
+	struct in6_addr	bound_addr;
+	struct in6_addr	connected_addr;
+	__be16		bound_port;
+	__be16		connected_port;
+	__u32		rcvbuf;
+	__u64		inum;
+} __attribute__((packed));
+
 struct rds_info_tcp_socket {
 	__be32          local_addr;
 	__be16          local_port;
@@ -175,6 +215,18 @@ struct rds_info_tcp_socket {
 	__u32           last_seen_una;
 } __attribute__((packed));
 
+struct rds6_info_tcp_socket {
+	struct in6_addr	local_addr;
+	__be16		local_port;
+	struct in6_addr	peer_addr;
+	__be16		peer_port;
+	__u64		hdr_rem;
+	__u64		data_rem;
+	__u32		last_sent_nxt;
+	__u32		last_expected_una;
+	__u32		last_seen_una;
+} __attribute__((packed));
+
 #define RDS_IB_GID_LEN	16
 struct rds_info_rdma_connection {
 	__be32		src_addr;
@@ -189,6 +241,19 @@ struct rds_info_rdma_connection {
 	__u32		rdma_mr_size;
 };
 
+struct rds6_info_rdma_connection {
+	struct in6_addr	src_addr;
+	struct in6_addr	dst_addr;
+	__u8		src_gid[RDS_IB_GID_LEN];
+	__u8		dst_gid[RDS_IB_GID_LEN];
+
+	__u32		max_send_wr;
+	__u32		max_recv_wr;
+	__u32		max_send_sge;
+	__u32		rdma_mr_max;
+	__u32		rdma_mr_size;
+};
+
 /* RDS message Receive Path Latency points */
 enum rds_message_rxpath_latency {
 	RDS_MSG_RX_HDR_TO_DGRAM_START = 0,
net/rds/connection.c +93 -7

178 lines not on this page · raw message

net/rds/ib.c +52 -0

76 lines not on this page · raw message

net/rds/ib_mr.h +2 -0
index 0ea4ab0..f440ace 100644
@@ -113,6 +113,8 @@ struct rds_ib_mr_pool *rds_ib_create_mr_pool(struct rds_ib_device *rds_dev,
 					     int npages);
 void rds_ib_get_mr_info(struct rds_ib_device *rds_ibdev,
 			struct rds_info_rdma_connection *iinfo);
+void rds6_ib_get_mr_info(struct rds_ib_device *rds_ibdev,
+			 struct rds6_info_rdma_connection *iinfo6);
 void rds_ib_destroy_mr_pool(struct rds_ib_mr_pool *);
 void *rds_ib_get_mr(struct scatterlist *sg, unsigned long nents,
 		    struct rds_sock *rs, u32 *key_ret);
net/rds/ib_rdma.c +10 -1
index 0ec9df0..e3c8bbb 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006, 2017 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 2018 Oracle and/or its affiliates. All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -180,6 +180,15 @@ void rds_ib_get_mr_info(struct rds_ib_device *rds_ibdev, struct rds_info_rdma_co
 	iinfo->rdma_mr_size = pool_1m->fmr_attr.max_pages;
 }
 
+void rds6_ib_get_mr_info(struct rds_ib_device *rds_ibdev,
+			 struct rds6_info_rdma_connection *iinfo6)
+{
+	struct rds_ib_mr_pool *pool_1m = rds_ibdev->mr_1m_pool;
+
+	iinfo6->rdma_mr_max = pool_1m->max_items;
+	iinfo6->rdma_mr_size = pool_1m->fmr_attr.max_pages;
+}
+
 struct rds_ib_mr *rds_ib_reuse_mr(struct rds_ib_mr_pool *pool)
 {
 	struct rds_ib_mr *ibmr = NULL;
net/rds/rds.h +4 -0
index f5f99d1..fd63b86 100644
@@ -875,6 +875,10 @@ int rds_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 void rds_inc_info_copy(struct rds_incoming *inc,
 		       struct rds_info_iterator *iter,
 		       __be32 saddr, __be32 daddr, int flip);
+void rds6_inc_info_copy(struct rds_incoming *inc,
+			struct rds_info_iterator *iter,
+			struct in6_addr *saddr, struct in6_addr *daddr,
+			int flip);
 
 /* send.c */
 int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len);
net/rds/recv.c +25 -0
index 4217961..10a2d0f 100644
@@ -792,3 +792,28 @@ void rds_inc_info_copy(struct rds_incoming *inc,
 
 	rds_info_copy(iter, &minfo, sizeof(minfo));
 }
+
+void rds6_inc_info_copy(struct rds_incoming *inc,
+			struct rds_info_iterator *iter,
+			struct in6_addr *saddr, struct in6_addr *daddr,
+			int flip)
+{
+	struct rds6_info_message minfo6;
+
+	minfo6.seq = be64_to_cpu(inc->i_hdr.h_sequence);
+	minfo6.len = be32_to_cpu(inc->i_hdr.h_len);
+
+	if (flip) {
+		minfo6.laddr = *daddr;
+		minfo6.faddr = *saddr;
+		minfo6.lport = inc->i_hdr.h_dport;
+		minfo6.fport = inc->i_hdr.h_sport;
+	} else {
+		minfo6.laddr = *saddr;
+		minfo6.faddr = *daddr;
+		minfo6.lport = inc->i_hdr.h_sport;
+		minfo6.fport = inc->i_hdr.h_dport;
+	}
+
+	rds_info_copy(iter, &minfo6, sizeof(minfo6));
+}
net/rds/tcp.c +44 -0

62 lines not on this page · raw message

Signature · 1 line
1.8.3.1
#14 lore raw
In reply to Ka-Cheong Poon

Some additional comments on this patchset (consolidated here, please tease this apart into patch1/patch2/patch3 as appropriate)

I looked at the most of rds-core, and the rds-tcp changes. Please make sure santosh looks at these carefully, especially.
- RDS bind key changes
- connection.c
- all the rds_rdma related changes (e.g., the ib* and rdma* files)

- rds_getname(): one of the existing properties of PF_RDS is that a connect() does not involve an implicit bind(). Note that you are basing the changes in rds_bind() on this behavior, thus I guess we make the choice of treating this as a feature, not a bug.

Since we are choosing to treat this behavior as a feature we need to be consistent in rds_getname(). If we find
(!peer and !ipv6_addr_any(rs_conn_addr)) and the socket is not yet bound, the returned address (address size, address family) should be based on the rs_conn_addr. (Otherwise if I connect to abc::1 without doing a bind(), and try to do a getsockname(), I get back a sockaddr_in?!)

- rds_cancel_sent_to() and rds_connect and rds_bind and rds_sendmsg As DaveM has already pointed out, we should be using sa_family to figure out sockaddr_in vs sockaddr_in6 (not the other way around of inspecting len first, and then the family- that way wont work if I pass in sockaddr_storage). E.g., see inet_dgram_connect.

        if (addr_len < sizeof(uaddr->sa_family))
                return -EINVAL;

- In net/rds/rds.h;

/* The following ports, 16385, 18634, 18635, are registered with IANA as
* the ports to be used for RDS over TCP and UDP. 18634 is the historical

What is "RDS over TCP and UDP"? There is no such thing as RDS-over-UDP. IN fact RDS has nothing to do with UDP. The comment is confused. See next item below, where the comment disappears.

- Also in net/rds/rds.h
Please dont define transport specific parameters like RD_CM_PORT in the common rds.h header. It is unfortunate that we already have RDS_PORT there, and we should try to clean that up as well. NOte that RDS_TCP_PORT is now in the correct transport-module-specific header (net/rds/tcp.h) and its unclean to drag it from there, into the common header as you are doing.

In fact I just tried to move the RDS_PORT definition into net/rds/rdma_transport.h and it built just-fine. So to summarize, please do the following:
1. move RDS_PORT into rdma_transport.h
2. add RDS_CM_PORT into rdma_transport.h
3. stop dragging RDS_TCP_PORT from its current happy home in net/rds/tcp.h to net/rds/rds.h

- net/rds/connection.c
As we have discussed offline before, the fact that we cannot report TCP seq# etc info via the existing rds-info API is not "a bug in the design of MPRDS" but rather a lacking in the API design. Moreover, much of the useful information around the TCP socket is already available via procfs, TCP_INFO etc, so the info by rds-info is rarely used for rds-tcp- the more useful information is around the RDS socket itself. So there is a bug in the comment, would be nice if you removed it.

Also, while you are there, s/exisiting/existing, please.

General comments:
-----------------
I remain unconvinced by your global <-> link-local arguments.

For UDP sockets we can do this:

eth0
host1 -------------------------------------- host2
abc::1/64 fe80::2
add abc::/64 as onlink subnet route

host1# traceroute6 -i eth0 -s abc::1 fe80::2

You just broke this for RDS and are using polemic to defend your case, but the main thrust of your diatribe seems to be "why would you need this?" I'll try to address that briefly here.

- There may be lot of valid reasons why host2 does not want to be configured with a global prefix. e.g., I only want host2 to be able to talk to onlink hosts.

- RDS mandatorily requires sockets to be bound. So the normal src addr selection (that would have caused host1 to use a link-local to talk to host2) is suppressed in this case

This is exactly the same as a UDP socket bound to abc::1

Note well, that one of the use-cases for RDS-TCP is to replace existing infra that uses UDP for cluster-IPC. This has come up before on netdev:

See https://www.mail-archive.com/[email protected]&q=subject:%22Re%5C%3A+%5C%5BPATCH+net%5C-next+0%5C%2F6%5C%5D+kcm%5C%3A+Kernel+Connection+Multiplexor+%5C%28KCM%5C%29%22&o=newest&f=1

so feature parity with udp is just as important as feature-parity for rds_rdma.

I hope that helps you see why we need to not break this gratuituously for rds-tcp.

BTW, etiquette is to cc folks who have offered review comments on the code. Please make sure to cc me in follow-ups to this thread.

Thank you.

--Sowmini

#15 lore raw
Quoted 4 lines · Sowmini Varadhan

- rds_getname(): one of the existing properties of PF_RDS is that a connect() does not involve an implicit bind(). Note that you are basing the changes in rds_bind() on this behavior, thus I guess we make the choice of treating this as a feature, not a bug.

This patch series does not change existing behavior. But I think this is a strange RDS semantics as it differs from other types of socket. But this is not about IPv6 support and can be dealt with later.

Quoted 6 lines

Since we are choosing to treat this behavior as a feature we need to be consistent in rds_getname(). If we find
(!peer and !ipv6_addr_any(rs_conn_addr)) and the socket is not yet bound, the returned address (address size, address family) should be based on the rs_conn_addr. (Otherwise if I connect to abc::1 without doing a bind(), and try to do a getsockname(), I get back a sockaddr_in?!)

OK, will change that.

Quoted 8 lines

- rds_cancel_sent_to() and rds_connect and rds_bind and rds_sendmsg As DaveM has already pointed out, we should be using sa_family to figure out sockaddr_in vs sockaddr_in6 (not the other way around of inspecting len first, and then the family- that way wont work if I pass in sockaddr_storage). E.g., see inet_dgram_connect.

         if (addr_len < sizeof(uaddr->sa_family))
                 return -EINVAL;

OK, will change that.

Quoted 8 lines

- In net/rds/rds.h;

/* The following ports, 16385, 18634, 18635, are registered with IANA as
* the ports to be used for RDS over TCP and UDP. 18634 is the historical

What is "RDS over TCP and UDP"? There is no such thing as RDS-over-UDP. IN fact RDS has nothing to do with UDP. The comment is confused. See next item below, where the comment disappears.

As mentioned in the above comments, they are registered with IANA. There is no RDS/UDP in Linux but the port numbers are registered nevertheless. And RDS can work over UDP AFAICT. BTW, it is really strange that RDS/TCP does not use the port number already registered. Anyway, the above comments are just a note on this fact in the IANA database. The following is a link to the IANA assignment.

https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.txt

Quoted 8 lines

- Also in net/rds/rds.h
Please dont define transport specific parameters like RD_CM_PORT in the common rds.h header. It is unfortunate that we already have RDS_PORT there, and we should try to clean that up as well. NOte that RDS_TCP_PORT is now in the correct transport-module-specific header (net/rds/tcp.h) and its unclean to drag it from there, into the common header as you are doing.

Quoted 7 lines

In fact I just tried to move the RDS_PORT definition into net/rds/rdma_transport.h and it built just-fine. So to summarize, please do the following:
1. move RDS_PORT into rdma_transport.h
2. add RDS_CM_PORT into rdma_transport.h
3. stop dragging RDS_TCP_PORT from its current happy home in net/rds/tcp.h to net/rds/rds.h

IMHO, there should only be RDS_PORT in the first place. And it can be used for all transports. For example, if RDS/SCTP is added, it can also use the same port number. There is no need to define a new port number for each transport. What is the rationale that each transport needs to have its own number and be defined in its own header file?

Quoted 8 lines

- net/rds/connection.c
As we have discussed offline before, the fact that we cannot report TCP seq# etc info via the existing rds-info API is not "a bug in the design of MPRDS" but rather a lacking in the API design. Moreover, much of the useful information around the TCP socket is already available via procfs, TCP_INFO etc, so the info by rds-info is rarely used for rds-tcp- the more useful information is around the RDS socket itself. So there is a bug in the comment, would be nice if you removed it.

If the behavior of a software component is modified/enhanced such that the existing API of this component has problems dealing with this new behavior, should the API be modified or a new API be added to handle that? If neither is done, shouldn't this be considered a bug?

Quoted 1 line

Also, while you are there, s/exisiting/existing, please.

OK, with change that.

Quoted 12 lines

General comments:
-----------------
I remain unconvinced by your global <-> link-local arguments.

For UDP sockets we can do this:

          eth0
    host1 -------------------------------------- host2
          abc::1/64                       fe80::2
                                          add abc::/64 as onlink subnet route
Quoted 5 lines
   host1# traceroute6 -i eth0 -s abc::1 fe80::2

You just broke this for RDS and are using polemic to defend your case, but the main thrust of your diatribe seems to be "why would you need this?" I'll try to address that briefly here.

It is unclear why you need to use words like "polemic" and "diatribe". What I wrote is in public and folks can judge whether they are personal in nature. I'd suggest that discussion should be done in a professional manner.

Quoted 3 lines

- There may be lot of valid reasons why host2 does not want to be configured with a global prefix. e.g., I only want host2 to be able to talk to onlink hosts.

Right. It can use link local address to do this. And peers can also use link local address to communicate with host2.

Quoted 3 lines

- RDS mandatorily requires sockets to be bound. So the normal src addr selection (that would have caused host1 to use a link-local to talk to host2) is suppressed in this case

This requirement may be something to be fixed in future. For example, one can do "rds-ping fe80::x%y" without specifying the source address to use and it works fine. The trick is that rds-ping uses an UDP socket to find the source address to use and then bind() the RDS socket to that address. So in this rds-ping example, it will use the correct link local address as specified in RFC 6724 to set up the RDS connection. This trick really should not be needed. But as I mentioned, this patch series does not change existing behavior.

Quoted 10 lines

This is exactly the same as a UDP socket bound to abc::1

Note well, that one of the use-cases for RDS-TCP is to replace existing infra that uses UDP for cluster-IPC. This has come up before on netdev:

See https://www.mail-archive.com/[email protected]&q=subject:%22Re%5C%3A+%5C%5BPATCH+net%5C-next+0%5C%2F6%5C%5D+kcm%5C%3A+Kernel+Connection+Multiplexor+%5C%28KCM%5C%29%22&o=newest&f=1

so feature parity with udp is just as important as feature-parity for rds_rdma.

FWIW, RDS is on top of other transport protocols. It does not really need to provide all services provided by the underlying protocols.

Quoted 2 lines

I hope that helps you see why we need to not break this gratuituously for rds-tcp.

Please note that I understand what you wrote. My objection is that it requires special set up and can lead to problems when people actually try to do that. RDS runs on other platforms too. This will frustrate users and generate calls. And RDS apps do not really need that as link local address i always available. Having said this, I will add the support to handle it to save further discussion.

Signature · 2 lines
K. Poon
[email protected]
#16 lore raw
Quoted 4 lines · Ka-Cheong Poon

This patch series does not change existing behavior. But I think this is a strange RDS semantics as it differs from other types of socket. But this is not about IPv6 support and can be dealt with later.

sure,

Quoted 15 lines
Quoted 6 lines

Since we are choosing to treat this behavior as a feature we need to be consistent in rds_getname(). If we find
(!peer and !ipv6_addr_any(rs_conn_addr)) and the socket is not yet bound, the returned address (address size, address family) should be based on the rs_conn_addr. (Otherwise if I connect to abc::1 without doing a bind(), and try to do a getsockname(), I get back a sockaddr_in?!)

OK, will change that.

Quoted 6 lines

- rds_cancel_sent_to() and rds_connect and rds_bind and rds_sendmsg As DaveM has already pointed out, we should be using sa_family to figure out sockaddr_in vs sockaddr_in6 (not the other way around of inspecting len first, and then the family- that way wont work if I pass in sockaddr_storage). E.g., see inet_dgram_connect.

Quoted 3 lines
Quoted 2 lines
        if (addr_len < sizeof(uaddr->sa_family))
                return -EINVAL;

OK, will change that.

thank you

Quoted 11 lines
Quoted 8 lines

- In net/rds/rds.h;

/* The following ports, 16385, 18634, 18635, are registered with IANA as
* the ports to be used for RDS over TCP and UDP. 18634 is the historical

What is "RDS over TCP and UDP"? There is no such thing as RDS-over-UDP. IN fact RDS has nothing to do with UDP. The comment is confused. See next item below, where the comment disappears.

As mentioned in the above comments, they are registered with IANA.

16385 is registered for RDS-TCP.

what does it mean to run rds-tcp and rds_rdma with the CM at the same time? Is this possible? (if it is, why not poach some other number like 2049 from NFS?!)

18635 is actually not used in the RDS code.
Why can you not use that for RDS_CM_PORT?

In general, please do NOT pull up these definitions into net/rds/rds.h They may change in the future- and the really belong in the transport specific header file - you question this further below, see my answer there.

Quoted 2 lines

There is no RDS/UDP in Linux but the port numbers are registered nevertheless. And RDS can work over UDP AFAICT. BTW, it is really

No it cannot. RDS needs a reliable transport. If you start using UDP (or pigeon-carrier) for the transport you have to build the reliability yourself.

The Oracle DB libraries either use RDS-TCP or RDS-IB or UDP (in the UDP case they do their own congestion management in userspace, and history has shown that it is hard to get that right, thus the desire to switch to rds-tcp).

Anyway, even though Andy Grover registered 18635 for "RDS-IB UDP", that is probably the most harmless one to use for this case, because
- it is unused,
- it calls itself rds-Infiniband,
- the likelihood of doing rds-udp is infinitesmally small (it is more likely that we will need to send packets from abc::1 -> fe80:2 before we need rds-udp :-) :-) :-))
- and if rds-udp happens, we can use 16385/udp for rds-udp

so please use 18635 for your RDS_CM_PORT and move it to IB specific header files and lets converge this thread.

Towing RDS_TCP_PORT around has absolutely nothing to do with your ipv6 patch set.

Quoted 2 lines

strange that RDS/TCP does not use the port number already registered. Anyway, the above comments are just a note on this fact in the IANA

16385 was in defacto use for a long time (since 2.6 kernels), thus I registered it as well when I started fixing this up.

the 18634/18635 are registered for rds-iB, (caps intended) not rds-tcp. They are available for your use.

Quoted 1 line

database. The following is a link to the IANA assignment.

yes, I am aware

Quoted 3 lines

IMHO, there should only be RDS_PORT in the first place. And it can be used for all transports. For example, if RDS/SCTP is added, it can also use the same port number. There is no need to define

if/when we add rds/sctp, we shall keep that in mind.

This is getting to be "arguing for the sake of arguing". I dont have the time for that.

Quoted 3 lines

a new port number for each transport. What is the rationale that each transport needs to have its own number and be defined in its own header file?

Some transports may not even need a port number. Some may need several. Some may use sysctl (suggested by Tom Herbert) to make this configurable. Until recently, we had rds/iWARP, that may need its own transport hooks, it does not make sense to peanut-butter that into the core module.

That is why it has to be in the transport. I hope that answers the question.

Quoted 5 lines

If the behavior of a software component is modified/enhanced such that the existing API of this component has problems dealing with this new behavior, should the API be modified or a new API be added to handle that? If neither is done, shouldn't this be considered a bug?

whatever. The design (parallelization for perf) is fine. Some parts of the API are work in progress based on priority/need. I dont want to bicker over this one, except to note that the judgemental nature of the comment is interesting.

Quoted 4 lines
Quoted 1 line

Also, while you are there, s/exisiting/existing, please.

OK, with change that.

Wonderful.

For the rest, I repeat: Oracle Clusters are using UDP/IPV6 today (with no RDS). You need feature compat with UDP for that reason.

--Sowmini

#17 lore raw
Quoted 5 lines · Sowmini Varadhan
Quoted 5 lines
Quoted 5 lines

- In net/rds/rds.h;

/* The following ports, 16385, 18634, 18635, are registered with IANA as
* the ports to be used for RDS over TCP and UDP. 18634 is the historical

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

What is "RDS over TCP and UDP"? There is no such thing as RDS-over-UDP. IN fact RDS has nothing to do with UDP. The comment is confused. See next item below, where the comment disappears.

As mentioned in the above comments, they are registered with IANA.

16385 is registered for RDS-TCP.

Yes, and it is noted in the comments. The above comment is stating the fact so that folks know what is already done when checking the code.

Quoted 3 lines

what does it mean to run rds-tcp and rds_rdma with the CM at the same time? Is this possible? (if it is, why not poach some other number like 2049 from NFS?!)

RDS can run over TCP and RDMA at the same time just like other Internet services running over different transports.

Quoted 2 lines

18635 is actually not used in the RDS code.
Why can you not use that for RDS_CM_PORT?

Just like any other Internet services, as long as a number is free, it can use that number and register it with IANA. There is no special meaning about the number itself. It is only a number to use and need to be agreed on. And the normal practice is that the same number be used for a service running over different Internet transports. This is a good practice to avoid confusion.

Quoted 4 lines

In general, please do NOT pull up these definitions into net/rds/rds.h They may change in the future- and the really belong in the transport specific header file - you question this further below, see my answer there.

If the number is changed, it will break compatibility. Just like any other Internet services, the registered service port number won't change unless under very special circumstances.

Quoted 6 lines
Quoted 2 lines

There is no RDS/UDP in Linux but the port numbers are registered nevertheless. And RDS can work over UDP AFAICT. BTW, it is really

No it cannot. RDS needs a reliable transport. If you start using UDP (or pigeon-carrier) for the transport you have to build the reliability yourself.

As you noted above, if one build the reliability on top of UDP, one can run RDS over UDP. Is there something which is preventing RDS to be run over other Internet transports?

Quoted 4 lines

The Oracle DB libraries either use RDS-TCP or RDS-IB or UDP (in the UDP case they do their own congestion management in userspace, and history has shown that it is hard to get that right, thus the desire to switch to rds-tcp).

So RDS can in fact run over UDP and there is already an implementation. Is there any problem with the comment then?

Quoted 11 lines

Anyway, even though Andy Grover registered 18635 for "RDS-IB UDP", that is probably the most harmless one to use for this case, because
- it is unused,
- it calls itself rds-Infiniband,
- the likelihood of doing rds-udp is infinitesmally small (it is more likely that we will need to send packets from abc::1 -> fe80:2 before we need rds-udp :-) :-) :-))
- and if rds-udp happens, we can use 16385/udp for rds-udp

so please use 18635 for your RDS_CM_PORT and move it to IB specific header files and lets converge this thread.

I don't understand the rationale. What is the problem of using 16385? The above points do not show any problem in using this number.

Quoted 2 lines

Towing RDS_TCP_PORT around has absolutely nothing to do with your ipv6 patch set.

Could you please elaborate this? For example, is it a problem of using port 80 for a HTPP web server running over SCTP? I don't believe there is. It is a normal practice in Internet services.

Quoted 5 lines
Quoted 2 lines

strange that RDS/TCP does not use the port number already registered. Anyway, the above comments are just a note on this fact in the IANA

16385 was in defacto use for a long time (since 2.6 kernels), thus I registered it as well when I started fixing this up.

So this is a fix for a previous issue. This is exactly the point I made above. The service port number won't change. I think this answers the "they may change in the future" point above.

Quoted 2 lines

the 18634/18635 are registered for rds-iB, (caps intended) not rds-tcp. They are available for your use.

As there is really no problem is using 16385 and Oracle Linux is already using it, I cannot agree on changing it. It will just cause inter-operability problem without any reason.

Quoted 9 lines
Quoted 1 line

database. The following is a link to the IANA assignment.

yes, I am aware

Quoted 3 lines

IMHO, there should only be RDS_PORT in the first place. And it can be used for all transports. For example, if RDS/SCTP is added, it can also use the same port number. There is no need to define

if/when we add rds/sctp, we shall keep that in mind.

This is the point of the comment. I'm glad that it is noted.

Quoted 7 lines

This is getting to be "arguing for the sake of arguing". I dont have the time for that.

Quoted 3 lines

a new port number for each transport. What is the rationale that each transport needs to have its own number and be defined in its own header file?

Quoted 8 lines

Some transports may not even need a port number. Some may need several. Some may use sysctl (suggested by Tom Herbert) to make this configurable. Until recently, we had rds/iWARP, that may need its own transport hooks, it does not make sense to peanut-butter that into the core module.

That is why it has to be in the transport. I hope that answers the question.

The above comment seems to suggest that any future extension of RDS must make use of a port number and cannot be changed just because a #define is put in the rds.h header file. I cannot see how this can be true. Anyway, if just moving the #define to a different file makes folks happier, I can do it to save time.

Signature · 2 lines
K. Poon
[email protected]