Re: [PATCH] network: allow accept_ra == 0 when enabling ipv6 forwarding

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(a)redhat.com&gt; > --- > 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)); > } >