link-local address fix for rdma_resolve_addr

Here is a patch to addr6_resolve_remote() to correctly handle link-local address. It should cover all the conditions Jason described.

With this patch rping works as expected.

Link-local with scope:

# /usr/bin/rping -c -a fe80::202:c903:1:1925%ib0

Link-local w/out scope:

# /usr/bin/rping -c  -a fe80::202:c903:1:1925
rdma_resolve_addr error -1

Other ipv6 address:

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

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

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

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

Looks pretty good to me, definitely on the right track.

Hmm..

Actually, upon comparing to tcp_ipv6.c, I'd say one more behavior is necessary. The code in tcp_ipv6 allows the destination to not specify a scope id if an interface has already been set. Looks like the two ways to set an interface ID are to use bind() or SO_BINDTODEVICE..

Specifying a source address to RDMA CM is similar to bind(), so if the source address is link local it must specify a sin6_scope_id and the dest address can specify 0, or the same value.

How af_inet6 handles bind():

                        if (addr_type & IPV6_ADDR_LINKLOCAL) {
                                if (addr_len >= sizeof(struct sockaddr_in6) &&
                                    addr->sin6_scope_id) {
                                        /* Override any existing binding, if another one
                                         * is supplied by user.
                                         */
                                        sk->sk_bound_dev_if = addr->sin6_scope_id;
                                }

                                /* Binding to link-local address requires an interface */
                                if (!sk->sk_bound_dev_if) {
                                        err = -EINVAL;
                                        goto out;
                                }
                                dev = dev_get_by_index(net, sk->sk_bound_dev_if);
                                if (!dev) {
                                        err = -ENODEV;
                                        goto out;
                                }
                        }

How tcp_ipv6 checks the destination address:

        if (addr_type&IPV6_ADDR_LINKLOCAL) {
                if (addr_len >= sizeof(struct sockaddr_in6) &&
                    usin->sin6_scope_id) {
                        /* If interface is set while binding, indices
                         * must coincide.
                         */
                        if (sk->sk_bound_dev_if &&
                            sk->sk_bound_dev_if != usin->sin6_scope_id)
                                return -EINVAL;

                        sk->sk_bound_dev_if = usin->sin6_scope_id;
                }

                /* Connect to link-local address requires an interface */
                if (!sk->sk_bound_dev_if)
                        return -EINVAL;
        }

Jason

Sean-
This patch should fix the behavior of rdma_resolve_addr when using link-local addressing.

Jason -
This new patch should closely emulate tcp_ipv6.c. when both source and destination scope_ids are given with link-local address.

With this patch rping works as expected.

Link-local with scope:

# /usr/bin/rping -c -a fe80::202:c903:1:1925%ib0

Link-local w/out scope:

# /usr/bin/rping -c  -a fe80::202:c903:1:1925
rdma_resolve_addr error -1

Other ipv6 address:

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

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

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

