GNOME Bugzilla – Bug 734149
[review] dcbw/ipv6-addrgenmode: generate IPv6LL address from NetworkManager
Last modified: 2014-09-19 18:57:15 UTC
See the second commit for the Big Explanation... This also requires some changes in libnl 3.2.25+ but the code is set up to handle falling back to the old behavior if that libnl suppport is not present. Thoughts on the general approach? Are there any corner-cases or pitfalls that I haven't thought of? I'm a bit wary since I thought the actual mechanics were going to be more complicated.
>> platform: add support for kernel IPv6LL address generation modes need to move mode inside #if block. Otherwise warns about unused variable. + NMPlatformLink device; + uint8_t mode; + +#if HAVE_LIBNL_INET6_ADDR_GEN_MODE + /* If we ever see a link with valid IPv6 link-local address + * generation modes, the kernel supports it. + */ + if ( priv->support_kernel_ipv6ll_addr_mode == FALSE + && rtnl_link_inet6_get_addr_gen_mode (rtnl_link,&mode) == 0) + priv->support_kernel_ipv6ll_addr_mode = TRUE; +#endif link_set_kernel_ipv6ll_addr_enabled(): should't it return FALSE in case of (!priv->support_kernel_ipv6ll_addr_mode)? And I think that link_get_kernel_ipv6ll_addr_enabled() should expect link_get() to return NULL. I would have: static gboolean link_get_kernel_ipv6ll_addr_enabled (NMPlatform *platform, int ifindex) { #if HAVE_LIBNL_INET6_ADDR_GEN_MODE NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE (platform); if (priv->support_kernel_ipv6ll_addr_mode) { auto_nl_object struct rtnl_link *rtnllink = link_get (platform, ifindex); uint8_t mode; if (rtnllink) { if (rtnl_link_inet6_get_addr_gen_mode (rtnllink, &mode) != 0) { /* Default to "enabled" on error */ return TRUE; } return mode == IN6_ADDR_GEN_MODE_EUI64; } #endif g_return_val_if_reached (TRUE); } static gboolean link_set_kernel_ipv6ll_addr_enabled (NMPlatform *platform, int ifindex, gboolean enabled) { #if HAVE_LIBNL_INET6_ADDR_GEN_MODE NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE (platform); if (priv->support_kernel_ipv6ll_addr_mode) auto_nl_object struct rtnl_link *change = _nm_rtnl_link_alloc (ifindex, NULL); guint8 mode; char buf[32]; mode = enabled ? IN6_ADDR_GEN_MODE_EUI64 : IN6_ADDR_GEN_MODE_NONE; rtnl_link_inet6_set_addr_gen_mode (change, mode); debug ("link: change %d: set IPv6 address generation mode to %s", ifindex, rtnl_link_inet6_addrgenmode2str (mode, buf, sizeof (buf))); return link_change (platform, ifindex, change); } #endif g_return_val_if_reached (FALSE); } btw. debug() evaluates the arguments now only if logging is enabled. No need for "if (nm_logging_enabled (LOGL_DEBUG, LOGD_PLATFORM))". >> core: take over IPv6LL address management if kernel supports it Jiri Pirko did a kernel patch for IPv6 LL address generation modes, which this NetworkManager patch makes use of. NetworkManager still keeps interfaces IFF_UP, but prevents the kernel from generating IPv6LL addresses for the interfaces, instead assinging its own ^^^^^^^^^ IPv6LL address as appropriate. could you mention the kernel commit ids? (net-next won't be rebased, so they are stable). gboolean nm_device_ipv6_sysctl_set (NMDevice *self, const char *property, const char *value) { + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + + if (priv->ip_ifindex <= 0 && priv->ifindex <= 0) + return FALSE; return nm_platform_sysctl_set (nm_utils_ip6_property_path (nm_device_get_ip_iface (self), property), value); } did this trigger an assertion for you? Maybe make a separate commit out of it? just wondering, why is this new mode accessibly via netlink, while otherwise we often use sysfs? if (!success) - nm_log_warn (LOGD_IP6, "(%s): failed to get interface identifier", nm_device_get_ip_iface (self)); + _LOGW (LOGD_IP6, "failed to get interface identifier"); + nm_platform_ip6_address_add (ip_ifindex, + lladdr, + in6addr_any, + 64, + NM_PLATFORM_LIFETIME_PERMANENT, + NM_PLATFORM_LIFETIME_PERMANENT, + IFA_F_MANAGETEMPADDR); why does it set IFA_F_MANAGETEMPADDR?
(In reply to comment #1) > >> platform: add support for kernel IPv6LL address generation modes > > > > need to move mode inside #if block. Otherwise warns about unused variable. > > + NMPlatformLink device; > + uint8_t mode; > + > +#if HAVE_LIBNL_INET6_ADDR_GEN_MODE > + /* If we ever see a link with valid IPv6 link-local address > + * generation modes, the kernel supports it. > + */ > + if ( priv->support_kernel_ipv6ll_addr_mode == FALSE > + && rtnl_link_inet6_get_addr_gen_mode (rtnl_link,&mode) == 0) > + priv->support_kernel_ipv6ll_addr_mode = TRUE; > +#endif Fixed. > I would have: > > > static gboolean > link_get_kernel_ipv6ll_addr_enabled (NMPlatform *platform, int ifindex) > { > *SNIP* > } > > static gboolean > link_set_kernel_ipv6ll_addr_enabled (NMPlatform *platform, int ifindex, > gboolean enabled) > { > *SNIP* > } I did what you suggest. > btw. debug() evaluates the arguments now only if logging is enabled. No need > for "if (nm_logging_enabled (LOGL_DEBUG, LOGD_PLATFORM))". Removed. > >> core: take over IPv6LL address management if kernel supports it > > Jiri Pirko did a kernel patch for IPv6 LL address generation modes, > which this NetworkManager patch makes use of. NetworkManager still > keeps interfaces IFF_UP, but prevents the kernel from generating > IPv6LL addresses for the interfaces, instead assinging its own > ^^^^^^^^^ > IPv6LL address as appropriate. > > could you mention the kernel commit ids? (net-next won't be rebased, so they > are stable). I updated the previous commit which adds the GENMODE support to the platform with the relevant kernel and libnl commit messages. I think that's actually the more relevant place than in nm-device.c. > gboolean > nm_device_ipv6_sysctl_set (NMDevice *self, const char *property, const char > *value) > { > + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); > + > + if (priv->ip_ifindex <= 0 && priv->ifindex <= 0) > + return FALSE; > return nm_platform_sysctl_set (nm_utils_ip6_property_path > (nm_device_get_ip_iface (self), property), value); > } > > did this trigger an assertion for you? Maybe make a separate commit out of it? Removed. I thought about it more and you're right, it's not required. > just wondering, why is this new mode accessibly via netlink, while otherwise we > often use sysfs? The option actually isn't exposed via procfs like the others are, it's only exposed via netlink. I think we should eventually transition the other stuff over to netlink too. > if (!success) > - nm_log_warn (LOGD_IP6, "(%s): failed to get interface identifier", > nm_device_get_ip_iface (self)); > + _LOGW (LOGD_IP6, "failed to get interface identifier"); Fixed. > + nm_platform_ip6_address_add (ip_ifindex, > + lladdr, > + in6addr_any, > + 64, > + NM_PLATFORM_LIFETIME_PERMANENT, > + NM_PLATFORM_LIFETIME_PERMANENT, > + IFA_F_MANAGETEMPADDR); > > why does it set IFA_F_MANAGETEMPADDR? I removed that part, you're right, MANAGETEMPADDR is only relevant for public IPv6 addresses. Repushed.
> platform: add support for kernel IPv6LL address generation modes >+ * nm_platform_link_get_kernel_ip6vll_addr_enabled: >+ * nm_platform_link_set_kernel_ip6vll_addr_enabled: These seem to have the constraint that you're not allowed to call them if nm_platform_check_support_kernel_ipv6ll_addr_disable() returns FALSE? (The linux implementations will hit an assertion if NM is built without HAVE_LIBNL_INET6_ADDR_GEN_MODE.) If that's correct, they should document that. Also, both function docs are missing "Returns:" It's also a bit awkward that nm_platform_check_support_kernel_ipv6ll_addr_disable() is not guaranteed to give the right answer at startup... It would be nice if NMLinuxPlatform's setup() could explicitly figure that out, like it does with IFA_FLAGS. > core: take over IPv6LL address management if kernel supports it So... while this will fix the case of users who want to manually assign IPv6 addresses, I believe it also makes it now impossible to get the kernel to do SLAAC when NM is running (other than by getting NM to unmanage the device, or writing a tool to change the addrgenmode yourself). I don't think that's a bad thing, but we're going to want to document that. (And it's another reason why we might want to keep "unmanaged" around...) (Related: do we still have to turn off accept_ra, etc?) >+check_and_add_ipv6ll_addr (NMDevice *self) >+ /* No IPv6 link-local address exists, but other addresses do. The comment is not correct. (eg, you'll get there if there are 0 addresses too.) >+ if (!nm_device_get_ip_iface_identifier (self, &iid)) >+ return; There should probably be some sort of "unable to add link-local address; IPv6 is probably broken" warning here. (Which means you probably don't want a warning inside nm_device_get_ip_iface_identifier(), since the different callers want different warnings, which may mean you don't even need that function.) >+set_disable_ipv6 (NMDevice *self, const char *value, gboolean disable_ipv6ll) This is confusing, and the two halves of it seem semi-unrelated. I think it would be clearer to just disable kernel ipv6ll generation when managing the device in _set_state_full(), and don't try to squash it into the same method as managing the disable_ipv6 sysctl. Also, you need to reset the addrgenmode when unmanaging the device.
(In reply to comment #3) > > platform: add support for kernel IPv6LL address generation modes > > >+ * nm_platform_link_get_kernel_ip6vll_addr_enabled: > >+ * nm_platform_link_set_kernel_ip6vll_addr_enabled: > > These seem to have the constraint that you're not allowed to call them if > nm_platform_check_support_kernel_ipv6ll_addr_disable() returns FALSE? (The > linux implementations will hit an assertion if NM is built without > HAVE_LIBNL_INET6_ADDR_GEN_MODE.) If that's correct, they should document that. You're right, I fixed that in a fixup commit (fixup! platform: add support for kernel IPv6LL address generation modes). Note that this reverts thomas' suggestion of using g_return_val_if_reached() in those functions in nm-linux-platform.c, but I think you're right and that things should fail gracefully if the platform/os doesn't support this feature. > Also, both function docs are missing "Returns:" Fixed in "fixup! platform: add support for kernel IPv6LL address generation modes" > It's also a bit awkward that > nm_platform_check_support_kernel_ipv6ll_addr_disable() is not guaranteed to > give the right answer at startup... It would be nice if NMLinuxPlatform's > setup() could explicitly figure that out, like it does with IFA_FLAGS. It's not explicit, but it's guaranteed to give the right answer by the time we've filled the link cache in nm-linux-platform::setup(), unless the system has no links at all yet. I've added an additional check to setup() right after allocating the link cache, which should give us the answer immediately. I left the original check too, just in case there are no links at all yet when NM starts up. Changes in a fixup. > > core: take over IPv6LL address management if kernel supports it > > So... while this will fix the case of users who want to manually assign IPv6 > addresses, I believe it also makes it now impossible to get the kernel to do > SLAAC when NM is running (other than by getting NM to unmanage the device, or > writing a tool to change the addrgenmode yourself). I don't think that's a bad > thing, but we're going to want to document that. (And it's another reason why > we might want to keep "unmanaged" around...) That tool actually already exists with /sbin/ip, jpirko did a patch to set (but apparently not read back) the genmode value. I'm not 100% against keeping unmanaged around, I just want to make sure that 99.9% of the reasons people have to use unmanaged are no longer valid. But anyway, where do you think we should document it? > (Related: do we still have to turn off accept_ra, etc?) Yeah, the ndisc/ra kernel code and the assign-LL address code are entirely separate, so if NM assigns the address, there are situations where the kernel would start RA. > >+check_and_add_ipv6ll_addr (NMDevice *self) > > >+ /* No IPv6 link-local address exists, but other addresses do. > > The comment is not correct. (eg, you'll get there if there are 0 addresses > too.) Fixed in a fixup commit. > >+ if (!nm_device_get_ip_iface_identifier (self, &iid)) > >+ return; > > There should probably be some sort of "unable to add link-local address; IPv6 > is probably broken" warning here. (Which means you probably don't want a > warning inside nm_device_get_ip_iface_identifier(), since the different callers > want different warnings, which may mean you don't even need that function.) Yeah, though since there's at least two callers right now, I feel like the accessor function is a bit cleaner. But I'm happy to remove it if you'd like. > >+set_disable_ipv6 (NMDevice *self, const char *value, gboolean disable_ipv6ll) > > This is confusing, and the two halves of it seem semi-unrelated. I think it > would be clearer to just disable kernel ipv6ll generation when managing the > device in _set_state_full(), and don't try to squash it into the same method as > managing the disable_ipv6 sysctl. I've changed this back and expanded it a bit in a fixup commit. See if you like that better. > Also, you need to reset the addrgenmode when unmanaging the device. Also in that same fixup commit.
last fixup commit still has: g_message ("#### support_user_ipv6ll %d", priv->support_user_ipv6ll); If we have "#if HAVE_LIBNL_INET6_ADDR_GEN_MODE" but no kernel support, this will always raise the warning nm_log_warn (LOGD_PLATFORM, "Unable to detect kernel support for IFLA_INET6_ADDR_GEN_MODE. Assume no kernel support."); because priv->support_user_ipv6ll doesn't ever get set to -1. I think you need: if (rtnl_link_inet6_get_addr_gen_mode (rtnl_link, &mode) == 0) priv->support_user_ipv6ll = 1; else priv->support_user_ipv6ll = -1;
You're right. I've fixed up the last fixup commit and repushed.
How about the follwoing in link_set_user_ipv6ll_enabled() and link_get_user_ipv6ll_enabled(): - if (priv->support_user_ipv6ll > 0) { + if (check_support_user_ipv6ll (platform)) { (it is not a real problem, because currently you always call: if (nm_platform_check_support_user_ipv6ll ()) nm_platform_link_set_user_ipv6ll_enabled (priv->ip_ifindex, TRUE); but still). The rest looks good
(In reply to comment #7) > How about the follwoing in link_set_user_ipv6ll_enabled() and > link_get_user_ipv6ll_enabled(): > > - if (priv->support_user_ipv6ll > 0) { > + if (check_support_user_ipv6ll (platform)) { Done.
Merged to git master.