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

[Laine Stump]

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 <iwien...@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));
      }