Please ignore this version, sorry, as I forgot to commit
the removal of the old functions in the patch.
On Fri, Sep 11, 2020 at 09:30:30PM +1000, Ian Wienand wrote:
> The original motivation for adding virNetDevIPCheckIPv6Forwarding
> (00d28a78b5d1f6eaf79f06ac59e31c568af9da37) was that networking routes
> would disappear when ipv6 forwarding was enabled for an interface.
>
> This is a fairly undocumented side-effect of the "accept_ra" sysctl
> for an interface. 1 means the interface will accept_ra's if not
> forwarding, 2 means always accept_RAs; but it is not explained that
> enabling forwarding when accept_ra==1 will also clear any kernel RA
> assigned routes, very likely breaking your networking.
>
> The check to warn about this currently uses netlink to go through all
> the routes and then look at the accept_ra status of the interfaces.
>
> However, it has been noticed that this problem does not affect systems
> where IPv6 RA configuration is handled in userspace, e.g. via tools
> such as NetworkManager. In this case, the error message from libvirt
> is spurious, and modifying the forwarding state will not affect the RA
> state or disable your networking.
>
> If you refer to the function rt6_purge_dflt_routers() in the kernel,
> we can see that the routes being purged are only those with the
> kernel's RTF_ADDRCONF flag set; that is, routes added by the kernel's
> RA handling. Why does it do this? I think this is a Linux
> implementation decision; it has always been like that and there are
> some comments suggesting that it is because a router should be
> statically configured, rather than accepting external configurations.
>
> The solution implemented here is to convert the existing check into a
> walk of /proc/net/ipv6_route and look for routes with this flag set.
> We then check the accept_ra status for the interface, and if enabling
> forwarding would break things raise an error.
>
> This should hopefully avoid "interactive" users, who are likely to be
> using NetworkManager and the like, having false warnings when enabling
> IPv6, but retain the error check for users relying on kernel-based
> IPv6 interface auto-configuration.
>
> Signed-off-by: Ian Wienand <iwienand@xxxxxxxxxx>
> ---
> src/util/virnetdevip.c | 108 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 106 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index 7bd5a75f85..93f47f22d9 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -43,8 +43,11 @@
> #ifdef __linux__
> # include <linux/sockios.h>
> # include <linux/if_vlan.h>
> +# include <linux/ipv6_route.h>
> #endif
>
> +#define PROC_NET_IPV6_ROUTE "/proc/net/ipv6_route"
> +
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> VIR_LOG_INIT("util.netdevip");
> @@ -688,15 +691,116 @@ virNetDevIPRouteAdd(const char *ifname,
> return 0;
> }
>
> +#endif /* defined(__linux__) && defined(HAVE_LIBNL) */
> +
> +
> +#if defined(__linux__)
> +
> +/**
> + * virNetDevIPCheckIPv6Forwarding
> + *
> + * This function checks if IPv6 routes have the RTF_ADDRCONF flag set,
> + * indicating they have been created by the kernel's RA configuration
> + * handling. These routes are subject to being flushed when ipv6
> + * forwarding is enabled unless accept_ra is explicitly set to "2".
> + * This will most likely result in ipv6 networking being broken.
> + *
> + * Returns: true if it is safe to enable forwarding, or false if
> + * breakable routes are found.
> + *
> + **/
> +bool
> +virNetDevIPCheckIPv6Forwarding(void)
> +{
> + int len;
> + char *cur;
> + g_autofree char *buf = NULL;
> + /* lines are 150 chars */
> + enum {MAX_ROUTE_SIZE = 150*100000};
> +
> + /* This is /proc/sys/net/ipv6/conf/all/accept_ra */
> + int all_accept_ra = virNetDevIPGetAcceptRA(NULL);
> +
> + /* Read ipv6 routes */
> + if ((len = virFileReadAll(PROC_NET_IPV6_ROUTE,
> + MAX_ROUTE_SIZE, &buf)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to read %s "
> + "for ipv6 forwarding checks"), PROC_NET_IPV6_ROUTE);
> + return false;
> + }
> +
> + /* VIR_DEBUG("%s output:\n%s", PROC_NET_IPV6_ROUTE, buf); */
> +
> + /* Dropping the last character to stop the loop */
> + if (len > 0)
> + buf[len-1] = '\0';
> +
> + cur = buf;
> + while (cur) {
> + char route[33], flags[9], iface[9];
> + unsigned int flags_val;
> + char *iface_val;
> + int num;
> + char *nl = strchr(cur, '\n');
> +
> + if (nl)
> + *nl++ = '\0';
> +
> + num = sscanf(cur, "%32s %*s %*s %*s %*s %*s %*s %*s %8s %8s",
> + route, flags, iface);
> +
> + cur = nl;
> + if (num != 3) {
> + VIR_DEBUG("Failed to parse route line: %s", cur);
> + continue;
> + }
> +
> + if (virStrToLong_ui(flags, NULL, 16, &flags_val)) {
> + VIR_DEBUG("Failed to parse flags: %s", flags);
> + continue;
> + }
> +
> + /* This is right justified, strip leading spaces */
> + iface_val = &iface[0];
> + while (*iface_val && g_ascii_isspace(*iface_val))
> + iface_val++;
> +
> + VIR_DEBUG("%s iface %s flags %s : RTF_ADDRCONF %sset",
> + route, iface_val, flags,
> + (flags_val & RTF_ADDRCONF ? "" : "not "));
> +
> + if (flags_val & RTF_ADDRCONF) {
> + int ret = virNetDevIPGetAcceptRA(iface_val);
> + VIR_DEBUG("%s reports accept_ra of %d",
> + iface_val, ret);
> + /* If the interface for this autoconfigured route
> + * has accept_ra == 1, or it is default and the "all"
> + * value of accept_ra == 1, it will be subject to
> + * flushing if forwarding is enabled.
> + */
> + if (ret == 1 || (ret == 0 && all_accept_ra == 1)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Check the host setup: interface %s has kernel "
> + "autoconfigured IPv6 routes and enabling forwarding "
> + " without accept_ra set to 2 will cause the kernel "
> + "to flush them, breaking networking."), iface_val);
> + return false;
> + }
> + }
> + }
> + return true;
> +}
> +#else
>
> bool
> virNetDevIPCheckIPv6Forwarding(void)
> {
> - VIR_WARN("built without libnl: unable to check if IPv6 forwarding can be safely enabled");
> + VIR_DEBUG("No checks for IPv6 forwarding issues on non-Linux systems", flags);
> return true;
> }
>
> -#endif /* defined(__linux__) && defined(WITH_LIBNL) */
> +#endif /* defined(__linux__) */
>
>
> /**
> --
> 2.26.2
>
