| Message ID | 20200812001449.28401-1-iwienand@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | network: allow accept_ra == 0 when enabling ipv6 forwarding | expand |
Yay! A user who follows up their conversation on the libvirt-users list with a patch! :-) On 8/11/20 8:14 PM, Ian Wienand wrote: > The checks modified here were added with > 00d28a78b5d1f6eaf79f06ac59e31c568af9da37 to avoid losing routes on > hosts. I'm Cc'ing the author of that patch, Cédric Bosdonnat, to make sure that whatever we end up doing doesn't upset his use case. > > However, tools such as systemd-networking and NetworkManager manage > RA's in userspace and thus IPv6 may be up and working on an interface > even with accept_ra == 0. > > This modifies the check to only error if an interface's accept_ra is > already set to "1"; as noted inline this seems to when it is likely > that enabling forwarding may change the RA acceptance behaviour of the > interface. Unfortunately, on my Fedora 32 machine that has NetworkManager enabled, not all the interfaces have accept_ra=0 by default. One of the bridge devices (created by NetworkManager in response to an ifcfg file in /etc/sysconfig/network-scripts) has accept_ra = 1. (there are several other interfaces that have accept_ra=1, but those interfaces are either not online, or they don't have an IPv6 address. So this doesn't work for all cases. I think we need to get more information on how to most easily, generically, and reliably determine if the kernel accept_ra setting can be ignored. Possibly the NetworkManager people can help us out here. (or alternately we could punt and just make a configuration setting, although that is taking the easy road, and not a good precedent to set) > > I have noticed this because I am using the IPv6 NAT features enabled > with 927acaedec7effbe67a154d8bfa0e67f7d08e6c7. I am using this on my > laptop which switches between wired and wireless connections; both of > which are configured in an unremarkable way by Fedora's NetworkManager > and get configured for IPv6 via SLAAC and whatever NetworkManager > magic it does. With this I can define and start a libvirt network > with <nat ipv6="yes"> and <ip family='ipv6' > address='fc00:abcd:ef12:34::' prefix='64'> and it seems to "just work" > for guests. > > Signed-off-by: Ian Wienand <iwienand@redhat.com> > --- > src/util/virnetdevip.c | 41 +++++++++++++++++++++++++++-------------- > 1 file changed, 27 insertions(+), 14 deletions(-) > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > index 409f062c5c..de27cacfc9 100644 > --- a/src/util/virnetdevip.c > +++ b/src/util/virnetdevip.c > @@ -496,7 +496,7 @@ virNetDevIPGetAcceptRA(const char *ifname) > } > > struct virNetDevIPCheckIPv6ForwardingData { > - bool hasRARoutes; > + bool hasKernelRARoutes; > > /* Devices with conflicting accept_ra */ > char **devices; > @@ -552,15 +552,26 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, > if (!ifname) > return -1; > > - accept_ra = virNetDevIPGetAcceptRA(ifname); > - > VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d", > ifname, ifindex, accept_ra); > > - if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > + accept_ra = virNetDevIPGetAcceptRA(ifname); > + /* 0 = do no accept RA > + * 1 = accept if forwarding disabled > + * 2 = ovveride and accept RA when forwarding enabled > + * > + * When RA is managed by userspace (systemd-networkd or > + * NetworkManager) accept_ra is unset and we don't need to > + * worry about it. If it is 1, enabling forwarding might > + * change the behaviour so the user needs to be warned. > + */ > + if (accept_ra == 0) > + return 0; > + > + if (accept_ra == 1 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > return -1; > > - data->hasRARoutes = true; > + data->hasKernelRARoutes = true; > return 0; > } > > @@ -590,11 +601,13 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, > VIR_DEBUG("Checking multipath route nexthop device %s (%d), accept_ra: %d", > ifname, nh->rtnh_ifindex, accept_ra); > > - if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > - return -1; > + if (accept_ra == 1) { > + if (virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > + return -1; > + data->hasKernelRARoutes = true; > + } > > VIR_FREE(ifname); > - data->hasRARoutes = true; > > len -= NLMSG_ALIGN(nh->rtnh_len); > VIR_WARNINGS_NO_CAST_ALIGN > @@ -613,7 +626,7 @@ virNetDevIPCheckIPv6Forwarding(void) > struct rtgenmsg genmsg; > size_t i; > struct virNetDevIPCheckIPv6ForwardingData data = { > - .hasRARoutes = false, > + .hasKernelRARoutes = false, > .devices = NULL, > .ndevices = 0 > }; > @@ -644,11 +657,11 @@ virNetDevIPCheckIPv6Forwarding(void) > goto cleanup; > } > > - valid = !data.hasRARoutes || data.ndevices == 0; > + valid = !data.hasKernelRARoutes || data.ndevices == 0; > > /* Check the global accept_ra if at least one isn't set on a > per-device basis */ > - if (!valid && data.hasRARoutes) { > + if (!valid && data.hasKernelRARoutes) { > int accept_ra = virNetDevIPGetAcceptRA(NULL); > valid = accept_ra == 2; > VIR_DEBUG("Checked global accept_ra: %d", accept_ra); > @@ -663,9 +676,9 @@ virNetDevIPCheckIPv6Forwarding(void) > } > > virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Check the host setup: enabling IPv6 forwarding with " > - "RA routes without accept_ra set to 2 is likely to cause " > - "routes loss. Interfaces to look at: %s"), > + _("Check the host setup: interface has accept_ra set to 1 " > + "and enabling forwarding without accept_ra set to 2 is " > + "likely to cause routes loss. Interfaces to look at: %s"), > virBufferCurrentContent(&buf)); > } >
On Wed, Aug 12, 2020 at 07:21:14PM -0400, Laine Stump wrote: > Yay! A user who follows up their conversation on the libvirt-users list with > a patch! :-) Heh, ipv6 in my work VM is my white whale, so, call me Ishmael :) > Unfortunately, on my Fedora 32 machine that has NetworkManager enabled, not > all the interfaces have accept_ra=0 by default. One of the bridge devices > (created by NetworkManager in response to an ifcfg file in > /etc/sysconfig/network-scripts) has accept_ra = 1. (there are several other > interfaces that have accept_ra=1, but those interfaces are either not > online, or they don't have an IPv6 address. Interesting, from a cursory search the only place NetworkManager seems to set accept_ra to 1 is [1] for ip tokens (TIL ...) and then that seems to turn if off again when the interface is enabled. But you know, that's also 18,000+ lines of logic in there ... so ... >From my memory of looking at the ifcfg-* files path it's a plugin for NM that reads those files and basically passes them as config into NM "core". So it doesn't do anything particularly magic. However I'm reminded of a bug that took us a very long time to diagnose with "glean", a tool to configure interfaces from config-drive data in OpenStack clouds. Some clouds don't do DHCP, so it is used to read the config-drive data set by the cloud and write out the network config. It had a long history, before systemd udev activation was a thing, an was calling "ip link set dev <interface> up" on the interface and then seeing if it had a carrier. This would enable accept_ra == 1 and the kernel would configure an IPv6 address; however since the interface then looked configured, and NetworkManager would ignore it, and thus not apply the IPv4 configuration we had written out. Hosts came up with an IPv6 but no IPv4; it was all very confusing till we put it together. My long winded point being that *maybe* something else is getting in the way ... > So this doesn't work for all cases. So the intent at least is to do this warning iff the existing interface is already set to "1". So if you did put that bridge into the VM network and we enabled forwarding on it, that would seem to change it's behaviour and possibly break it? > I think we need to get more information on how to most easily, > generically, and reliably determine if the kernel accept_ra setting > can be ignored. Possibly the NetworkManager people can help us out > here. ++ to that, if the correct response is to error, or automatically set it to "2", or ignore it are all open questions :) -i [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/52af5e901e4e5e7727ae83db18a37730b5f898fe/src/devices/nm-device.c#L1553 [2] https://review.opendev.org/#/c/688031/
On Wed, 2020-08-12 at 19:21 -0400, Laine Stump wrote: > Yay! A user who follows up their conversation on the libvirt-users list > with a patch! :-) > > On 8/11/20 8:14 PM, Ian Wienand wrote: > > The checks modified here were added with > > 00d28a78b5d1f6eaf79f06ac59e31c568af9da37 to avoid losing routes on > > hosts. > > I'm Cc'ing the author of that patch, Cédric Bosdonnat, to make sure that > whatever we end up doing doesn't upset his use case. Ouch! that is old... even reading my bugzilla log I have troubles explaining that thing properly. I'll try it though. So the hypervisor has at least one (Router Advertised) RA route. After defining a network like the following, the RA route is removed if accept_ra isn't set to 2. <network ipv6="yes"> <name>test5</name> <forward mode="nat"/> <bridge name="708837c1d27-br0" stp="off"/> <mac address="52:54:00:45:5f:27"/> <ip family="ipv6" address="fc00:0000:0000:000f:0000:0000:0000:0001" prefix="64"/> </network> The RA route was removed in networkEnableIPForwarding() when setting /proc/sys/net/ipv6/conf/all/forwarding to 1. Me not being a network expert (and even less on ipv6) doesn't help. I hope this explanation will help you better see the use case I had. -- Cedric > > However, tools such as systemd-networking and NetworkManager manage > > RA's in userspace and thus IPv6 may be up and working on an interface > > even with accept_ra == 0. > > > > This modifies the check to only error if an interface's accept_ra is > > already set to "1"; as noted inline this seems to when it is likely > > that enabling forwarding may change the RA acceptance behaviour of the > > interface. > > Unfortunately, on my Fedora 32 machine that has NetworkManager enabled, > not all the interfaces have accept_ra=0 by default. One of the bridge > devices (created by NetworkManager in response to an ifcfg file in > /etc/sysconfig/network-scripts) has accept_ra = 1. (there are several > other interfaces that have accept_ra=1, but those interfaces are either > not online, or they don't have an IPv6 address. > > > So this doesn't work for all cases. I think we need to get more > information on how to most easily, generically, and reliably determine > if the kernel accept_ra setting can be ignored. Possibly the > NetworkManager people can help us out here. > > > (or alternately we could punt and just make a configuration setting, > although that is taking the easy road, and not a good precedent to set) > > > > I have noticed this because I am using the IPv6 NAT features enabled > > with 927acaedec7effbe67a154d8bfa0e67f7d08e6c7. I am using this on my > > laptop which switches between wired and wireless connections; both of > > which are configured in an unremarkable way by Fedora's NetworkManager > > and get configured for IPv6 via SLAAC and whatever NetworkManager > > magic it does. With this I can define and start a libvirt network > > with <nat ipv6="yes"> and <ip family='ipv6' > > address='fc00:abcd:ef12:34::' prefix='64'> and it seems to "just work" > > for guests. > > > > Signed-off-by: Ian Wienand <iwienand@redhat.com> > > --- > > src/util/virnetdevip.c | 41 +++++++++++++++++++++++++++-------------- > > 1 file changed, 27 insertions(+), 14 deletions(-) > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > > index 409f062c5c..de27cacfc9 100644 > > --- a/src/util/virnetdevip.c > > +++ b/src/util/virnetdevip.c > > @@ -496,7 +496,7 @@ virNetDevIPGetAcceptRA(const char *ifname) > > } > > > > struct virNetDevIPCheckIPv6ForwardingData { > > - bool hasRARoutes; > > + bool hasKernelRARoutes; > > > > /* Devices with conflicting accept_ra */ > > char **devices; > > @@ -552,15 +552,26 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, > > if (!ifname) > > return -1; > > > > - accept_ra = virNetDevIPGetAcceptRA(ifname); > > - > > VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d", > > ifname, ifindex, accept_ra); > > > > - if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > > + accept_ra = virNetDevIPGetAcceptRA(ifname); > > + /* 0 = do no accept RA > > + * 1 = accept if forwarding disabled > > + * 2 = ovveride and accept RA when forwarding enabled > > + * > > + * When RA is managed by userspace (systemd-networkd or > > + * NetworkManager) accept_ra is unset and we don't need to > > + * worry about it. If it is 1, enabling forwarding might > > + * change the behaviour so the user needs to be warned. > > + */ > > + if (accept_ra == 0) > > + return 0; > > + > > + if (accept_ra == 1 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > > return -1; > > > > - data->hasRARoutes = true; > > + data->hasKernelRARoutes = true; > > return 0; > > } > > > > @@ -590,11 +601,13 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, > > VIR_DEBUG("Checking multipath route nexthop device %s (%d), accept_ra: %d", > > ifname, nh->rtnh_ifindex, accept_ra); > > > > - if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > > - return -1; > > + if (accept_ra == 1) { > > + if (virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > > + return -1; > > + data->hasKernelRARoutes = true; > > + } > > > > VIR_FREE(ifname); > > - data->hasRARoutes = true; > > > > len -= NLMSG_ALIGN(nh->rtnh_len); > > VIR_WARNINGS_NO_CAST_ALIGN > > @@ -613,7 +626,7 @@ virNetDevIPCheckIPv6Forwarding(void) > > struct rtgenmsg genmsg; > > size_t i; > > struct virNetDevIPCheckIPv6ForwardingData data = { > > - .hasRARoutes = false, > > + .hasKernelRARoutes = false, > > .devices = NULL, > > .ndevices = 0 > > }; > > @@ -644,11 +657,11 @@ virNetDevIPCheckIPv6Forwarding(void) > > goto cleanup; > > } > > > > - valid = !data.hasRARoutes || data.ndevices == 0; > > + valid = !data.hasKernelRARoutes || data.ndevices == 0; > > > > /* Check the global accept_ra if at least one isn't set on a > > per-device basis */ > > - if (!valid && data.hasRARoutes) { > > + if (!valid && data.hasKernelRARoutes) { > > int accept_ra = virNetDevIPGetAcceptRA(NULL); > > valid = accept_ra == 2; > > VIR_DEBUG("Checked global accept_ra: %d", accept_ra); > > @@ -663,9 +676,9 @@ virNetDevIPCheckIPv6Forwarding(void) > > } > > > > virReportError(VIR_ERR_INTERNAL_ERROR, > > - _("Check the host setup: enabling IPv6 forwarding with " > > - "RA routes without accept_ra set to 2 is likely to cause " > > - "routes loss. Interfaces to look at: %s"), > > + _("Check the host setup: interface has accept_ra set to 1 " > > + "and enabling forwarding without accept_ra set to 2 is " > > + "likely to cause routes loss. Interfaces to look at: %s"), > > virBufferCurrentContent(&buf)); > > } > > > >
On Tue, Sep 01, 2020 at 08:27:47AM +0000, Cedric Bosdonnat wrote: > So the hypervisor has at least one (Router Advertised) RA route. > After defining a network like the following, the RA route is removed if > accept_ra isn't set to 2. > > <network ipv6="yes"> > <name>test5</name> > <forward mode="nat"/> > <bridge name="708837c1d27-br0" stp="off"/> > <mac address="52:54:00:45:5f:27"/> > <ip > family="ipv6" > address="fc00:0000:0000:000f:0000:0000:0000:0001" > prefix="64"/> > </network> > > The RA route was removed in networkEnableIPForwarding() when > setting /proc/sys/net/ipv6/conf/all/forwarding to 1. > > Me not being a network expert (and even less on ipv6) doesn't help. > > I hope this explanation will help you better see the use case I had. So it seems to be the intention of the kernel that when you enable forwarding your routes are flushed; changing the sysctl gets into addrconf_fixup_forwarding() [1] which then calls rt6_purge_dflt_routers() when the forwarding status is changed. That then purges default routes, unless accept_ra == 2; that was introduced with [3]. I guess the idea is that a router should not accept auto-configuration? HOWEVER ... if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) && (!idev || idev->cnf.accept_ra != 2) && fib6_info_hold_safe(rt)) { rcu_read_unlock(); ip6_del_rt(net, rt); goto restart; } I feel like this is checking the RTF_ADDRCONF flag before it flushes any routes. Checking that flag ... #define RTF_ADDRCONF 0x00040000 which I do not have set at all, from : $ cat /proc/net/ipv6_route | awk '{print $1 " " and(strtonum("0x"$9),strtonum("0x40000"))}' Based on this, I'm concluding that the userspace tools do not set this flag on their routes, and so they are never flushed. Empirically, fiddling forwarding on and off I don't see any routes flushed. So, I do not think that enabling forwarding will remove routes on the most common "sitting in-front of the computer" cases where you're using NetworkManager/systemd userspace magic. Given this, I'd propose we revert the check? -i [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv6/addrconf.c#n840 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv6/route.c#n4282 [3] https://lkml.org/lkml/2013/3/26/745
On Wed, 2020-09-09 at 12:39 +1000, Ian Wienand wrote: > On Tue, Sep 01, 2020 at 08:27:47AM +0000, Cedric Bosdonnat wrote: > > So the hypervisor has at least one (Router Advertised) RA route. > > After defining a network like the following, the RA route is removed if > > accept_ra isn't set to 2. > > > > <network ipv6="yes"> > > <name>test5</name> > > <forward mode="nat"/> > > <bridge name="708837c1d27-br0" stp="off"/> > > <mac address="52:54:00:45:5f:27"/> > > <ip > > family="ipv6" > > address="fc00:0000:0000:000f:0000:0000:0000:0001" > > prefix="64"/> > > </network> > > > > The RA route was removed in networkEnableIPForwarding() when > > setting /proc/sys/net/ipv6/conf/all/forwarding to 1. > > > > Me not being a network expert (and even less on ipv6) doesn't help. > > > > I hope this explanation will help you better see the use case I had. > > So it seems to be the intention of the kernel that when you enable > forwarding your routes are flushed; changing the sysctl gets into > addrconf_fixup_forwarding() [1] which then calls > rt6_purge_dflt_routers() when the forwarding status is changed. That > then purges default routes, unless accept_ra == 2; that was introduced > with [3]. > > I guess the idea is that a router should not accept > auto-configuration? > > HOWEVER ... > > if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) && > (!idev || idev->cnf.accept_ra != 2) && > fib6_info_hold_safe(rt)) { > rcu_read_unlock(); > ip6_del_rt(net, rt); > goto restart; > } > > I feel like this is checking the RTF_ADDRCONF flag before it flushes > any routes. Checking that flag ... > > #define RTF_ADDRCONF 0x00040000 > > which I do not have set at all, from : > > $ cat /proc/net/ipv6_route | awk '{print $1 " " and(strtonum("0x"$9),strtonum("0x40000"))}' > > Based on this, I'm concluding that the userspace tools do not set this > flag on their routes, and so they are never flushed. Empirically, > fiddling forwarding on and off I don't see any routes flushed. > > So, I do not think that enabling forwarding will remove routes on the > most common "sitting in-front of the computer" cases where you're > using NetworkManager/systemd userspace magic. > > Given this, I'd propose we revert the check? The check didn't involve any NetworkManager at all, but a network with RA route for the default route. Completely removing the check is rather likely to introduce a regression on that side. -- Cedric
On 9/9/20 2:38 AM, Cedric Bosdonnat wrote: > On Wed, 2020-09-09 at 12:39 +1000, Ian Wienand wrote: >> On Tue, Sep 01, 2020 at 08:27:47AM +0000, Cedric Bosdonnat wrote: >>> So the hypervisor has at least one (Router Advertised) RA route. >>> After defining a network like the following, the RA route is removed if >>> accept_ra isn't set to 2. >>> >>> <network ipv6="yes"> >>> <name>test5</name> >>> <forward mode="nat"/> >>> <bridge name="708837c1d27-br0" stp="off"/> >>> <mac address="52:54:00:45:5f:27"/> >>> <ip >>> family="ipv6" >>> address="fc00:0000:0000:000f:0000:0000:0000:0001" >>> prefix="64"/> >>> </network> >>> >>> The RA route was removed in networkEnableIPForwarding() when >>> setting /proc/sys/net/ipv6/conf/all/forwarding to 1. >>> >>> Me not being a network expert (and even less on ipv6) doesn't help. >>> >>> I hope this explanation will help you better see the use case I had. >> So it seems to be the intention of the kernel that when you enable >> forwarding your routes are flushed; changing the sysctl gets into >> addrconf_fixup_forwarding() [1] which then calls >> rt6_purge_dflt_routers() when the forwarding status is changed. That >> then purges default routes, unless accept_ra == 2; that was introduced >> with [3]. >> >> I guess the idea is that a router should not accept >> auto-configuration? >> >> HOWEVER ... >> >> if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) && >> (!idev || idev->cnf.accept_ra != 2) && >> fib6_info_hold_safe(rt)) { >> rcu_read_unlock(); >> ip6_del_rt(net, rt); >> goto restart; >> } >> >> I feel like this is checking the RTF_ADDRCONF flag before it flushes >> any routes. Checking that flag ... >> >> #define RTF_ADDRCONF 0x00040000 >> >> which I do not have set at all, from : >> >> $ cat /proc/net/ipv6_route | awk '{print $1 " " and(strtonum("0x"$9),strtonum("0x40000"))}' >> >> Based on this, I'm concluding that the userspace tools do not set this >> flag on their routes, and so they are never flushed. Empirically, >> fiddling forwarding on and off I don't see any routes flushed. >> >> So, I do not think that enabling forwarding will remove routes on the >> most common "sitting in-front of the computer" cases where you're >> using NetworkManager/systemd userspace magic. >> >> Given this, I'd propose we revert the check? > The check didn't involve any NetworkManager at all, Right. I think we've established that any system using systemd-networkd or NetworkManager doesn't care what is the setting of accept_ra in the kernel. But we can't just leave users of more traditional network management systems (which leave RA handling to the kernel) out in the cold to fend for themselves. I mean, the RA code is there in the kernel for a reason; if it's really not necessary or used any more, then remove it (yes, that is a *joke* - I know you can't remove an API from the kernel). As long as it's there, we need to assume that some people may use it, and act accordingly. Does your detailed spelunking of the kernel (nicely detailed above) maybe lead to some more reliable method of recognizing that we don't need the check (it kind of *sounds* like it does, but I'm unable to concentrate about it long enough to come up with a guaranteed answer) > but a network with > RA route for the default route. Completely removing the check is rather > likely to introduce a regression on that side.
On Wed, Sep 09, 2020 at 06:38:04AM +0000, Cedric Bosdonnat wrote: > The check didn't involve any NetworkManager at all, but a network with > RA route for the default route. Completely removing the check is rather > likely to introduce a regression on that side. Thanks for putting up with my bumbling about here :) So firstly; I think we can state the big picture original problem as "the kernel was seen to flush existing ipv6 routes when ipv6 forwarding is enabled, unless accept_ra==2 is set, which <may|probably|does> break peoples networking"? If that premise is correct, then I also think I'm correct in saying from ~[1] that the kernel will only flush routes with RTF_ADDRCONF set? if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) && (!idev || idev->cnf.accept_ra != 2) && fib6_info_hold_safe(rt)) { rcu_read_unlock(); ip6_del_rt(net, rt); goto restart; } If that premise is correct, then I also think that userspace tools do not set this flag on routes they setup when they handle RA's in userspace. I couldn't see it set on any of my routes on my laptop, and enabling/disabling forwarding didn't seem to flush any routes. If *that* premise is correct too, then as I understand the current code it queries netlink for all the routes, checks if they have RTPROT_RA set, and if accept_ra != 2 gives the error. If **that** premise is correct, then I think that just checking RTPROT_RA is too strict -- per the prior steps the route will only be flushed by the kernel if it has RTF_ADDRCONF set on it, and for many people using userspace tools their routing is not affected by enabling forwarding at all. If ***that*** premise is correct -- what to do about it? I don't think netlink exposes RTF_ADDRCONF? It can be seen via the flags dump in /proc/net/ipv6_route however (maybe that's a field in netlink?). But there may be room for conversation on how much this warning helps v hinders in 2020; it's not like it fixes the problem for you. Happy to be pointed out at which step I've gone wrong :) -i [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv6/route.c#n4282
Hi Ian, On Thu, 2020-09-10 at 09:48 +1000, Ian Wienand wrote: > On Wed, Sep 09, 2020 at 06:38:04AM +0000, Cedric Bosdonnat wrote: > > The check didn't involve any NetworkManager at all, but a network with > > RA route for the default route. Completely removing the check is rather > > likely to introduce a regression on that side. > > Thanks for putting up with my bumbling about here :) > > So firstly; I think we can state the big picture original problem as > "the kernel was seen to flush existing ipv6 routes when ipv6 > forwarding is enabled, unless accept_ra==2 is set, which > <may|probably|does> break peoples networking"? That is right > If that premise is correct, then I also think I'm correct in saying > from ~[1] that the kernel will only flush routes with RTF_ADDRCONF > set? > > if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) && > (!idev || idev->cnf.accept_ra != 2) && > fib6_info_hold_safe(rt)) { > rcu_read_unlock(); > ip6_del_rt(net, rt); > goto restart; > } > > If that premise is correct, then I also think that userspace tools do > not set this flag on routes they setup when they handle RA's in > userspace. I couldn't see it set on any of my routes on my laptop, > and enabling/disabling forwarding didn't seem to flush any routes. > > If *that* premise is correct too, then as I understand the current > code it queries netlink for all the routes, checks if they have > RTPROT_RA set, and if accept_ra != 2 gives the error. > > If **that** premise is correct, then I think that just checking > RTPROT_RA is too strict -- per the prior steps the route will only be > flushed by the kernel if it has RTF_ADDRCONF set on it, and for many > people using userspace tools their routing is not affected by enabling > forwarding at all. After reproducing here, I see the RTF_ADDRCONF flag set on those routes in /proc/net/ipv6_route. Looks like your theory fits with the use case. > If ***that*** premise is correct -- what to do about it? I don't > think netlink exposes RTF_ADDRCONF? It can be seen via the flags dump > in /proc/net/ipv6_route however (maybe that's a field in netlink?). > But there may be room for conversation on how much this warning helps > v hinders in 2020; it's not like it fixes the problem for you. You surely know more netlink than I do ;) -- Cédric
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 409f062c5c..de27cacfc9 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -496,7 +496,7 @@ virNetDevIPGetAcceptRA(const char *ifname) } struct virNetDevIPCheckIPv6ForwardingData { - bool hasRARoutes; + bool hasKernelRARoutes; /* Devices with conflicting accept_ra */ char **devices; @@ -552,15 +552,26 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, if (!ifname) return -1; - accept_ra = virNetDevIPGetAcceptRA(ifname); - VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d", ifname, ifindex, accept_ra); - if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) + accept_ra = virNetDevIPGetAcceptRA(ifname); + /* 0 = do no accept RA + * 1 = accept if forwarding disabled + * 2 = ovveride and accept RA when forwarding enabled + * + * When RA is managed by userspace (systemd-networkd or + * NetworkManager) accept_ra is unset and we don't need to + * worry about it. If it is 1, enabling forwarding might + * change the behaviour so the user needs to be warned. + */ + if (accept_ra == 0) + return 0; + + if (accept_ra == 1 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) return -1; - data->hasRARoutes = true; + data->hasKernelRARoutes = true; return 0; } @@ -590,11 +601,13 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, VIR_DEBUG("Checking multipath route nexthop device %s (%d), accept_ra: %d", ifname, nh->rtnh_ifindex, accept_ra); - if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) - return -1; + if (accept_ra == 1) { + if (virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) + return -1; + data->hasKernelRARoutes = true; + } VIR_FREE(ifname); - data->hasRARoutes = true; len -= NLMSG_ALIGN(nh->rtnh_len); VIR_WARNINGS_NO_CAST_ALIGN @@ -613,7 +626,7 @@ virNetDevIPCheckIPv6Forwarding(void) struct rtgenmsg genmsg; size_t i; struct virNetDevIPCheckIPv6ForwardingData data = { - .hasRARoutes = false, + .hasKernelRARoutes = false, .devices = NULL, .ndevices = 0 }; @@ -644,11 +657,11 @@ virNetDevIPCheckIPv6Forwarding(void) goto cleanup; } - valid = !data.hasRARoutes || data.ndevices == 0; + valid = !data.hasKernelRARoutes || data.ndevices == 0; /* Check the global accept_ra if at least one isn't set on a per-device basis */ - if (!valid && data.hasRARoutes) { + if (!valid && data.hasKernelRARoutes) { int accept_ra = virNetDevIPGetAcceptRA(NULL); valid = accept_ra == 2; VIR_DEBUG("Checked global accept_ra: %d", accept_ra); @@ -663,9 +676,9 @@ virNetDevIPCheckIPv6Forwarding(void) } virReportError(VIR_ERR_INTERNAL_ERROR, - _("Check the host setup: enabling IPv6 forwarding with " - "RA routes without accept_ra set to 2 is likely to cause " - "routes loss. Interfaces to look at: %s"), + _("Check the host setup: interface has accept_ra set to 1 " + "and enabling forwarding without accept_ra set to 2 is " + "likely to cause routes loss. Interfaces to look at: %s"), virBufferCurrentContent(&buf)); }
The checks modified here were added with 00d28a78b5d1f6eaf79f06ac59e31c568af9da37 to avoid losing routes on hosts. However, tools such as systemd-networking and NetworkManager manage RA's in userspace and thus IPv6 may be up and working on an interface even with accept_ra == 0. This modifies the check to only error if an interface's accept_ra is already set to "1"; as noted inline this seems to when it is likely that enabling forwarding may change the RA acceptance behaviour of the interface. I have noticed this because I am using the IPv6 NAT features enabled with 927acaedec7effbe67a154d8bfa0e67f7d08e6c7. I am using this on my laptop which switches between wired and wireless connections; both of which are configured in an unremarkable way by Fedora's NetworkManager and get configured for IPv6 via SLAAC and whatever NetworkManager magic it does. With this I can define and start a libvirt network with <nat ipv6="yes"> and <ip family='ipv6' address='fc00:abcd:ef12:34::' prefix='64'> and it seems to "just work" for guests. Signed-off-by: Ian Wienand <iwienand@redhat.com> --- src/util/virnetdevip.c | 41 +++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 14 deletions(-) -- 2.26.2