GNOME Bugzilla – Bug 705170
[review] th/ipv6-privacy: implement temporary addresses (aka privacy extensions)
Last modified: 2014-01-30 16:30:29 UTC
Implement temporary addresses (aka privacy extensions) that were left out when implementing router discovery in userspace (bug 699772).
For information, kernel code which generates temporary addresses is located in function __ipv6_regen_rndid in file net/ipv6/addrconf.c: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/ipv6/addrconf.c?id=8a226b2cfa776db6011fc84b71578513161cd3d3#n1853 This function is called by addrconf_prefix_rcv, itself called by ndisc_router_discovery in net/ipv6/ndisc.c. By the way, addrconf_prefix_rcv calls inet6_prefix_notify which sends an RTNLGRP_IPV6_PREFIX notification. Why NetworkManager is reimplementing kernel code instead of using the netlink interface? It may be because this interface is not so much stable or because there are missing notifications or anything else, but as I'm new and unfamiliar with this part of the networking code, these are just guesses.
There were very long talks about fixing and using the kernel interface but it proved (a) so difficult comparing to creating a userspace library and (b) so wrong to do in the kernel in the first place. If you're interested, I have written a summary about it: https://fedoraproject.org/wiki/Networking/Ideas/AutomaticConfiguration There you can see that it's not about small improvements to the Netlink interface. Unfortunately we don't have a central place for such documentation for NetworkManager but it might be worth starting it at some point of time. Thanks you for the links to the kernel implementation.
For reference, there is a downstream RedHat bug as well: https://bugzilla.redhat.com/show_bug.cgi?id=1003859 I am working on this now... the branch is called th/ipv6-privacy, but it's still WIP. But it could prove difficult to implement it without kernel changes. I opened RedHat kernel-bug https://bugzilla.redhat.com/show_bug.cgi?id=1021871 for that. I copy the following text from rhbz#1021871 for reference. It asks, "what is needed by the kernel", but it should help us to understand what NM has to do: >>>>>>>>>>>>>>> here some random thoughts and RFC: - the kernel performs DAD on the added addresses. For public (non temporary) addresses, it sets the flag to IFA_F_DADFAILED. But temporary addresses, get silently removed. Maybe we have to differentiate, between temporary addresses that got created/updated over netlink and kernel-generated once, and do not remove externally touched ones (to let the user-space application detect the DAD failure reliable). - Several places in net/ipv6/addrconf.c are with "#ifdef CONFIG_IPV6_PRIVACY", because it is unexpected that IFA_F_TEMPORARY is set externally. This might be correct in some places, but at least in ipv6_get_saddr_eval it is wrong (please check other places too). - For outgoing addresses the kernel should either prefer the public or the temporary address (depending on use_tempaddr). First question: let's say, the kernel should use the temporary address. It adds both the public and the temporary address at the same time and starts DAD. Does the kernel in this case ensure, that the public address is not ready before the temporary address (and as such will not be automatically chosen as source address?). I looked at the source and AFAIS, it does not ensure this. Seems shaky to me. Second question: how can a user space application avoid such a race? It could add the preferred address (e.g. the temporary one) in a first step, and only after that address survived DAD, set the other one (e.g. the public one). But this is also not too nice. - Is there a way to detect that we have a kernel with the changes that you are making now? So that NM can act in a fallback mode, to support older kernels? I think NetworkManager also needs to be smart to support older kernels without the comming patches. The following questions arise: - We cannot set temporary addresses, so we cannot influence which address the kernel will choose for outgoing connections (can we?). That is pretty bad and defeats the purpose of ip6-privacy. I don't see, what we can do about it(?) - Since we cannot add temporary addresses anyway, we will get a proper notification with IFA_F_DADFAILED. So to detect failed DAD is easy in this case.
(In reply to comment #3) > For reference, there is a downstream RedHat bug as well: > https://bugzilla.redhat.com/show_bug.cgi?id=1003859 Could that bug report be made publicly available? > I am working on this now... the branch is called th/ipv6-privacy, but it's > still WIP. I will definitely look at it. > But it could prove difficult to implement it without kernel changes. I opened > RedHat kernel-bug https://bugzilla.redhat.com/show_bug.cgi?id=1021871 for that. And that one? > I copy the following text from rhbz#1021871 for reference. It asks, "what is > needed by the kernel", but it should help us to understand what NM has to do: > > >>>>>>>>>>>>>>> > > here some random thoughts and RFC: > > - the kernel performs DAD on the added addresses. For public (non > temporary) addresses, it sets the flag to IFA_F_DADFAILED. But > temporary addresses, get silently removed. Very interesting, at the least. > Maybe we have > to differentiate, between temporary addresses that got created/updated > over netlink and kernel-generated once, and do not remove externally > touched ones (to let the user-space application detect the DAD > failure reliable). +1 > - Several places in net/ipv6/addrconf.c are with > "#ifdef CONFIG_IPV6_PRIVACY", because it is unexpected that > IFA_F_TEMPORARY is set externally. This might be correct in some > places, but at least in ipv6_get_saddr_eval it is wrong (please > check other places too). Sounds reasonable. It would be nice if you could post links when it's discussed in the kernel networking community (at least when there's some conclusion). > - For outgoing addresses the kernel should either prefer the > public or the temporary address (depending on use_tempaddr). Sounds reasonable to use `use_tempaddr` for that even when `accept_ra` is zero. > First question: let's say, the kernel should use the temporary > address. It adds both the public and the temporary address at the > same time and starts DAD. Does the kernel in this case ensure, that > the public address is not ready before the temporary address (and > as such will not be automatically chosen as source address?). I > looked at the source and AFAIS, it does not ensure this. Seems shaky > to me. Last time I tested the proper function of temporary addresses, it didn't work properly at all. When a new temporary address was added, it started to be used even for already existing TCP connections, breaking them effectively. I'm keeping some (although partially obsolete) notes on kernel TCP/IP networking problems here (feel free to edit): https://fedoraproject.org/wiki/Networking/Bugs#Kernel Kernel IPv6 networking is not yet ready for the real world except for basic setups. It's more about a good number of relatively easy issues than anything else, though. > Second question: how can a user space application avoid such a race? > It could add the preferred address (e.g. the temporary one) in a > first step, and only after that address survived DAD, set the other > one (e.g. the public one). But this is also not too nice. As IPv6 networking in Linux is by no means mature, I would consider fixing the necessary kernel bits the way to go. Especially if there's someone ready to do that. > - Is there a way to detect that we have a kernel with the changes that > you are making now? So that NM can act in a fallback mode, to support > older kernels? I'm quite curious about the fallback mode. > I think NetworkManager also needs to be smart to support older kernels > without the comming patches. The following questions arise: > > - We cannot set temporary addresses, so we cannot influence which address > the kernel will choose for outgoing connections (can we?). That is > pretty bad and defeats the purpose of ip6-privacy. I don't see, what > we can do about it(?) One way would be to require a minimum kernel version for IPv6 privacy address support, either in documentation, or even enforced by NetworkManager. Given there are numerous problems even with the kernel temporary addresses, that shouldn't be too bad. Sorry for leaving this unresearched when I was working on userspace router discovery. I was actually not aware of the problem with adding temporary addresses and distingushing them for source address selection purposes. Thank you for taking over.
Ok, now catch up on the current state... jpirko added a patch to the kernel [1], that extends the address ifa_flags to 32bit and adds a new flag IFA_F_MANAGETEMPADDR. This allows user space to add/change a /64 address, and the kernel will create and manage temporary addresses. NM will set accept_ra=0 to disable RA in kernel and set use_tempaddr to control source address selection. To update the lifetimes of temporary addresses, user space just modifies the exiting address with new lifetimes. The kernel should update the lifetimes, as if it would have received a RA update. Up to now, NM handles RA in userspace and adds addresses as /128 to avoid adding a prefix route for the /64 prefix if on-link=0. This might cause confusion for users [2] and it ~might~ be unexpected because autoconf addresses are for the /64 prefix. Now, with the new behavior for IFA_F_MANAGETEMPADDR, we have to add those addresses as /64. It might be possible to change those kernel patches to allow /128 addresses, but it's probably better to do the following instead: We will add the IFA_F_MANAGETEMPADDR address as /64 and add another kernel flag IFA_F_NOPREFIXROUTE that suppresses the creation of the prefix route [3]. All this needs features of the kernel and libnl. NetworkManager will detect, whether kernel+libnl supports the new flags. If not, it will continue adding the autoconf addresses as /128 (and of course, still no ipv6-privacy in this case). Sounds like a plan? [1] already in netdev, will probably be in 3.13. RHEL-7.0 kernel already patched. [2] bug 1045118, https://www.mail-archive.com/ipv6-ops@lists.cluenet.de/msg00809.html [3] currently discussed on netdev mailing list.
This sounds like a solid plan, +1 from me. Thanks!
+1 from me
(In reply to comment #5) > [1] already in netdev, will probably be in 3.13. RHEL-7.0 kernel already > patched. I got that wrong. Won't make it for 3.13, very likely 3.14. We can however patch RHEL-7 and Fedora20 kernel earlier. And here there is a related bug for RHEL-7 about /128 prefix: https://bugzilla.redhat.com/show_bug.cgi?id=1044590
Pushed branch th/ipv6-privacy for review. This also fixes adding autoconf addresses as /64 instead of /128. Kernel patches are now in net-dev/master and posted for RHEL-7. Patch for libnl is pushed to master, will be included in upcoming version 3.2.24. Please review.
> core: cleanup data types for nm_platform_sysctl_get_int32 > tmp = strtoul (value, &end, 10); That should be strtol() now shouldn't it? > core/rdisc: limit the number of autoconf addresses to max_addresses > Only be aware, that NM uses the > same config value as the kernel, but counts differently. Hm... I don't get this. If we're not counting the same way as the kernel, then doesn't that mean we may try to add more addresses then the kernel will actually allow? > core/platform: workaround new address flag in address_to_string > core/platform: add check_support_kernel_extended_ifa_flags function NMPlatform is supposed to be an abstraction, so talking specifically about "libnl" in the API is weird... You really only need a single method that checks for both libnl and kernel support anyway, since having one without the other is useless to us. (Maybe "nm_platform_supports_ifa_flag(flag)" so you can separately test NOPREFIXROUTE and MANAGETEMPADDR.) I don't like the _nm_platform_check_support_kernel_extended_ifa_flags_setup() thing either... can't you do that as part of nm-linux-platform.c:setup() ? > core/rdisc: add support for IPv6 privacy >+ "IPv6 private addresses. This feature is not avilable."); typo "avilable" (three times) >+ * we have no ipv6-privay and must add autoconf addresses "privacy"
(In reply to comment #10) > I don't like the _nm_platform_check_support_kernel_extended_ifa_flags_setup() > thing either... The new version you just pushed is much nicer
(In reply to comment #10) > > core: cleanup data types for nm_platform_sysctl_get_int32 > > > tmp = strtoul (value, &end, 10); > > That should be strtol() now shouldn't it? Correct > > core/rdisc: limit the number of autoconf addresses to max_addresses > > > Only be aware, that NM uses the > > same config value as the kernel, but counts differently. > > Hm... I don't get this. If we're not counting the same way as the kernel, then > doesn't that mean we may try to add more addresses then the kernel will > actually allow? Reworded commit message to answer this concern. To quote: Note, that the kernel uses 'max_addresses' only to limit public, autoconf addresses. So this limit does not affect NM adding as many addresses as it wants. > > core/platform: workaround new address flag in address_to_string > > core/platform: add check_support_kernel_extended_ifa_flags function > > NMPlatform is supposed to be an abstraction, so talking specifically about > "libnl" in the API is weird... You really only need a single method that checks > for both libnl and kernel support anyway, since having one without the other is > useless to us. (Maybe "nm_platform_supports_ifa_flag(flag)" so you can > separately test NOPREFIXROUTE and MANAGETEMPADDR.) Actually, the only reason of exposing both functions is to show more details in warn_support_extended_ifa_flags(). I agree, it's not a overly strong reason, but since it's just NM internal I'd be fine with this. I think, the abstraction is mainly to allow better testing (with the fake-platform stub), not to hide the fact that we ware using libnl or the linux kernel. This is not violated and if we need to test/stub these functions in the future, it can be easily re factored into virtual functions (actually, I already changed one of the functions into a virtual function). Thanks, repushed branch with fixes.
(In reply to comment #12) > > > core/rdisc: limit the number of autoconf addresses to max_addresses > Reworded commit message to answer this concern. To quote: OK, so actually, what was confusing me was that I thought max_addresses was a hard limit to the number of addresses that could be set on the device, but actually it's just a limit on the number of addresses that kernel autoconf will add, and there's no (kernel-enforced) limit on the number of addresses that userland can add. Right? So this is just about obeying a limit set by the admin, and the fact that we do the counting slightly differently from the kernel isn't a big deal. > I think, the abstraction is mainly to allow better testing (with the > fake-platform stub), not to hide the fact that we are using libnl or the linux > kernel. Yes, mostly, although it does make it at least theoretically more possible that someone could port NM to BSD too... Anyway, it's OK as is I guess.
Just fixed errors in _check_support_kernel_extended_ifa_flags_cb() and repushed the branch. I also did some additional testing, and it seems to work correctly.
> core/platform: workaround new address flag in address_to_string Whitespace/formatting in nm_platform_ip6_address_to_string() with the if() statements that are added... > core/platform: add check_support_kernel_extended_ifa_flags function In _check_support_kernel_extended_ifa_flags_cb() I would #define IFA_FLAGS if it's not already defined. That's clearer than "8 /* IFA_FLAGS */" to me, anyway. Could the checks be done more simply? It seems like a single nl_send_simple(priv->nlh_event, RTM_GETADDR) could be sent in setup() in nm-linux-platform.c so that event_notification() gets called for all the returned addresses, and then at the top of event_notification() just look for IFA_FLAGS. Thus the initial startup dump happens, and if any IPv6 address gets added after that the code would look for IFA_FLAGS too, all in one place. Then check_support_kernel_extended_ifa_flags() would simply return a boolean. That way we don't need the priv->support_kernel_extended_ifa_flags_tries because all addresses would only be checked once, at startup. Could that work? (I prototyped it quickly and it looks feasible, but I didn't check it in-depth. Also, you need to use priv->nlh_event for the initial check, because priv->nlh requires strict sequence checking and somehow this request screws that up...) > core/rdisc: add support for IPv6 privacy In print_support_extended_ifa_flags() it might be better to just say what libnl and the kernel don't support (eg, IFA_FLAGS). For the warn=2 and warn=1 levels, the kernel *does* support IPv6 private addresses, just not the way NetworkManager wants to use them. So perhaps all the messages should just say that "kernel and libnl don't support the IFA_FLAGS attribute, which is required for private IPv6 address support." Or something like that.
(In reply to comment #15) > > core/platform: workaround new address flag in address_to_string > > Whitespace/formatting in nm_platform_ip6_address_to_string() with the if() > statements that are added... Fixed > > core/platform: add check_support_kernel_extended_ifa_flags function > > In _check_support_kernel_extended_ifa_flags_cb() I would #define IFA_FLAGS if > it's not already defined. That's clearer than "8 /* IFA_FLAGS */" to me, > anyway. IFA_FLAGS is an enum value. So, this doesn't work. > Could the checks be done more simply? It seems like a single > nl_send_simple(priv->nlh_event, RTM_GETADDR) could be sent in setup() in > nm-linux-platform.c so that event_notification() gets called for all the > returned addresses, and then at the top of event_notification() just look for > IFA_FLAGS. Thus the initial startup dump happens, and if any IPv6 address gets > added after that the code would look for IFA_FLAGS too, all in one place. Then > check_support_kernel_extended_ifa_flags() would simply return a boolean. > > That way we don't need the priv->support_kernel_extended_ifa_flags_tries > because all addresses would only be checked once, at startup. Could that work? > > (I prototyped it quickly and it looks feasible, but I didn't check it in-depth. > Also, you need to use priv->nlh_event for the initial check, because priv->nlh > requires strict sequence checking and somehow this request screws that up...) The retry is there, because the detection only works if the kernel reports any IPv6 addresses -- and it fails, in the absence of such. If you suggest, not to retry (in setup()), this could also be removed here. When doing it in setup(), we either have to do it async or we have to wait. Waiting I think is again complicated and async means, that the result will only be known after a unspecified amount of time. Let's talk about this on IRC... > > core/rdisc: add support for IPv6 privacy > > In print_support_extended_ifa_flags() it might be better to just say what libnl > and the kernel don't support (eg, IFA_FLAGS). For the warn=2 and warn=1 > levels, the kernel *does* support IPv6 private addresses, just not the way > NetworkManager wants to use them. So perhaps all the messages should just say > that "kernel and libnl don't support the IFA_FLAGS attribute, which is required > for private IPv6 address support." Or something like that. Reworded.
(In reply to comment #16) > (In reply to comment #15) > > Could the checks be done more simply? It seems like a single > > nl_send_simple(priv->nlh_event, RTM_GETADDR) could be sent in setup() in > > nm-linux-platform.c so that event_notification() gets called for all the > > returned addresses, and then at the top of event_notification() just look for > > IFA_FLAGS. Thus the initial startup dump happens, and if any IPv6 address gets > > added after that the code would look for IFA_FLAGS too, all in one place. Then > > check_support_kernel_extended_ifa_flags() would simply return a boolean. > > > > That way we don't need the priv->support_kernel_extended_ifa_flags_tries > > because all addresses would only be checked once, at startup. Could that work? > > > > (I prototyped it quickly and it looks feasible, but I didn't check it in-depth. > > Also, you need to use priv->nlh_event for the initial check, because priv->nlh > > requires strict sequence checking and somehow this request screws that up...) > > The retry is there, because the detection only works if the kernel reports any > IPv6 addresses -- and it fails, in the absence of such. If you suggest, not to > retry (in setup()), this could also be removed here. > > When doing it in setup(), we either have to do it async or we have to wait. > Waiting I think is again complicated and async means, that the result will only > be known after a unspecified amount of time. > > Let's talk about this on IRC... Repushed a (much) simplified version as dcbw suggested.
Looks great, though instead of nl_send_simple(), could we save a few LOC and use: nl_rtgen_request(sk, RTM_GETADDR, AF_INET6, NLM_F_DUMP); in setup()?
(In reply to comment #18) > Looks great, though instead of nl_send_simple(), could we save a few LOC and > use: > > nl_rtgen_request(sk, RTM_GETADDR, AF_INET6, NLM_F_DUMP); > > in setup()? Changed, !fixup pushed
Looks good.
Merged branch upstream: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=3bb54e20849469eccca565b3e69a544541adcbaf (note, that you need support from kernel and libnl too. NetworkManager will log a message, telling you whether the system supports private addresses.)