link-local address fix for rdma_resolve_addr

Sean, Roland

Here is the updated patch that Jason and I discussed last week.

rdma_resolve_addr() returns an error when attempting to resolve ipv6 link-local address. This patch fixes the handling of link-local address.

The patch was tested using rping run as such:

Link-local with scope:

# /usr/bin/rping -c -a <remote-system-link-local>%ib0

Link-local w/out scope: (expect failure)

# /usr/bin/rping -c -a <remote-system-link-local>
rdma_resolve_addr error -1

Own interface link local:

# /usr/bin/rping -c -a <remote-system-link-local>%ib0

Other ipv6 address:

# /usr/bin/rping -c -a 2001:db8:1234::2

(server side started with rping -s -P -v -a ::0)

Tested against ofed build OFED-1.5-20091019-0811 and kernel 2.6.30.

------------------------------------------------------

 drivers/infiniband/core/addr.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

index bd07803..3442256 100644
@@ -278,6 +278,21 @@ static int addr6_resolve_remote(struct sockaddr_in6 *src_in,
 	fl.nl_u.ip6_u.daddr = dst_in->sin6_addr;
 	fl.nl_u.ip6_u.saddr = src_in->sin6_addr;
 
+	if (ipv6_addr_type(&src_in->sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+		if (!src_in->sin6_scope_id)
+			return -EINVAL;
+		fl.oif = src_in->sin6_scope_id;
+	}
+	if (ipv6_addr_type(&dst_in->sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+		if (dst_in->sin6_scope_id) {
+			if (fl.oif && fl.oif != dst_in->sin6_scope_id)
+				return -EINVAL;
+			fl.oif = dst_in->sin6_scope_id;
+		}
+		if (!fl.oif)
+			return -EINVAL;
+	}
+
 	dst = ip6_route_output(&init_net, NULL, &fl);
 	if (!dst)
 		return ret;

@@ -390,14 +405,16 @@ static int addr_resolve_local(struct sockaddr *src_in,
 	case AF_INET6:
 	{
 		struct in6_addr *a;
+		int found = 0;
 
 		for_each_netdev(&init_net, dev)
 			if (ipv6_chk_addr(&init_net,
 					  &((struct sockaddr_in6 *) dst_in)->sin6_addr,
-					  dev, 1))
+					  dev, 1)) {
+				found = 1;
 				break;
-
-		if (!dev)
+			}
+		if (!found)
 			return -EADDRNOTAVAIL;
 
 		a = &((struct sockaddr_in6 *) src_in)->sin6_addr;

@@ -406,6 +423,8 @@ static int addr_resolve_local(struct sockaddr *src_in,
 			src_in->sa_family = dst_in->sa_family;
 			((struct sockaddr_in6 *) src_in)->sin6_addr =
 				((struct sockaddr_in6 *) dst_in)->sin6_addr;
+			((struct sockaddr_in6 *) src_in)->sin6_scope_id =
+				((struct sockaddr_in6 *) dst_in)->sin6_scope_id;
 			ret = rdma_copy_addr(addr, dev, dev->dev_addr);
 		} else if (ipv6_addr_loopback(a)) {
 			ret = rdma_translate_ip(dst_in, addr);

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

The intent is to see if the destination address is local. A source address may not be given.

- Sean

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

Well, that makes more sense, but it still pretty strange to match the IP list like that, the proper thing is to query RT6_TABLE_LOCAL, like the IPv4 case does.

Anyhow, couldn't the whole addr_resolve_local routine be replaced with something like this in addr_resolve_remote:
if (rt->idev == init_net->loopback_dev)

    rdma_translate_ip(rt->rt_src, dev_addr, NULL);

for IPv4 and similar for IPv6?

That does query the proper RT_TABLEs to determine if the IP is local and then we get the searching and ip_dev_find only for the case where the address is definitely looped back. Much closer to how the IP stack works normally.

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

Jason, Sean
I have separated the patches into three to make them simpler to discuss. One patch fixes the handling of link-local address in addr6_resolve_remote(). The second patch fixes the bugs in addr_resolve_local(). The 3d patch I am posting now for discussion.

This patch, as Jason's suggested, moves the function of addr_resolve_local() into addr4_resolve_remote() and addr6_resolve_remote(). It eliminates the need for addr_resolve_local().

 drivers/infiniband/core/addr.c |   99 +++++++--------------------------------
 1 files changed, 18 insertions(+), 81 deletions(-)

index bd07803..f7a5861 100644
@@ -233,6 +233,14 @@ static int addr4_resolve_remote(struct sockaddr_in *src_in,
 	if (ret)
 		goto out;
 
+	if (rt->idev->dev == init_net.loopback_dev){
+		ret = rdma_translate_ip((struct sockaddr *)dst_in, addr);
+		if (!ret)
+			memcpy(addr->dst_dev_addr, addr->src_dev_addr,
+				MAX_ADDR_LEN);
+		goto put;
+	}
+
 	/* If the device does ARP internally, return 'done' */
 	if (rt->idev->dev->flags & IFF_NOARP) {
 		rdma_copy_addr(addr, rt->idev->dev, NULL);

@@ -282,6 +290,14 @@ static int addr6_resolve_remote(struct sockaddr_in6 *src_in,
 	if (!dst)
 		return ret;
 
+	if (dst->dev == init_net.loopback_dev) {
+		ret=rdma_translate_ip((struct sockaddr *)dst_in, addr);
+		if (!ret)
+			memcpy(addr->dst_dev_addr, addr->src_dev_addr,
+				MAX_ADDR_LEN);
+		goto release;
+	}
+
 	if (dst->dev->flags & IFF_NOARP) {
 		ret = rdma_copy_addr(addr, dst->dev, NULL);
 	} else {

@@ -289,7 +305,7 @@ static int addr6_resolve_remote(struct sockaddr_in6 *src_in,
 		if (neigh && (neigh->nud_state & NUD_VALID))
 			ret = rdma_copy_addr(addr, neigh->dev, neigh->ha);
 	}
-
+release:
 	dst_release(dst);
 	return ret;
 }

@@ -352,82 +368,6 @@ static void process_req(struct work_struct *work)
 	}
 }
 
-static int addr_resolve_local(struct sockaddr *src_in,
-			      struct sockaddr *dst_in,
-			      struct rdma_dev_addr *addr)
-{
-	struct net_device *dev;
-	int ret;
-
-	switch (dst_in->sa_family) {
-	case AF_INET:
-	{
-		__be32 src_ip = ((struct sockaddr_in *) src_in)->sin_addr.s_addr;
-		__be32 dst_ip = ((struct sockaddr_in *) dst_in)->sin_addr.s_addr;
-
-		dev = ip_dev_find(&init_net, dst_ip);
-		if (!dev)
-			return -EADDRNOTAVAIL;
-
-		if (ipv4_is_zeronet(src_ip)) {
-			src_in->sa_family = dst_in->sa_family;
-			((struct sockaddr_in *) src_in)->sin_addr.s_addr = dst_ip;
-			ret = rdma_copy_addr(addr, dev, dev->dev_addr);
-		} else if (ipv4_is_loopback(src_ip)) {
-			ret = rdma_translate_ip(dst_in, addr);
-			if (!ret)
-				memcpy(addr->dst_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
-		} else {
-			ret = rdma_translate_ip(src_in, addr);
-			if (!ret)
-				memcpy(addr->dst_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
-		}
-		dev_put(dev);
-		break;
-	}
-
-#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
-	case AF_INET6:
-	{
-		struct in6_addr *a;
-
-		for_each_netdev(&init_net, dev)
-			if (ipv6_chk_addr(&init_net,
-					  &((struct sockaddr_in6 *) dst_in)->sin6_addr,
-					  dev, 1))
-				break;
-
-		if (!dev)
-			return -EADDRNOTAVAIL;
-
-		a = &((struct sockaddr_in6 *) src_in)->sin6_addr;
-
-		if (ipv6_addr_any(a)) {
-			src_in->sa_family = dst_in->sa_family;
-			((struct sockaddr_in6 *) src_in)->sin6_addr =
-				((struct sockaddr_in6 *) dst_in)->sin6_addr;
-			ret = rdma_copy_addr(addr, dev, dev->dev_addr);
-		} else if (ipv6_addr_loopback(a)) {
-			ret = rdma_translate_ip(dst_in, addr);
-			if (!ret)
-				memcpy(addr->dst_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
-		} else  {
-			ret = rdma_translate_ip(src_in, addr);
-			if (!ret)
-				memcpy(addr->dst_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
-		}
-		break;
-	}
-#endif
-
-	default:
-		ret = -EADDRNOTAVAIL;
-		break;
-	}
-
-	return ret;
-}
-
 int rdma_resolve_ip(struct rdma_addr_client *client,
 		    struct sockaddr *src_addr, struct sockaddr *dst_addr,
 		    struct rdma_dev_addr *addr, int timeout_ms,

@@ -455,10 +395,7 @@ int rdma_resolve_ip(struct rdma_addr_client *client,
 	src_in = (struct sockaddr *) &req->src_addr;
 	dst_in = (struct sockaddr *) &req->dst_addr;
 
-	req->status = addr_resolve_local(src_in, dst_in, addr);
-	if (req->status == -EADDRNOTAVAIL)
-		req->status = addr_resolve_remote(src_in, dst_in, addr);
-
+	req->status = addr_resolve_remote(src_in, dst_in, addr);
 	switch (req->status) {
 	case 0:
 		req->timeout = jiffies;

This looks exactly like what I was thinking of - have you tested this?

If it is OK, I'd make it the first in the series.

There were two things I was not sure about in my example.
1) Is 'init_net.loopback_dev' the correct reference for the loop device? Or is it something like dev_net(rt->idev->dev)->loopback_dev ?

I'm sensing it may be the latter, but can't investigate right now Donno much about this new namespace stuff really

2) Was rt->idev->dev the right choice for the ipv4 case? Or is it rt->u.dst.dev ?

The TCP case kinda looks like
int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)

        tmp = ip_route_connect(&rt, nexthop, inet->saddr,
                               RT_CONN_FLAGS(sk), sk->sk_bound_dev_if,
                               IPPROTO_TCP,
                               inet->sport, usin->sin_port, sk, 1);
        sk_setup_caps(sk, &rt->u.dst);

void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
__sk_dst_set(sk, dst);

And all later things key off the sk_get_dst. So I'm thinking that u.dst.dev might be correct.

I have no idea what the difference is though (can't look too hard right now)

The main other fixup I see is to remove

                ret = cma_bind_addr(id, src_addr, dst_addr);

addr_resolve_remote called by addr_resolve_ip to setup the bind device from the routing lookup. (This is what I mentioned in my last email)

Which then lets you fixup the checking and handling of the sin6_scopeid on the source address - and fixes the main other routing difference against the TCP stack.

Thanks for working on this!

Jason

Yes I did do some testing, but that brings up a good question. I am not sure I know what all should be tested? I am running rping with different destination address (and scoping). On the ipv4 side:
rping -c -a <ip-of-my-ib0-interface>
rping -c -a <ip-of-remote-nodes-ib0-interface>

For ipv6 I ran what I described previously. What I do need to do is add the option to rping to specify a source address and run it with various address. Any help you can give defining what exactly needs to be tested would be appreciated.

I think you may be correct I will look at that closer. I did explicitly verify the test worked in both cases.

Lots of discussion :) I will go through the mails, address the comments and post the entire series of patches.

Thanks for all your input.

Dave.

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

You can also test with ucmatose to verify ipv4 still works. Use the -b option to bind to a specific address.

- Sean

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

Sean
This patch adds ipv6 support to ucmatose.

------------------------------------------------------

 examples/cmatose.c |   31 +++++++++++++++++++------------
 1 files changed, 19 insertions(+), 12 deletions(-)

index 8c12347..481a6d0 100644
@@ -516,15 +516,15 @@ static int get_addr(char *dst, struct sockaddr_in *addr)
 		return ret;
 	}
 
-	if (res->ai_family != PF_INET) {
+	if (res->ai_family == PF_INET)
+		memcpy(addr, res->ai_addr, sizeof(struct sockaddr_in));
+	else if (res->ai_family == PF_INET6)
+		memcpy(addr, res->ai_addr, sizeof(struct sockaddr_in6));
+	else
 		ret = -1;
-		goto out;
-	}
 
-	*addr = *(struct sockaddr_in *) res->ai_addr;
-out:
-	freeaddrinfo(res);
-	return ret;
+        freeaddrinfo(res);
+        return ret;
 }
 
 static int run_server(void)

@@ -543,11 +543,18 @@ static int run_server(void)
 		ret = get_addr(src_addr, &test.src_in);
 		if (ret)
 			goto out;
-	} else
+		if(test.src_in.sin_family == AF_INET)
+			((struct sockaddr_in *) &test.src_in)->sin_port = port;
+		else
+			((struct sockaddr_in6 *) &test.src_in)->sin6_port=port;
+		
+	} else {
 		test.src_in.sin_family = PF_INET;
+		test.src_in.sin_port = port;
+	}
+
+	ret = rdma_bind_addr(listen_id, (struct sockaddr *)&test.src_in);
 
-	test.src_in.sin_port = port;
-	ret = rdma_bind_addr(listen_id, test.src_addr);
 	if (ret) {
 		perror("cmatose: bind address failed");
 		goto out;

@@ -628,8 +635,8 @@ static int run_client(void)
 	printf("cmatose: connecting\n");
 	for (i = 0; i < connections; i++) {
 		ret = rdma_resolve_addr(test.nodes[i].cma_id,
-					src_addr ? test.src_addr : NULL,
-					test.dst_addr, 2000);
+			src_addr ? (struct sockaddr *)&test.src_in : NULL,
+			(struct sockaddr *)&test.dst_in, 2000);
 		if (ret) {
 			perror("cmatose: failure getting addr");
 			connect_error();

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

Thanks.

I pulled this patch into my local git tree with just a couple of minor cleanups. What other patches, if any, did you use to test with it?

- Sean

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

From: David J. Wilder <[dwilder-r/[email protected]]>

---
We need to update struct cmatest to allow storing an IPv6 address, or we can overrun the buffer.

Running with this patch, the client causes a kernel bug on 2.6.31 in addr_resolve_local, so we'll still fixes for that. It also meant that I couldn't test this patch.

 examples/cmatose.c |   53 ++++++++++++++++++++++++++++------------------------
 1 files changed, 29 insertions(+), 24 deletions(-)

index 8c12347..84831ec 100644
@@ -75,10 +75,7 @@ struct cmatest {
 	int			connects_left;
 	int			disconnects_left;
 
-	struct sockaddr_in	dst_in;
-	struct sockaddr		*dst_addr;
-	struct sockaddr_in	src_in;
-	struct sockaddr		*src_addr;
+	struct rdma_addr	addr;
 };
 
 static struct cmatest test;

@@ -505,7 +502,7 @@ static int migrate_channel(struct rdma_cm_id *listen_id)
 	return ret;
 }
 
-static int get_addr(char *dst, struct sockaddr_in *addr)
+static int get_addr(char *dst, struct sockaddr *addr)
 {
 	struct addrinfo *res;
 	int ret;

@@ -516,15 +513,15 @@ static int get_addr(char *dst, struct sockaddr_in *addr)
 		return ret;
 	}
 
-	if (res->ai_family != PF_INET) {
+	if (res->ai_family == PF_INET)
+		memcpy(addr, res->ai_addr, sizeof(struct sockaddr_in));
+	else if (res->ai_family == PF_INET6)
+		memcpy(addr, res->ai_addr, sizeof(struct sockaddr_in6));
+	else
 		ret = -1;
-		goto out;
-	}
 
-	*addr = *(struct sockaddr_in *) res->ai_addr;
-out:
-	freeaddrinfo(res);
-	return ret;
+        freeaddrinfo(res);
+        return ret;
 }
 
 static int run_server(void)

@@ -540,14 +537,21 @@ static int run_server(void)
 	}
 
 	if (src_addr) {
-		ret = get_addr(src_addr, &test.src_in);
+		ret = get_addr(src_addr, &test.addr.src_addr);
 		if (ret)
 			goto out;
-	} else
-		test.src_in.sin_family = PF_INET;
+		if (test.addr.src_addr.sa_family == AF_INET)
+			((struct sockaddr_in *) &test.addr.src_addr)->sin_port = port;
+		else
+			((struct sockaddr_in6 *) &test.addr.src_addr)->sin6_port = port;
+		
+	} else {
+		test.addr.src_addr.sa_family = PF_INET;
+		((struct sockaddr_in *) &test.addr.src_addr)->sin_port = port;
+	}
+
+	ret = rdma_bind_addr(listen_id, &test.addr.src_addr);
 
-	test.src_in.sin_port = port;
-	ret = rdma_bind_addr(listen_id, test.src_addr);
 	if (ret) {
 		perror("cmatose: bind address failed");
 		goto out;

@@ -614,22 +618,25 @@ static int run_client(void)
 
 	printf("cmatose: starting client\n");
 	if (src_addr) {
-		ret = get_addr(src_addr, &test.src_in);
+		ret = get_addr(src_addr, &test.addr.src_addr);
 		if (ret)
 			return ret;
 	}
 
-	ret = get_addr(dst_addr, &test.dst_in);
+	ret = get_addr(dst_addr, &test.addr.dst_addr);
 	if (ret)
 		return ret;
 
-	test.dst_in.sin_port = port;
+	if (test.addr.dst_addr.sa_family == AF_INET)
+		((struct sockaddr_in *) &test.addr.dst_addr)->sin_port = port;
+	else
+		((struct sockaddr_in6 *) &test.addr.dst_addr)->sin6_port = port;
 
 	printf("cmatose: connecting\n");
 	for (i = 0; i < connections; i++) {
 		ret = rdma_resolve_addr(test.nodes[i].cma_id,
-					src_addr ? test.src_addr : NULL,
-					test.dst_addr, 2000);
+					src_addr ? &test.addr.src_addr : NULL,
+					&test.addr.dst_addr, 2000);
 		if (ret) {
 			perror("cmatose: failure getting addr");
 			connect_error();

@@ -717,8 +724,6 @@ int main(int argc, char **argv)
 		}
 	}
 
-	test.dst_addr = (struct sockaddr *) &test.dst_in;
-	test.src_addr = (struct sockaddr *) &test.src_in;
 	test.connects_left = connections;
 	test.disconnects_left = connections;
 

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

I have been testing ucmatose against today's ofed 1.5 build, with the patch I posted earlier with fixes to resolve_local() and ipv6_resolve_remote().

This patch is still missing the binding changes Jason suggested it just fixes the existing code. I did not see the panic you saw on 2.6.32, but I have run on that kernel yet.

Dave..

Patch (posted earlier)

rdma_resolve_addr() returns an error when attempting to resolve ipv6 link-local address. This patch fixes the handling of link-local address.

------------------------------------------------------

 drivers/infiniband/core/addr.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

index bd07803..3442256 100644
@@ -278,6 +278,21 @@ static int addr6_resolve_remote(struct sockaddr_in6 *src_in,
 	fl.nl_u.ip6_u.daddr = dst_in->sin6_addr;
 	fl.nl_u.ip6_u.saddr = src_in->sin6_addr;
 
+	if (ipv6_addr_type(&src_in->sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+		if (!src_in->sin6_scope_id)
+			return -EINVAL;
+		fl.oif = src_in->sin6_scope_id;
+	}
+	if (ipv6_addr_type(&dst_in->sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+		if (dst_in->sin6_scope_id) {
+			if (fl.oif && fl.oif != dst_in->sin6_scope_id)
+				return -EINVAL;
+			fl.oif = dst_in->sin6_scope_id;
+		}
+		if (!fl.oif)
+			return -EINVAL;
+	}
+
 	dst = ip6_route_output(&init_net, NULL, &fl);
 	if (!dst)
 		return ret;

@@ -390,14 +405,16 @@ static int addr_resolve_local(struct sockaddr *src_in,
 	case AF_INET6:
 	{
 		struct in6_addr *a;
+		int found = 0;
 
 		for_each_netdev(&init_net, dev)
 			if (ipv6_chk_addr(&init_net,
 					  &((struct sockaddr_in6 *) dst_in)->sin6_addr,
-					  dev, 1))
+					  dev, 1)) {
+				found = 1;
 				break;
-
-		if (!dev)
+			}
+		if (!found)
 			return -EADDRNOTAVAIL;
 
 		a = &((struct sockaddr_in6 *) src_in)->sin6_addr;

@@ -406,6 +423,8 @@ static int addr_resolve_local(struct sockaddr *src_in,
 			src_in->sa_family = dst_in->sa_family;
 			((struct sockaddr_in6 *) src_in)->sin6_addr =
 				((struct sockaddr_in6 *) dst_in)->sin6_addr;
+			((struct sockaddr_in6 *) src_in)->sin6_scope_id =
+				((struct sockaddr_in6 *) dst_in)->sin6_scope_id;
 			ret = rdma_copy_addr(addr, dev, dev->dev_addr);
 		} else if (ipv6_addr_loopback(a)) {
 			ret = rdma_translate_ip(dst_in, addr);

One quick comment, remove '_remote' from function names:
addr4_resolve_remote, addr6_resolve_remote, and addr_resolve_remote

It does all the behaviors, it passes the input into a routing lookup
- though it is true the source needs to be passed into the routing lookup, which I think David had in another patch. Dave; I guess you need to include that here now.

So, first case:

Looks like:

$ ip route get 10.0.0.11
local 10.0.0.11 dev lo  src 10.0.0.11 
    cache <local>  mtu 16436 advmss 16396 hoplimit 64

trips through the if == loopback and does
'rdma_translate_ip(10.0.0.11)'

Same as the old code (little different path, but seems to give the same result)

Second case:

Looks like:

$ ip route get 10.0.0.11 from 127.0.0.1
local 10.0.0.11 from 127.0.0.1 dev lo 
    cache <local>  mtu 16436 advmss 16396 hoplimit 64

so trips through the if == loopback and does rdma_translate_ip(10.0.0.11)

Same as the old code

Third case:

$ ip route get 10.0.0.11 from 10.0.0.11
local 10.0.0.11 from 10.0.0.11 dev lo 
    cache <local>  mtu 16436 advmss 16396 hoplimit 64

Again, does rdma_translate_ip(10.0.0.11)

And the weird case is different:

$ ip route get 10.0.0.11 from 192.168.122.1
local 10.0.0.11 from 192.168.122.1 dev lo 
    cache <local>  mtu 16436 advmss 16396 hoplimit 64

(192.168.122.1 is bound to a different device on my system than 10.0.0.11)

The new case trips the if == loopback and does
rdma_translate_ip(10.0.0.11)

The old case does rdma_translate_ip(192.168.122.1)

I don't think this is a significant difference, both behaviors are reasonable choices and the code/complexity savings are worth it, IMHO. [Alternatively, I suppose one could call rdma_translate_ip(rt->rt_src) and if that fails then do rdma_translate_ip(dst_in) but why bother?]

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

Oops, there is a little woopsie here:

Dave:

+ if (rt->idev->dev == init_net.loopback_dev){
+ ret = rdma_translate_ip((struct sockaddr *)dst_in, addr);
+ if (!ret)
+ memcpy(addr->dst_dev_addr, addr->src_dev_addr,
+ MAX_ADDR_LEN);
+ goto put;
+ }
+

The 'goto put' will skip over the source address assignment step.

Maybe move this:

        if (!src_ip) {
                src_in->sin_family = dst_in->sin_family;
                src_in->sin_addr.s_addr = rt->rt_src;
        }

Up above your if (rt->idev)

And similarly for v6.

Also add a src_in = rt->rt_src assignment for v6.

I'd also remove the test for 0 address, just do it unconditionally (IIRC routing table always returns src if src is not 0)

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

I agree this usage case is weird, but I'm still not sure about the patch. If the destination service is listening on 10.0.0.11, then the listen is bound to a different gid than the source gid that we're trying to connect from. The src_dev_addr and dst_dev_addr need to reflect this, so that a PR gives us a path that routes the CM REQ correctly.

- Sean

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

You've lost me..

The local loopback case uses PRs?

Even so, it still seems OK to me:

Path:
addr4_resolve_remote

  $ ip route get 10.0.0.11 from 192.168.122.1
    local 10.0.0.11 from 192.168.122.1 dev lo
  srcIP = 192.168.122.1
  rdma_translate_ip(dst_ip = 10.0.0.11)
   rdma_copy_addr("eth0");
    src_dev_addr = eth0.dev_addr  (ie GID of 10.0.0.11)
  memcpy(dst_dev_addr = src_dev_addr) (ie GID of 10.0.0.11)

So everthing is bound to the GID of 10.0.0.11 which matches the listen of 10.0.0.11, which seems OK.

The CM REP is sent to GID of 10.0.0.11 from GID of 10.0.0.11, with an private data that indicates a source IP of 192.168.122.1 and a dest IP of 10.0.0.11. This seems optimal to me?

By what I understand you said I would think the prior version would be the one that is in trouble, if I am listening on 10.0.0.11 and you make a connection sourced from 192.168.122.1, which is looped back, it seems reasonable to me that this will succeed and the outgoing connection would necessarily have be bound to the destination address since the listening process is attached there.

Am I missing something?

Remember, there is still one more missing part there, the cma_bind I was talking about has to go away. We want to bind the source device based on the routing lookup, with loopback binding to the destination device.

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

Yes - the rdma cm makes no distinction when resolve route is called. It does a PR query.

The source could have called rdma_bind_addr(192.168.122.1) prior to calling rdma_resolve_addr(). (DAPL does this.) This would have returned a different RDMA device than binding to 10.0.0.11. The client app could have allocated resources on that device, but the CM REQ will carry the gid/lid of the other device. The endpoints won't be able to communicate.

Yes, it's weird, and may not be optimal, but if a source address is explicitly given, then its mapping to a specific RDMA device should be honored.

- Sean

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

That is very difficult to fit into the semantics the IP routing model uses :( And it looks like an API problem in DAPL :(

So, I see now, you are proposing that in this case the connection attempt to be routed through the network and not looped back.. I actually have a big problem with that, ignoring a 'lo' entry in a routing table is very much not IP like and not a good idea. That should be respected..

I guess I'd much rather see that one situation return EHOSTUNREACH or something.

But, I suppose you are going to tell me that Intel MPI uses DAPL to loopback connect to other processes on the same node, and relies on this? :( :( :(

Sigh. Anyhow, lets not get side tracked. It seems to me, the easy way out for David's approach is to simply check if the device is already bound via rdma_bind() and if so force it to that device no matter what the routing table lookup returns. Can you suggest a reliable way to make that check?

[What happens now if I do this:
rdma_bind(10.0.0.11)
rdma_resolve_addr(src = 192.168.122.1 dst = 10.0.0.11)
Does the cma_bind path check that it is already bound and give out an error? too late for me to check]

Once the cma_bind for rdma_resolve_addr is moved into the addr_resolve_remote function then people using the API without calling bind on the client path will get sane IP-like behavior.

Remember, on Linux the IP is *not* attached to a device, it is part of the host itself. So the idea that a source address somehow specifies a RDMA device does not fit into the Linux IP networking model.

Unfortunately the definition of rdma_bind kinda bakes this mismatched model into the API :(

Truth be told, to fit the Linux IP model, the RDMA CM should have provided exactly only two ways to bind a cm_id to a specific device - rdma_accept and rdma_resolve_addr.

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

This depends on if your view is that the rdma cm is trying to match IP routing, trying to use IP addresses as convenient names for RDMA ports, or something in between that may lean more one way or the other depending on the device type. IMO, the primary reason for using IP addressing over IB is more for convenience, than compliance.

I hesitate comparing RDMA versus IP too closely.

I'm not entirely sure of the best way to test this. Checking the src_dev_addr is my first thought.

rdma_resolve_addr only calls bind if the rdma id is not already bound. The src_addr simply gets ignored in this case, and the bound address is used instead.

I like the approach, I'm just concerned about the case where the app has requested a specific source address, which today means that a specific RDMA device should be used.

I would say we're extending the linux networking model to incorporate this concept, while staying in our imposed little RDMA sand box world. Even iWarp has these issues.

I think this is more restrictive than things need to be.

- Sean

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

I more or less agree for IB, but iWarp really should match exactly, and TBH I'm disappointed that no iWarp folks ever reviewed this stuff for consistency with the IP stack, it goes to show that netdev wasn't far off their criticisms I guess :(

This should get updated to return -EINVAL for this case I think, since it is nonsense. Either src = null or src == bind addr

You get somethings and you loose somethings.. The Linux IP model is not choosen arbitarily, there are solid reasons why it works like that.

Mainly for RDMA what you get is more kernel flexability, IP like capabilities, better bonding, and better support of IB APM semantics:
- bind() + listen() actually works properly if more than one interface is bound to the same IP - the cm_id returned by accept is bound to the hca and port that accepted the connection
[ This is a L3 form of bonding Linux supports ]

This is actually something of a mandatory notion to implement the full generality of the IB CM protocol which allows the CM REP to contain a port GUID of another port on the same node (multi-port APM is an IB feature). So you never know what port the accept() result will get bound to.

BTW: I suppose ideally AF_IB would have a way to say 'CM accept REPs on any port on this node' Hmm, reserved GID prefix perhaps? Hmm.

When used with bonding this would also afford the kernel with the ability to accept incoming connections across all the redundant RDMA devices - and still have correct bound-to-IP semantics.

- rdma_resolve_addr more or less as the inverse of all the above
* multiple interfaces with same IP case works, kernel and routing table can distribute outgoing connections
* multi-port APM works, kernel and user space can choose primary and backup port for the IP addy
* bonding works, kernel can balance outgoing connections across the bond slaves.

These are all useful features.

Could we have had the above and still had pretty much enough flexability? Yes, I think so. The main stumbling block, IMHO, seems to have been the tying of PDs and a few other non QP verbs objects to a ibv_context, it should have been the other way around - a PD, comp channel, etc are fist class kernel objects that exist across all ports and devices. When a QP is connected to them then the kernel would setup the HW to match the PD, MRs, etc.

CQs and SRQs would still have to be created after an accpt/route_addr, but that seems pretty mild <shrug>

But that is of course not how things worked out, it would be possible to get there from here, but I doubt there is any interest.. It is late, I'm just musing what could have been :)

Still, it must be a total nightmare to write a daemon that listens on INADDR_ANY, works correctly with multiple devices plus hotplug, and dosn't leak PDs or something silly - and that is a shame.

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

You can redirect (REJ) a REQ to a different port, but you can't accept (REP) a connection on a different port.

- Sean

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

Do you have a compliance statment for that? :)

Spec says:

The initiator is responsible for proposing the Port Addresses (Primary and optional Alternate) that the target (REQ recipient) is to use for the channel. Based on the path defined by those port addresses, the initiator provides timeout information and the Service Level to be used by the target for any messages that it initiates. The SL from initiator to target need not be the same as from target to initiator, but the SL that is contained within the REQ message is the one that the initiator would prefer the target use. Path in- formation is available from Subnet Administration (see section 15.2.5.16 PathRecord).

'Proposing the Port Addresses' == Any Port On the Node, IMHO.

Someone went to alot of trouble to include enough unnecessary information to make that possible, and it is necessary for multi port APM.

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

The REP doesn't carry port (i.e. GID) information.

Under reject reasons:

12 Primary Remote Port GID rejected
The recipient of the REQ message could not (or would not) accept the Primary Remote Port GID
GID of acceptable port.

13 Primary Remote Port LID rejected
The recipient of the REQ message could not (or would not) accept the Primary Remote Port LID
LID of acceptable port.

I interpret this as the REQ carries the proposal. If the passive side aggress with the proposal, it accepts the connection, otherwise it rejects.

- Sean

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

Why would it need to? The REQ contains all the port information necessary, the active side doesn't get to pick the port, it just gets to approve it.

I agree.

But the proposal contains a complete description of the data carrying port (GUID, GID, LID) and there is *nothing* in the spec that says that description must match the port the GMP is received on.

Don't get confused by GMP redirection, that is yet again something else :)

So, IBA allows the active side to propose a data connection with primary path on Port 1, alternate path on Port 2 and send the GMP on Port 2.

The passive CM then looks at this and decides if it is within local policy constraints, if not it returns 12 or 13.

Thus, you cannot know what port the data QP will be bound to until you process the REQ - just like IP.

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

I'm not following you at all now. You said:

This is actually something of a mandatory notion to implement the full generality of the IB CM protocol which allows the CM REP to contain a port GUID of another port on the same node (multi-port APM is an IB feature). So you never know what port the accept() result will get bound to.

The REP does not contain a port GUID, so what exactly were you trying to say here?

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

Sorry, typo, it was too late. I ment REQ.

The point was, that you don't know what port the QP will be bound to until you get the REQ (ie the RDMA CM user does rdma_accept()). So early binding limits capabilities.

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

Jason, Have you even looked into or tested any of the bonding load-balancing modes with ipoib? some/most of them are not applicable to IPoIB and I don't think that the ones which may be such were ever tested. Next, multiple interfaces with the same ip address isn't something I see very useful for production environment (but I'd be happy to get educated what L3 bonding is and where it can play), next, multi-port APM isn't something I ever heard to be required by customers and more important, from comments made by Sean in the past, I don't think it fits the rdma-cm spirit.

All in all, someone comes here and suggests some fixes to the rdma-cm address resolution code to have IPv6 work. I don't think Dave should carry on his back/patch all your proposed future enhancements. Let him fix things and following that you can work on the patches to support all these nice/nitch features starting with IPv4 and then IPv6.

Or

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

I was saying that point in the rdmacm where the rdma_cm_id is bound to a local RDMA device should have only been rdma_resolve_addr and rdma_accept.

Overloading rdma_bind_addr to both bind to an IP and bind to an RDMA device was a bad API choice.

Sean is right, there may be special cases that require an early binding, but a seperate API - like IP's SO_BINDTODEVICE - would has been better - and users are forewarned that calling it restricts the environments their app will support :(

As it stands we have several impossible situations. Sean, Dave, and I were disucssing the trades offs of what this means relative to IP route resolution - but it affects bonding too.

If you rdma_bind_addr to the IP of a bonding device, the stack must pick one of the local RDMA ports immediately. If you then call rdma_listen there is a problem: incoming connections may target either RDMA device, but you are only bound to one of them.

An app cannot say 'I want to listen on this IP, any RDMA device' with the current API, as you can in IP, and that is a shame.

Traditionally with ethernet the L2 bonding is really only used for link aggregation, L1 failure, and a simple multi-switch HA scheme. It is not deployed if you have multiple ethernet domains. Some people prefer to have dual, independent ethernet fabrics, and in that case you rely on routing features to get the multipath, and HA features of bonding.

Go back on the list and look up the posts from Leo who first discovered this, what he was trying to do is kinda the L3 bonding approach.

I think multi-port APM can be fit into the API we have, but yes, there are some unfortunate design choices in verbs that make it a little awkward.

David has been doing a good job and I am glad he is working on the IPv6 support. My comments are only intended to clarify how this is all supposed to work and why the IP flow is actually still relevant to RDMA connections.

This (now quite long) discussion about RDMA device binding I really think has clarified what everyone expects from the API, and I've certainly found Sean's comments about early device binding quite interesting.

Unfortunately to make sin6_scope_id, and the loopback cases work properly does kinda require confronting these issues. The IPv6 notion of scope is new and it conflicts with the RDMA CM notion of 'scope' created by the local RDMA device binding. This is very different from how IP works. Reconciling the two ideas in a sane way is a big source of trouble.

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

As you wrote, for the most case, binding comes into play only for users calling rdma_resolve_address or rdma_accept, for users the need explicit binding the rdma-cm provides rdma_bind and it binds to both IP and Device, if you can do better, send a patch, binding can't be removed from the API since it has users and it makes sense from users to require it.

its just naming, will $ sed s/rdma_bind/rdma_set_opt(RDMA_BINDTODEVICE)/g make you happier? why?

Don't tell me that Dave's patches are blocked b/c you discovered the rdma_bind design and now you don't like it, as I wrote you, Dave sent patch to fix the IPv6 support, during the discussion on his patches you come and bring up more and more issues you consider as problems (but I don't) and block the patch set, I don't think this is appropriate. Let the patches go and send your patches to fix the problems you see. Why anyone touching some code piece has to fix problems you see in that piece?!

An app can say, I want to listen on that IP and the RDMA device which is associated with this IP now. When bonding does fail-over it generated a netevent, the rdma-cm catches this event and generates address change event, apps can redo their bind/listen at this point. For the time being, we never got a user report on a problem, people are doing listen on all IPs probably which works perfect with bonding. Currently the HA mode of bonding will respond on ARP only on one of the devices and as such connection requests will not target any rdma device but rather only the active one. If this is such a shame, send fix, spraying mud on the maintainer and/or someone sending another patch is a shame, isn't it?

okay, thanks for the crash course.

if Loe has a problem and you want to help him, bring it on the list, debate, send patches, jumping into someone's else patch isn't the constructive way to go.

As I see it, your comments block the the patches sent by Dave, Sean?

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

Wow, seriously? You do understand the purpose of review, right?

Documentation/development-process/7.AdvancedTopics:
Different developers will review code from different points of view. Some are mostly concerned with coding style and whether code lines have trailing white space. Others will focus primarily on whether the change implemented by the patch as a whole is a good thing for the kernel or not. Yet others will check for problematic locking, excessive stack usage, possible security issues, duplication of code found elsewhere, adequate documentation, adverse effects on performance, user-space ABI changes, etc. All types of review, if they lead to better code going into the kernel, are welcome and worthwhile.

And yes, actually, accounting for how rdma_bind() is different from bind() when doing route resolution is pretty much the main remaining problem.

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

I think I do, maybe not to the depth you and your arguments are, but again, repeating myself: my kind of simple argument is that your review is way beyond the --change-- suggested by a patch but rather of a whole logic, and you block a patch b/c you don't like the logic this patch integrates with. To some extent such practice is excepted, but you took it to way beyond acceptable limit. I don't accept your assertion that the whole logic is broken and it makes sense to me to have a patch from Dave to fix the IPv6 part of it. Next or in parallel you are welcome to sent a patch fixing/re-writing the whole bind logic or even the whole rdma stack or the whole kernel.

go and fix that

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