@@ -278,6 +278,21 @@ static int addr6_resolve_remote(struct s
 	fl.nl_u.ip6_u.daddr = dst_in->sin6_addr;
 	fl.nl_u.ip6_u.saddr = src_in->sin6_addr;
 
+	if (ipv6_addr_type(&dst_in->sin6_addr) & IPV6_ADDR_LINKLOCAL){
+		// Link-local address require an interface to be specified.
+		if (!(dst_in->sin6_scope_id||src_in->sin6_scope_id))
+			return -EINVAL;
+
+		// If src and dst interfaces are supplied they must match.
+		if ( (dst_in->sin6_scope_id && src_in->sin6_scope_id) &&
+			(src_in->sin6_scope_id != dst_in->sin6_scope_id) )
+				return -EINVAL;
+		if ( dst_in->sin6_scope_id )
+			fl.oif = dst_in->sin6_scope_id;
+		else
+			fl.oif = src_in->sin6_scope_id;
+	}
+
 	dst = ip6_route_output(&init_net, NULL, &fl);
 	if (!dst)
 		return ret;

Maybe like this:

        fl.oif = 0;

	if (ipv6_addr_type(&src_in->sin6_addr) & IPV6_ADDR_LINKLOCAL) {
	    if (!src_in->sin6_scope_id)
                return -EINVAL;
            fl.oif = src_in->sin6scope_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;
	}

src and dest have to be tested seperately, the TCPv6 versions don't require them to be both link local.

Don't forget to run checkpatch ;)

Regards,

This looks good. Once concern, it may be obtuse, but if both the src and dst are link-local addresses should only one need to be scoped? This patch will required the src to always be scoped when using link local.

Dave...

The TCPv6 code requires the src to be scoped (or SO_BINDTODEVICE to be used prior). If src is scoped then it is optional to scope dest, but if dest is scoped then it must match. I intended to capture that logic..

Can you give a quick test and send a git format-patch/checkpatch'd patch to Roland, with your signed off line, etc. You can put a Reviewed-by line from me as well if you like.

Thanks,
Jason

I found the bug in addr_resolve_local(). (more comments below)

@@ -390,14 +390,17 @@ static int addr_resolve_local(struct soc
 	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 +409,8 @@ static int addr_resolve_local(struct soc
 			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);

Sorry I don't get it..
Are you saying that ip6_route_output() will resolve the address even if it is a link-local address bound to my own interface? Therefor addr_resolve_local() is not needed.

Yes, that is the hacky work around I was mentioning..

Yes, and more. In Linux the routing table takes as input the source (optional), device (optional) and destination address and returns as output the device to use.

To determine the device to bind to you ask the routing table what device to use for all the route information you have.

For example:

$ ip route get fe80::c2d from fe80::213:72ff:fe29:e65d oif eth0
fe80::c2d via fe80::c2d dev eth0  src fe80::213:72ff:fe29:e65d  metric 0 
    cache  mtu 1500 advmss 1440 hoplimit 4294967295

$ ip route get fe80::c2d oif eth0
fe80::c2d via fe80::c2d dev eth0  src fe80::213:72ff:fe29:e65d  metric 0 
    cache  mtu 1500 advmss 1440 hoplimit 4294967295

You can see in both cases the routing table returns a 'src' entry. 'src' is the address to bind to if no bind address was specified.

When doing link local addresess the sin6_scope_id should sets the 'oif' key in the routing lookup, which will result in the correct src address and output device being selected by the routing algorithm. For instance on my machine here, I have two interfaces:

$ ip route get fe80::c2d oif virbr0 
fe80::c2d via fe80::c2d dev virbr0  src fe80::2c5d:c4ff:feb8:1ce5  metric 0 
    cache  mtu 1500 advmss 1440 hoplimit 4294967295

As you can see it is returning the link local address for virbr0 as the source.

So the algorithm in RDMA CM should look like this:
- If src is specified then set the bind local address to src [if src is link local then it must specify sin6_scope_id, and sin6_scope_id becomes the oif input to the route lookup]
- If dst is link local then its sin6_scope_id is the oif to the route lookup (and must match src, as we did last go round)
- Src (or 'any'), dst and device (or 'any') are passed to the route lookup
- The RDMA CM ID is bound to the device returned by the route lookup
- If the src address was not specified then the connection source IP is set to the 'src' value from the route lookup.

This is why addr_resolve_local/rdma_translate_ip is not needed, that entire entire function is done by the routing table code.

You can see why this becomes important when it is combined with policy routing, for instance consider this example:

$ ip rule
32765:  from 10.0.0.4 lookup dnat
$ ip route show table dnat
default via 10.0.0.1 dev eth1
$ ip route get 10.0.0.100
10.0.0.100 dev eth0  src 10.0.0.2 
$ ip route get 10.0.0.100 from 10.0.0.4
10.0.0.100 from 10.0.0.4 via 10.0.0.1 dev eth1
    cache  mtu 1500 advmss 1460 hoplimit 64

The two results are radically different and dependant on the source address. (10.0.0.4 could be attached to eth0, and eth1!)

The actual fixing to the code is not hard, remove rdma_translate_ip, addr_resolve_local, split addr_resolve_remote into a part to resolve the route and a part that does the arp/nd. Make the route resolve part work almost exactly like addr4_resolve_remote (noting that the v6 version is wrong, since is doesn't respect unset source addres, another bug). Call rdma_copy_addr based on the rt->idev->dev (or should it be odev??). Do the ARP.

The pain is in retesting everything :|

Jason

There is the wrinkle in this plan. If you pass ip6_route_output() your own link-local address (scoped or not) it returns a neighbor entry bound to the loop back device.

$ ip route get fe80::202:c903:1:28ed oif ib0
local fe80::202:c903:1:28ed from :: via :: dev lo  table local  proto
none  src fe80::202:c903:1:28ed  metric 0  mtu 16436 advmss 16376
hoplimit 4294967295

Thats not going to work as lo has no RDMA support. I suspect this is why addr_resolve_local() was added in the first place :)

I will post a patch with our first solution and include the fix to addr_resolve_local().

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

Well, that really is correct. The RDMA CM could try to do something smart if lo is returned from the route lookup - ie choose the first RDMA device bound to that IP or something.

There are other problems with the loop back case, like the Linux stack doesn't generate neighbor entries for its own IP addresses.

The loopback case is far less important than the other policy routing features anyhow..

May as well, a little bit of progress is better than none at all.

Also note that I observed that the IPv6 code path never sets the source address from the routing table if IN6_ADDR_ANY is used as source, this is certainly wrong and could cause other problems down the line, probably more so for iwarp than IB though..

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

Hum.. this change is not working with rping. The src address has been filled in already with a link-local address and no scope_id so it is returning -EINVAL. Another bug I guess.... I am doing some more debug.

Ah feh, you are running into that other bug..

http://lists.openfabrics.org/pipermail/general/2009-July/060612.html

Basically it does this:

int rdma_resolve_addr(struct rdma_cm_id *id, struct sockaddr *src_addr,

                      struct sockaddr *dst_addr, int timeout_ms)
                ret = rdma_resolve_ip(&addr_client, (struct sockaddr *) &id->route.addr.src_addr,
                                      dst_addr, &id->route.addr.dev_addr,
                                      timeout_ms, addr_handler,
				      id_priv);

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, void (*callback)(int status, struct sockaddr *src_addr,
struct rdma_dev_addr *addr, void *context),
void *context)
req->status = addr_resolve_local(src_in, dst_in, addr);

static int addr_resolve_local(struct sockaddr *src_in,
struct sockaddr *dst_in, struct rdma_dev_addr *addr)
dev = ip_dev_find(&init_net, dst_ip);
[..]

       for_each_netdev(&init_net, dev)
                        if (ipv6_chk_addr(&init_net,
                                          &((struct sockaddr_in6 *) addr)->sin6_addr,
                                          dev, 1))
                                break;

So, it tries to match the source addr to the addrs bound to the device, which is wrong - that isn't how the ip stack works.

You can patch this up a little bit by fixing up addr_resolve_local to set sin6_scope_ip.

But really the correct thing to do is to remove addr_resolve_local and place the source address into the struct flowi and use the result of the route lookup to bind to the source device, and set the source address if it is unset.

Jason