| Message ID | 1376993766-5723-1-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Tue, 20 Aug 2013 12:16:06 +0200 > When an Xin6 tunnel is set up, we check other netdevices to inherit the link- > local address. If none is available, the interface will not have any link-local > address. RFC4862 expects that each interface has a link local address. > > Now than this kind of tunnels supports x-netns, it's easy to fall in this case > (by creating the tunnel in a netns where ethernet interfaces stand and then > moving it to a other netns where no ethernet interface is available). > > RFC4291, Appendix A suggests two methods: the first is the one currently > implemented, the second is to generate a unique identifier, so that we can > always generate the link-local address. Let's use eth_random_addr() to generate > this interface indentifier. > > I remove completly the previous method, hence for the whole life of the > interface, the link-local address remains the same (previously, it depends on > which ethernet interfaces were up when the tunnel interface was set up). > > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> Applied, but this brings up an issue I keep noticing. We talk about eth_random_addr() and "uniqueness" together all the time, but the former never implies the latter. And we're going to run into situations where any conflicts generated by this random address generater will cause reall failures. Therefore we'll have to create a system to prevent them. Probably using some simple table that keeps track of the addresses we've generated. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 21/08/2013 08:48, David Miller a écrit : > From: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Date: Tue, 20 Aug 2013 12:16:06 +0200 > >> When an Xin6 tunnel is set up, we check other netdevices to inherit the link- >> local address. If none is available, the interface will not have any link-local >> address. RFC4862 expects that each interface has a link local address. >> >> Now than this kind of tunnels supports x-netns, it's easy to fall in this case >> (by creating the tunnel in a netns where ethernet interfaces stand and then >> moving it to a other netns where no ethernet interface is available). >> >> RFC4291, Appendix A suggests two methods: the first is the one currently >> implemented, the second is to generate a unique identifier, so that we can >> always generate the link-local address. Let's use eth_random_addr() to generate >> this interface indentifier. >> >> I remove completly the previous method, hence for the whole life of the >> interface, the link-local address remains the same (previously, it depends on >> which ethernet interfaces were up when the tunnel interface was set up). >> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > Applied, but this brings up an issue I keep noticing. > > We talk about eth_random_addr() and "uniqueness" together all the > time, but the former never implies the latter. > > And we're going to run into situations where any conflicts generated > by this random address generater will cause reall failures. > > Therefore we'll have to create a system to prevent them. Probably > using some simple table that keeps track of the addresses we've > generated. > Ok, I will look at this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel <nicolas.dichtel@6wind.com> writes: > Le 21/08/2013 08:48, David Miller a écrit : > >> Applied, but this brings up an issue I keep noticing. >> >> We talk about eth_random_addr() and "uniqueness" together all the >> time, but the former never implies the latter. >> >> And we're going to run into situations where any conflicts generated >> by this random address generater will cause reall failures. >> >> Therefore we'll have to create a system to prevent them. Probably >> using some simple table that keeps track of the addresses we've >> generated. >> > Ok, I will look at this. Are eth_random_addr() collisions really any different than interfaces having the same address for other reasons? Reality is that we never can trust a mac address to be truly unique, regardless of source. And most of the time it doesn't matter. There is no problem with two interfaces having the same link local address as long as they are on different links, for example. What's important is that we deal with collisions gracefully in cases where they do matter, allowing an administrator to fix the issue. I believe we already do that on DAD failure with any mac address derived link local address, disabling IPv6 and logging the reason. Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 21/08/2013 11:02, Bjørn Mork a écrit : > Nicolas Dichtel <nicolas.dichtel@6wind.com> writes: >> Le 21/08/2013 08:48, David Miller a écrit : >> >>> Applied, but this brings up an issue I keep noticing. >>> >>> We talk about eth_random_addr() and "uniqueness" together all the >>> time, but the former never implies the latter. >>> >>> And we're going to run into situations where any conflicts generated >>> by this random address generater will cause reall failures. >>> >>> Therefore we'll have to create a system to prevent them. Probably >>> using some simple table that keeps track of the addresses we've >>> generated. >>> >> Ok, I will look at this. > > Are eth_random_addr() collisions really any different than interfaces > having the same address for other reasons? I would tend to say yes, it's different. It's easy for an administrator to fix a configuration for a physical interface, because it's statically configured and there is a limited number of interfaces. For virtual interfaces, they can be dynamically created and destroyed by daemons and we can have a lot of interfaces. Hence it could be hard to fix them. Trying to avoid these errors at kernel level could be useful. I've start to write a patch, and to test it I've just run a simple test which generate 1 000 000 of random addresses. I've run it several times (maybe not enough ;-)) and I never get a duplicated address... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel <nicolas.dichtel@6wind.com> writes: > Le 21/08/2013 11:02, Bjørn Mork a écrit : >> Nicolas Dichtel <nicolas.dichtel@6wind.com> writes: >>> Le 21/08/2013 08:48, David Miller a écrit : >>> >>>> Applied, but this brings up an issue I keep noticing. >>>> >>>> We talk about eth_random_addr() and "uniqueness" together all the >>>> time, but the former never implies the latter. >>>> >>>> And we're going to run into situations where any conflicts generated >>>> by this random address generater will cause reall failures. >>>> >>>> Therefore we'll have to create a system to prevent them. Probably >>>> using some simple table that keeps track of the addresses we've >>>> generated. >>>> >>> Ok, I will look at this. >> >> Are eth_random_addr() collisions really any different than interfaces >> having the same address for other reasons? > I would tend to say yes, it's different. > It's easy for an administrator to fix a configuration for a physical > interface, because it's statically configured and there is a limited > number of interfaces. > > For virtual interfaces, they can be dynamically created and destroyed > by daemons and we can have a lot of interfaces. Hence it could be hard > to fix them. If they are created by daemons then it should be up to the daemons to fix them. Or? > Trying to avoid these errors at kernel level could be useful. I strongly believe in fixing configuration issues in userspace if at all possible. You are setting a new policy every time you implement an automatic fix or workaround. It is so much better to keep that out of the kernel, or the next question you will face is "How do I change this policy? I want the addresses to be assigned by function Y" I see no reason why the daemon creating these interfaces can't also fixup any collisions. Or maybe better: If your daemon create millions of interfaces, and cares about unique addresses, then it should implement it's own address management. > I've start to write a patch, and to test it I've just run a simple > test which generate 1 000 000 of random addresses. I've run it several > times (maybe not enough ;-)) and I never get a duplicated address... Well, there are only 2^46 combinations so you are guaranteed to hit a collision if you just generate 70 368 744 177 665 random addresses :-) Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 21/08/2013 13:37, Bjørn Mork a écrit : > Nicolas Dichtel <nicolas.dichtel@6wind.com> writes: > >> Le 21/08/2013 11:02, Bjørn Mork a écrit : >>> Nicolas Dichtel <nicolas.dichtel@6wind.com> writes: >>>> Le 21/08/2013 08:48, David Miller a écrit : >>>> >>>>> Applied, but this brings up an issue I keep noticing. >>>>> >>>>> We talk about eth_random_addr() and "uniqueness" together all the >>>>> time, but the former never implies the latter. >>>>> >>>>> And we're going to run into situations where any conflicts generated >>>>> by this random address generater will cause reall failures. >>>>> >>>>> Therefore we'll have to create a system to prevent them. Probably >>>>> using some simple table that keeps track of the addresses we've >>>>> generated. >>>>> >>>> Ok, I will look at this. >>> >>> Are eth_random_addr() collisions really any different than interfaces >>> having the same address for other reasons? >> I would tend to say yes, it's different. >> It's easy for an administrator to fix a configuration for a physical >> interface, because it's statically configured and there is a limited >> number of interfaces. >> >> For virtual interfaces, they can be dynamically created and destroyed >> by daemons and we can have a lot of interfaces. Hence it could be hard >> to fix them. > > If they are created by daemons then it should be up to the daemons to > fix them. Or? > >> Trying to avoid these errors at kernel level could be useful. > > I strongly believe in fixing configuration issues in userspace if at all > possible. You are setting a new policy every time you implement an > automatic fix or workaround. It is so much better to keep that out of > the kernel, or the next question you will face is "How do I change this > policy? I want the addresses to be assigned by function Y" > > I see no reason why the daemon creating these interfaces can't also > fixup any collisions. Or maybe better: If your daemon create millions > of interfaces, and cares about unique addresses, then it should > implement it's own address management. Ok ok, you convince me ;-) I will wait David feedback. > >> I've start to write a patch, and to test it I've just run a simple >> test which generate 1 000 000 of random addresses. I've run it several >> times (maybe not enough ;-)) and I never get a duplicated address... > > Well, there are only 2^46 combinations so you are guaranteed to hit a > collision if you just generate 70 368 744 177 665 random addresses :-) Yes, it was just to say that the function which generate these addresses has a "good" entropy ;-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index ad12f7c43f25..5125436ca67e 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -1814,6 +1814,16 @@ static int addrconf_ifid_gre(u8 *eui, struct net_device *dev) return __ipv6_isatap_ifid(eui, *(__be32 *)dev->dev_addr); } +static int addrconf_ifid_ip6tnl(u8 *eui, struct net_device *dev) +{ + memcpy(eui, dev->perm_addr, 3); + memcpy(eui + 5, dev->perm_addr + 3, 3); + eui[3] = 0xFF; + eui[4] = 0xFE; + eui[0] ^= 2; + return 0; +} + static int ipv6_generate_eui64(u8 *eui, struct net_device *dev) { switch (dev->type) { @@ -1832,6 +1842,8 @@ static int ipv6_generate_eui64(u8 *eui, struct net_device *dev) return addrconf_ifid_eui64(eui, dev); case ARPHRD_IEEE1394: return addrconf_ifid_ieee1394(eui, dev); + case ARPHRD_TUNNEL6: + return addrconf_ifid_ip6tnl(eui, dev); } return -1; } @@ -2709,7 +2721,8 @@ static void addrconf_dev_config(struct net_device *dev) (dev->type != ARPHRD_ARCNET) && (dev->type != ARPHRD_INFINIBAND) && (dev->type != ARPHRD_IEEE802154) && - (dev->type != ARPHRD_IEEE1394)) { + (dev->type != ARPHRD_IEEE1394) && + (dev->type != ARPHRD_TUNNEL6)) { /* Alas, we support only Ethernet autoconfiguration. */ return; } @@ -2795,44 +2808,6 @@ ipv6_inherit_linklocal(struct inet6_dev *idev, struct net_device *link_dev) return -1; } -static void ip6_tnl_add_linklocal(struct inet6_dev *idev) -{ - struct net_device *link_dev; - struct net *net = dev_net(idev->dev); - - /* first try to inherit the link-local address from the link device */ - if (idev->dev->iflink && - (link_dev = __dev_get_by_index(net, idev->dev->iflink))) { - if (!ipv6_inherit_linklocal(idev, link_dev)) - return; - } - /* then try to inherit it from any device */ - for_each_netdev(net, link_dev) { - if (!ipv6_inherit_linklocal(idev, link_dev)) - return; - } - pr_debug("init ip6-ip6: add_linklocal failed\n"); -} - -/* - * Autoconfigure tunnel with a link-local address so routing protocols, - * DHCPv6, MLD etc. can be run over the virtual link - */ - -static void addrconf_ip6_tnl_config(struct net_device *dev) -{ - struct inet6_dev *idev; - - ASSERT_RTNL(); - - idev = addrconf_add_dev(dev); - if (IS_ERR(idev)) { - pr_debug("init ip6-ip6: add_dev failed\n"); - return; - } - ip6_tnl_add_linklocal(idev); -} - static int addrconf_notify(struct notifier_block *this, unsigned long event, void *ptr) { @@ -2900,9 +2875,6 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event, addrconf_gre_config(dev); break; #endif - case ARPHRD_TUNNEL6: - addrconf_ip6_tnl_config(dev); - break; case ARPHRD_LOOPBACK: init_loopback(dev); break; diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c index cc3bb201b8b0..d6e00a39274c 100644 --- a/net/ipv6/ip6_tunnel.c +++ b/net/ipv6/ip6_tunnel.c @@ -41,6 +41,7 @@ #include <linux/netfilter_ipv6.h> #include <linux/slab.h> #include <linux/hash.h> +#include <linux/etherdevice.h> #include <asm/uaccess.h> #include <linux/atomic.h> @@ -1471,6 +1472,9 @@ static void ip6_tnl_dev_setup(struct net_device *dev) dev->flags |= IFF_NOARP; dev->addr_len = sizeof(struct in6_addr); dev->priv_flags &= ~IFF_XMIT_DST_RELEASE; + /* This perm addr will be used as interface identifier by IPv6 */ + dev->addr_assign_type = NET_ADDR_RANDOM; + eth_random_addr(dev->perm_addr); }
When an Xin6 tunnel is set up, we check other netdevices to inherit the link- local address. If none is available, the interface will not have any link-local address. RFC4862 expects that each interface has a link local address. Now than this kind of tunnels supports x-netns, it's easy to fall in this case (by creating the tunnel in a netns where ethernet interfaces stand and then moving it to a other netns where no ethernet interface is available). RFC4291, Appendix A suggests two methods: the first is the one currently implemented, the second is to generate a unique identifier, so that we can always generate the link-local address. Let's use eth_random_addr() to generate this interface indentifier. I remove completly the previous method, hence for the whole life of the interface, the link-local address remains the same (previously, it depends on which ethernet interfaces were up when the tunnel interface was set up). Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- net/ipv6/addrconf.c | 56 +++++++++++++-------------------------------------- net/ipv6/ip6_tunnel.c | 4 ++++ 2 files changed, 18 insertions(+), 42 deletions(-)