When enabling IPv6 on all interfaces, we may get the host Router
Advertisement routes discarded. To avoid this, the user needs to set
accept_ra to 2 for the interfaces with such routes.

See https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
on this topic.

To avoid user mistakenly losing routes on their hosts, check
accept_ra values before enabling IPv6 forwarding. If a RA route is
detected, but neither the corresponding device nor global accept_ra
is set to 2, the network will fail to start.
---
 src/libvirt_private.syms    |   1 +
 src/network/bridge_driver.c |  16 +++--
 src/util/virnetdevip.c      | 158 ++++++++++++++++++++++++++++++++++++++++++++
 src/util/virnetdevip.h      |   1 +
 4 files changed, 171 insertions(+), 5 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0fe88c3fa..ec6553520 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2056,6 +2056,7 @@ virNetDevBridgeSetVlanFiltering;
 virNetDevIPAddrAdd;
 virNetDevIPAddrDel;
 virNetDevIPAddrGet;
+virNetDevIPCheckIPv6Forwarding;
 virNetDevIPInfoAddToDev;
 virNetDevIPInfoClear;
 virNetDevIPRouteAdd;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3f6561055..d02cd19f9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -61,6 +61,7 @@
 #include "virlog.h"
 #include "virdnsmasq.h"
 #include "configmake.h"
+#include "virnetlink.h"
 #include "virnetdev.h"
 #include "virnetdevip.h"
 #include "virnetdevbridge.h"
@@ -2377,11 +2378,16 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
     }
 
     /* If forward.type != NONE, turn on global IP forwarding */
-    if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE &&
-        networkEnableIPForwarding(v4present, v6present) < 0) {
-        virReportSystemError(errno, "%s",
-                             _("failed to enable IP forwarding"));
-        goto err3;
+    if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE) {
+        if (!virNetDevIPCheckIPv6Forwarding())
+            goto err3; /* Precise error message already provided */
+
+
+        if (networkEnableIPForwarding(v4present, v6present) < 0) {
+            virReportSystemError(errno, "%s",
+                                 _("failed to enable IP forwarding"));
+            goto err3;
+        }
     }
 
 
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 42fbba1eb..a4d382427 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -508,6 +508,158 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
     return ret;
 }
 
+static int
+virNetDevIPGetAcceptRA(const char *ifname)
+{
+    char *path = NULL;
+    char *buf = NULL;
+    char *suffix;
+    int accept_ra = -1;
+
+    if (virAsprintf(&path, "/proc/sys/net/ipv6/conf/%s/accept_ra",
+                    ifname ? ifname : "all") < 0)
+        goto cleanup;
+
+    if ((virFileReadAll(path, 512, &buf) < 0) ||
+        (virStrToLong_i(buf, &suffix, 10, &accept_ra) < 0))
+        goto cleanup;
+
+ cleanup:
+    VIR_FREE(path);
+    VIR_FREE(buf);
+
+    return accept_ra;
+}
+
+struct virNetDevIPCheckIPv6ForwardingData {
+    bool hasRARoutes;
+
+    /* Devices with conflicting accept_ra */
+    char **devices;
+    size_t ndevices;
+};
+
+static int
+virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
+                                       void *opaque)
+{
+    struct rtmsg *rtmsg = NLMSG_DATA(resp);
+    int accept_ra = -1;
+    struct rtattr *rta;
+    char *ifname = NULL;
+    struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
+    int ret = 0;
+    int len = RTM_PAYLOAD(resp);
+    int oif = -1;
+
+    /* Ignore messages other than route ones */
+    if (resp->nlmsg_type != RTM_NEWROUTE)
+        return ret;
+
+    /* Extract a few attributes */
+    for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
+        switch (rta->rta_type) {
+        case RTA_OIF:
+            oif = *(int *)RTA_DATA(rta);
+
+            if (!(ifname = virNetDevGetName(oif)))
+                goto error;
+            break;
+        }
+    }
+
+    /* No need to do anything else for non RA routes */
+    if (rtmsg->rtm_protocol != RTPROT_RA)
+        goto cleanup;
+
+    data->hasRARoutes = true;
+
+    /* Check the accept_ra value for the interface */
+    accept_ra = virNetDevIPGetAcceptRA(ifname);
+    VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra);
+
+    if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0)
+        goto error;
+
+ cleanup:
+    VIR_FREE(ifname);
+    return ret;
+
+ error:
+    ret = -1;
+    goto cleanup;
+}
+
+bool
+virNetDevIPCheckIPv6Forwarding(void)
+{
+    struct nl_msg *nlmsg = NULL;
+    bool valid = false;
+    struct rtgenmsg genmsg;
+    size_t i;
+    struct virNetDevIPCheckIPv6ForwardingData data = {
+        .hasRARoutes = false,
+        .devices = NULL,
+        .ndevices = 0
+    };
+
+
+    /* Prepare the request message */
+    if (!(nlmsg = nlmsg_alloc_simple(RTM_GETROUTE,
+                                     NLM_F_REQUEST | NLM_F_DUMP))) {
+        virReportOOMError();
+        goto cleanup;
+    }
+
+    memset(&genmsg, 0, sizeof(genmsg));
+    genmsg.rtgen_family = AF_INET6;
+
+    if (nlmsg_append(nlmsg, &genmsg, sizeof(genmsg), NLMSG_ALIGNTO) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("allocated netlink buffer is too small"));
+        goto cleanup;
+    }
+
+    /* Send the request and loop over the responses */
+    if (virNetlinkDumpCommand(nlmsg, virNetDevIPCheckIPv6ForwardingCallback,
+                              0, 0, NETLINK_ROUTE, 0, &data) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Failed to loop over IPv6 routes"));
+        goto cleanup;
+    }
+
+    valid = !data.hasRARoutes || data.ndevices == 0;
+
+    /* Check the global accept_ra if at least one isn't set on a
+       per-device basis */
+    if (!valid && data.hasRARoutes) {
+        int accept_ra = virNetDevIPGetAcceptRA(NULL);
+        valid = accept_ra == 2;
+        VIR_DEBUG("Checked global accept_ra: %d", accept_ra);
+    }
+
+    if (!valid) {
+        virBuffer buf = VIR_BUFFER_INITIALIZER;
+        for (i = 0; i < data.ndevices; i++) {
+            virBufferAdd(&buf, data.devices[i], -1);
+            if (i < data.ndevices - 1)
+                virBufferAddLit(&buf, ", ");
+        }
+
+        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"),
+                       virBufferCurrentContent(&buf));
+        virBufferFreeAndReset(&buf);
+    }
+
+ cleanup:
+    nlmsg_free(nlmsg);
+    for (i = 0; i < data.ndevices; i++)
+        VIR_FREE(data.devices[i]);
+    return valid;
+}
 
 #else /* defined(__linux__) && defined(HAVE_LIBNL) */
 
@@ -655,6 +807,12 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs ATTRIBUTE_UNUSED,
     return -1;
 }
 
+bool
+virNetDevIPCheckIPv6Forwarding(void)
+{
+    VIR_WARN("built without libnl: unable to check if IPv6 forwarding can be safely enabled");
+    return true;
+}
 
 #endif /* defined(__linux__) && defined(HAVE_LIBNL) */
 
diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h
index b7abdf94d..cc466ca25 100644
--- a/src/util/virnetdevip.h
+++ b/src/util/virnetdevip.h
@@ -83,6 +83,7 @@ int virNetDevIPAddrGet(const char *ifname, virSocketAddrPtr addr)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
 int virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
     ATTRIBUTE_NONNULL(1);
+bool virNetDevIPCheckIPv6Forwarding(void);
 
 /* virNetDevIPRoute object */
 void virNetDevIPRouteFree(virNetDevIPRoutePtr def);
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
> When enabling IPv6 on all interfaces, we may get the host Router
> Advertisement routes discarded. To avoid this, the user needs to set
> accept_ra to 2 for the interfaces with such routes.
> 
> See https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
> on this topic.
> 
> To avoid user mistakenly losing routes on their hosts, check
> accept_ra values before enabling IPv6 forwarding. If a RA route is
> detected, but neither the corresponding device nor global accept_ra
> is set to 2, the network will fail to start.

Since I already asked the question and didn't hear a positive response,
I'm guessing "no news is bad news", i.e. there isn't a reliable way to
fix this problem automatically. Assuming that, reporting the problem and
failing seems like the next best (least worse) thing...


> ---
>  src/libvirt_private.syms    |   1 +
>  src/network/bridge_driver.c |  16 +++--
>  src/util/virnetdevip.c      | 158 ++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virnetdevip.h      |   1 +
>  4 files changed, 171 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 0fe88c3fa..ec6553520 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2056,6 +2056,7 @@ virNetDevBridgeSetVlanFiltering;
>  virNetDevIPAddrAdd;
>  virNetDevIPAddrDel;
>  virNetDevIPAddrGet;
> +virNetDevIPCheckIPv6Forwarding;
>  virNetDevIPInfoAddToDev;
>  virNetDevIPInfoClear;
>  virNetDevIPRouteAdd;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 3f6561055..d02cd19f9 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -61,6 +61,7 @@
>  #include "virlog.h"
>  #include "virdnsmasq.h"
>  #include "configmake.h"
> +#include "virnetlink.h"
>  #include "virnetdev.h"
>  #include "virnetdevip.h"
>  #include "virnetdevbridge.h"
> @@ -2377,11 +2378,16 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>      }
>  
>      /* If forward.type != NONE, turn on global IP forwarding */
> -    if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE &&
> -        networkEnableIPForwarding(v4present, v6present) < 0) {
> -        virReportSystemError(errno, "%s",
> -                             _("failed to enable IP forwarding"));
> -        goto err3;
> +    if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE) {
> +        if (!virNetDevIPCheckIPv6Forwarding())
> +            goto err3; /* Precise error message already provided */
> +
> +
> +        if (networkEnableIPForwarding(v4present, v6present) < 0) {
> +            virReportSystemError(errno, "%s",
> +                                 _("failed to enable IP forwarding"));
> +            goto err3;
> +        }
>      }
>  
>  
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index 42fbba1eb..a4d382427 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -508,6 +508,158 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
>      return ret;
>  }
>  
> +static int
> +virNetDevIPGetAcceptRA(const char *ifname)
> +{
> +    char *path = NULL;
> +    char *buf = NULL;
> +    char *suffix;
> +    int accept_ra = -1;
> +
> +    if (virAsprintf(&path, "/proc/sys/net/ipv6/conf/%s/accept_ra",
> +                    ifname ? ifname : "all") < 0)
> +        goto cleanup;
> +
> +    if ((virFileReadAll(path, 512, &buf) < 0) ||
> +        (virStrToLong_i(buf, &suffix, 10, &accept_ra) < 0))
> +        goto cleanup;
> +
> + cleanup:
> +    VIR_FREE(path);
> +    VIR_FREE(buf);
> +
> +    return accept_ra;
> +}
> +
> +struct virNetDevIPCheckIPv6ForwardingData {
> +    bool hasRARoutes;
> +
> +    /* Devices with conflicting accept_ra */
> +    char **devices;
> +    size_t ndevices;
> +};
> +
> +static int
> +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> +                                       void *opaque)
> +{
> +    struct rtmsg *rtmsg = NLMSG_DATA(resp);
> +    int accept_ra = -1;
> +    struct rtattr *rta;
> +    char *ifname = NULL;
> +    struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> +    int ret = 0;
> +    int len = RTM_PAYLOAD(resp);
> +    int oif = -1;
> +
> +    /* Ignore messages other than route ones */
> +    if (resp->nlmsg_type != RTM_NEWROUTE)
> +        return ret;
> +
> +    /* Extract a few attributes */
> +    for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> +        switch (rta->rta_type) {
> +        case RTA_OIF:
> +            oif = *(int *)RTA_DATA(rta);
> +
> +            if (!(ifname = virNetDevGetName(oif)))
> +                goto error;
> +            break;
> +        }
> +    }
> +
> +    /* No need to do anything else for non RA routes */
> +    if (rtmsg->rtm_protocol != RTPROT_RA)
> +        goto cleanup;
> +
> +    data->hasRARoutes = true;
> +
> +    /* Check the accept_ra value for the interface */
> +    accept_ra = virNetDevIPGetAcceptRA(ifname);
> +    VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra);
> +
> +    if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0)
> +        goto error;
> +
> + cleanup:
> +    VIR_FREE(ifname);
> +    return ret;
> +
> + error:
> +    ret = -1;
> +    goto cleanup;
> +}
> +
> +bool
> +virNetDevIPCheckIPv6Forwarding(void)
> +{
> +    struct nl_msg *nlmsg = NULL;
> +    bool valid = false;
> +    struct rtgenmsg genmsg;
> +    size_t i;
> +    struct virNetDevIPCheckIPv6ForwardingData data = {
> +        .hasRARoutes = false,
> +        .devices = NULL,
> +        .ndevices = 0
> +    };
> +
> +
> +    /* Prepare the request message */
> +    if (!(nlmsg = nlmsg_alloc_simple(RTM_GETROUTE,
> +                                     NLM_F_REQUEST | NLM_F_DUMP))) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    memset(&genmsg, 0, sizeof(genmsg));
> +    genmsg.rtgen_family = AF_INET6;
> +
> +    if (nlmsg_append(nlmsg, &genmsg, sizeof(genmsg), NLMSG_ALIGNTO) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("allocated netlink buffer is too small"));
> +        goto cleanup;
> +    }
> +
> +    /* Send the request and loop over the responses */
> +    if (virNetlinkDumpCommand(nlmsg, virNetDevIPCheckIPv6ForwardingCallback,
> +                              0, 0, NETLINK_ROUTE, 0, &data) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Failed to loop over IPv6 routes"));
> +        goto cleanup;
> +    }
> +
> +    valid = !data.hasRARoutes || data.ndevices == 0;
> +
> +    /* Check the global accept_ra if at least one isn't set on a
> +       per-device basis */
> +    if (!valid && data.hasRARoutes) {
> +        int accept_ra = virNetDevIPGetAcceptRA(NULL);
> +        valid = accept_ra == 2;
> +        VIR_DEBUG("Checked global accept_ra: %d", accept_ra);
> +    }
> +
> +    if (!valid) {
> +        virBuffer buf = VIR_BUFFER_INITIALIZER;
> +        for (i = 0; i < data.ndevices; i++) {
> +            virBufferAdd(&buf, data.devices[i], -1);
> +            if (i < data.ndevices - 1)
> +                virBufferAddLit(&buf, ", ");
> +        }
> +
> +        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"),
> +                       virBufferCurrentContent(&buf));
> +        virBufferFreeAndReset(&buf);
> +    }
> +
> + cleanup:
> +    nlmsg_free(nlmsg);
> +    for (i = 0; i < data.ndevices; i++)
> +        VIR_FREE(data.devices[i]);
> +    return valid;
> +}
>  
>  #else /* defined(__linux__) && defined(HAVE_LIBNL) */
>  
> @@ -655,6 +807,12 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs ATTRIBUTE_UNUSED,
>      return -1;
>  }
>  
> +bool
> +virNetDevIPCheckIPv6Forwarding(void)
> +{
> +    VIR_WARN("built without libnl: unable to check if IPv6 forwarding can be safely enabled");
> +    return true;
> +}


Thanks for remembering this stub (I usually forget it).

>  
>  #endif /* defined(__linux__) && defined(HAVE_LIBNL) */
>  
> diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h
> index b7abdf94d..cc466ca25 100644
> --- a/src/util/virnetdevip.h
> +++ b/src/util/virnetdevip.h
> @@ -83,6 +83,7 @@ int virNetDevIPAddrGet(const char *ifname, virSocketAddrPtr addr)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>  int virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
>      ATTRIBUTE_NONNULL(1);
> +bool virNetDevIPCheckIPv6Forwarding(void);
>  
>  /* virNetDevIPRoute object */
>  void virNetDevIPRouteFree(virNetDevIPRoutePtr def);
> 

ACK. Nicely done! +++ Would review again :-)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, 2017-03-16 at 21:12 -0400, Laine Stump wrote:
> On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
> > When enabling IPv6 on all interfaces, we may get the host Router
> > Advertisement routes discarded. To avoid this, the user needs to set
> > accept_ra to 2 for the interfaces with such routes.
> > 
> > See https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
> > on this topic.
> > 
> > To avoid user mistakenly losing routes on their hosts, check
> > accept_ra values before enabling IPv6 forwarding. If a RA route is
> > detected, but neither the corresponding device nor global accept_ra
> > is set to 2, the network will fail to start.
> 
> Since I already asked the question and didn't hear a positive response,
> I'm guessing "no news is bad news", i.e. there isn't a reliable way to
> fix this problem automatically. Assuming that, reporting the problem and
> failing seems like the next best (least worse) thing...

The only automatic thing that we could do is remember the RA routes that
are set before enabling ipv6 forwarding and reset them after having lost them.

IMHO automatically changing the accept_ra parameter for the user could lead to
unknown (possibly buggy) cases. Before this patch, the network is started, but
the host may loose routes (the default one in my case). After, it keeps the
host routes, fails to start the network and tells the user to fix his host
network config.


--
Cedric

> 
> > ---
> >  src/libvirt_private.syms    |   1 +
> >  src/network/bridge_driver.c |  16 +++--
> >  src/util/virnetdevip.c      | 158 ++++++++++++++++++++++++++++++++++++++++++++
> >  src/util/virnetdevip.h      |   1 +
> >  4 files changed, 171 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 0fe88c3fa..ec6553520 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -2056,6 +2056,7 @@ virNetDevBridgeSetVlanFiltering;
> >  virNetDevIPAddrAdd;
> >  virNetDevIPAddrDel;
> >  virNetDevIPAddrGet;
> > +virNetDevIPCheckIPv6Forwarding;
> >  virNetDevIPInfoAddToDev;
> >  virNetDevIPInfoClear;
> >  virNetDevIPRouteAdd;
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index 3f6561055..d02cd19f9 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -61,6 +61,7 @@
> >  #include "virlog.h"
> >  #include "virdnsmasq.h"
> >  #include "configmake.h"
> > +#include "virnetlink.h"
> >  #include "virnetdev.h"
> >  #include "virnetdevip.h"
> >  #include "virnetdevbridge.h"
> > @@ -2377,11 +2378,16 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
> >      }
> >  
> >      /* If forward.type != NONE, turn on global IP forwarding */
> > -    if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE &&
> > -        networkEnableIPForwarding(v4present, v6present) < 0) {
> > -        virReportSystemError(errno, "%s",
> > -                             _("failed to enable IP forwarding"));
> > -        goto err3;
> > +    if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE) {
> > +        if (!virNetDevIPCheckIPv6Forwarding())
> > +            goto err3; /* Precise error message already provided */
> > +
> > +
> > +        if (networkEnableIPForwarding(v4present, v6present) < 0) {
> > +            virReportSystemError(errno, "%s",
> > +                                 _("failed to enable IP forwarding"));
> > +            goto err3;
> > +        }
> >      }
> >  
> >  
> > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> > index 42fbba1eb..a4d382427 100644
> > --- a/src/util/virnetdevip.c
> > +++ b/src/util/virnetdevip.c
> > @@ -508,6 +508,158 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
> >      return ret;
> >  }
> >  
> > +static int
> > +virNetDevIPGetAcceptRA(const char *ifname)
> > +{
> > +    char *path = NULL;
> > +    char *buf = NULL;
> > +    char *suffix;
> > +    int accept_ra = -1;
> > +
> > +    if (virAsprintf(&path, "/proc/sys/net/ipv6/conf/%s/accept_ra",
> > +                    ifname ? ifname : "all") < 0)
> > +        goto cleanup;
> > +
> > +    if ((virFileReadAll(path, 512, &buf) < 0) ||
> > +        (virStrToLong_i(buf, &suffix, 10, &accept_ra) < 0))
> > +        goto cleanup;
> > +
> > + cleanup:
> > +    VIR_FREE(path);
> > +    VIR_FREE(buf);
> > +
> > +    return accept_ra;
> > +}
> > +
> > +struct virNetDevIPCheckIPv6ForwardingData {
> > +    bool hasRARoutes;
> > +
> > +    /* Devices with conflicting accept_ra */
> > +    char **devices;
> > +    size_t ndevices;
> > +};
> > +
> > +static int
> > +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> > +                                       void *opaque)
> > +{
> > +    struct rtmsg *rtmsg = NLMSG_DATA(resp);
> > +    int accept_ra = -1;
> > +    struct rtattr *rta;
> > +    char *ifname = NULL;
> > +    struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> > +    int ret = 0;
> > +    int len = RTM_PAYLOAD(resp);
> > +    int oif = -1;
> > +
> > +    /* Ignore messages other than route ones */
> > +    if (resp->nlmsg_type != RTM_NEWROUTE)
> > +        return ret;
> > +
> > +    /* Extract a few attributes */
> > +    for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> > +        switch (rta->rta_type) {
> > +        case RTA_OIF:
> > +            oif = *(int *)RTA_DATA(rta);
> > +
> > +            if (!(ifname = virNetDevGetName(oif)))
> > +                goto error;
> > +            break;
> > +        }
> > +    }
> > +
> > +    /* No need to do anything else for non RA routes */
> > +    if (rtmsg->rtm_protocol != RTPROT_RA)
> > +        goto cleanup;
> > +
> > +    data->hasRARoutes = true;
> > +
> > +    /* Check the accept_ra value for the interface */
> > +    accept_ra = virNetDevIPGetAcceptRA(ifname);
> > +    VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra);
> > +
> > +    if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0)
> > +        goto error;
> > +
> > + cleanup:
> > +    VIR_FREE(ifname);
> > +    return ret;
> > +
> > + error:
> > +    ret = -1;
> > +    goto cleanup;
> > +}
> > +
> > +bool
> > +virNetDevIPCheckIPv6Forwarding(void)
> > +{
> > +    struct nl_msg *nlmsg = NULL;
> > +    bool valid = false;
> > +    struct rtgenmsg genmsg;
> > +    size_t i;
> > +    struct virNetDevIPCheckIPv6ForwardingData data = {
> > +        .hasRARoutes = false,
> > +        .devices = NULL,
> > +        .ndevices = 0
> > +    };
> > +
> > +
> > +    /* Prepare the request message */
> > +    if (!(nlmsg = nlmsg_alloc_simple(RTM_GETROUTE,
> > +                                     NLM_F_REQUEST | NLM_F_DUMP))) {
> > +        virReportOOMError();
> > +        goto cleanup;
> > +    }
> > +
> > +    memset(&genmsg, 0, sizeof(genmsg));
> > +    genmsg.rtgen_family = AF_INET6;
> > +
> > +    if (nlmsg_append(nlmsg, &genmsg, sizeof(genmsg), NLMSG_ALIGNTO) < 0) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("allocated netlink buffer is too small"));
> > +        goto cleanup;
> > +    }
> > +
> > +    /* Send the request and loop over the responses */
> > +    if (virNetlinkDumpCommand(nlmsg, virNetDevIPCheckIPv6ForwardingCallback,
> > +                              0, 0, NETLINK_ROUTE, 0, &data) < 0) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Failed to loop over IPv6 routes"));
> > +        goto cleanup;
> > +    }
> > +
> > +    valid = !data.hasRARoutes || data.ndevices == 0;
> > +
> > +    /* Check the global accept_ra if at least one isn't set on a
> > +       per-device basis */
> > +    if (!valid && data.hasRARoutes) {
> > +        int accept_ra = virNetDevIPGetAcceptRA(NULL);
> > +        valid = accept_ra == 2;
> > +        VIR_DEBUG("Checked global accept_ra: %d", accept_ra);
> > +    }
> > +
> > +    if (!valid) {
> > +        virBuffer buf = VIR_BUFFER_INITIALIZER;
> > +        for (i = 0; i < data.ndevices; i++) {
> > +            virBufferAdd(&buf, data.devices[i], -1);
> > +            if (i < data.ndevices - 1)
> > +                virBufferAddLit(&buf, ", ");
> > +        }
> > +
> > +        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"),
> > +                       virBufferCurrentContent(&buf));
> > +        virBufferFreeAndReset(&buf);
> > +    }
> > +
> > + cleanup:
> > +    nlmsg_free(nlmsg);
> > +    for (i = 0; i < data.ndevices; i++)
> > +        VIR_FREE(data.devices[i]);
> > +    return valid;
> > +}
> >  
> >  #else /* defined(__linux__) && defined(HAVE_LIBNL) */
> >  
> > @@ -655,6 +807,12 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs ATTRIBUTE_UNUSED,
> >      return -1;
> >  }
> >  
> > +bool
> > +virNetDevIPCheckIPv6Forwarding(void)
> > +{
> > +    VIR_WARN("built without libnl: unable to check if IPv6 forwarding can be safely enabled");
> > +    return true;
> > +}
> 
> 
> Thanks for remembering this stub (I usually forget it).
> 
> >  
> >  #endif /* defined(__linux__) && defined(HAVE_LIBNL) */
> >  
> > diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h
> > index b7abdf94d..cc466ca25 100644
> > --- a/src/util/virnetdevip.h
> > +++ b/src/util/virnetdevip.h
> > @@ -83,6 +83,7 @@ int virNetDevIPAddrGet(const char *ifname, virSocketAddrPtr addr)
> >      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> >  int virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
> >      ATTRIBUTE_NONNULL(1);
> > +bool virNetDevIPCheckIPv6Forwarding(void);
> >  
> >  /* virNetDevIPRoute object */
> >  void virNetDevIPRouteFree(virNetDevIPRoutePtr def);
> > 
> 
> ACK. Nicely done! +++ Would review again :-)
> 
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

> +
> +static int
> +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> +                                       void *opaque)
> +{
> +    struct rtmsg *rtmsg = NLMSG_DATA(resp);
> +    int accept_ra = -1;
> +    struct rtattr *rta;
> +    char *ifname = NULL;
> +    struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> +    int ret = 0;
> +    int len = RTM_PAYLOAD(resp);
> +    int oif = -1;
> +
> +    /* Ignore messages other than route ones */
> +    if (resp->nlmsg_type != RTM_NEWROUTE)
> +        return ret;
> +
> +    /* Extract a few attributes */
> +    for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> +        switch (rta->rta_type) {
> +        case RTA_OIF:
> +            oif = *(int *)RTA_DATA(rta);
> +
> +            if (!(ifname = virNetDevGetName(oif)))
> +                goto error;
> +            break;

Did you really mean to break from the for loop if ifname is set?  This
breaks only from the switch/case.  Of course Coverity doesn't know much
more than you'd be going back to the top of the for loop and could
overwrite ifname again. It proclaims a resource leak...

John
> +        }
> +    }
> +
> +    /* No need to do anything else for non RA routes */
> +    if (rtmsg->rtm_protocol != RTPROT_RA)
> +        goto cleanup;
> +
> +    data->hasRARoutes = true;
> +
> +    /* Check the accept_ra value for the interface */
> +    accept_ra = virNetDevIPGetAcceptRA(ifname);
> +    VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra);
> +
> +    if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0)
> +        goto error;
> +
> + cleanup:
> +    VIR_FREE(ifname);
> +    return ret;
> +
> + error:
> +    ret = -1;
> +    goto cleanup;
> +}

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
> [...]
> 
> > +
> > +static int
> > +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> > +                                       void *opaque)
> > +{
> > +    struct rtmsg *rtmsg = NLMSG_DATA(resp);
> > +    int accept_ra = -1;
> > +    struct rtattr *rta;
> > +    char *ifname = NULL;
> > +    struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> > +    int ret = 0;
> > +    int len = RTM_PAYLOAD(resp);
> > +    int oif = -1;
> > +
> > +    /* Ignore messages other than route ones */
> > +    if (resp->nlmsg_type != RTM_NEWROUTE)
> > +        return ret;
> > +
> > +    /* Extract a few attributes */
> > +    for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> > +        switch (rta->rta_type) {
> > +        case RTA_OIF:
> > +            oif = *(int *)RTA_DATA(rta);
> > +
> > +            if (!(ifname = virNetDevGetName(oif)))
> > +                goto error;
> > +            break;
> 
> Did you really mean to break from the for loop if ifname is set?  This
> breaks only from the switch/case.  Of course Coverity doesn't know much
> more than you'd be going back to the top of the for loop and could
> overwrite ifname again. It proclaims a resource leak...

In my dev version I was also getting the RTA_DST attribute to print some
debugging message that I removed in the submitted version. The break was
here between the switch cases.

In the current case I don't really care if we break out of the loop or not.
As there aren't that many attributes in an rtnetlink message to loop over,
breaking wouldn't gain a lot of cycles.

What I don't get though is what this break is actually doing? isn't it
for the switch case even though there is no other case after it?

--
Cedric

> John
> > +        }
> > +    }
> > +
> > +    /* No need to do anything else for non RA routes */
> > +    if (rtmsg->rtm_protocol != RTPROT_RA)
> > +        goto cleanup;
> > +
> > +    data->hasRARoutes = true;
> > +
> > +    /* Check the accept_ra value for the interface */
> > +    accept_ra = virNetDevIPGetAcceptRA(ifname);
> > +    VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra);
> > +
> > +    if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0)
> > +        goto error;
> > +
> > + cleanup:
> > +    VIR_FREE(ifname);
> > +    return ret;
> > +
> > + error:
> > +    ret = -1;
> > +    goto cleanup;
> > +}
> 
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 03/23/2017 04:44 AM, Cedric Bosdonnat wrote:
> On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
>> [...]
>>
>>> +
>>> +static int
>>> +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
>>> +                                       void *opaque)
>>> +{
>>> +    struct rtmsg *rtmsg = NLMSG_DATA(resp);
>>> +    int accept_ra = -1;
>>> +    struct rtattr *rta;
>>> +    char *ifname = NULL;
>>> +    struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
>>> +    int ret = 0;
>>> +    int len = RTM_PAYLOAD(resp);
>>> +    int oif = -1;
>>> +
>>> +    /* Ignore messages other than route ones */
>>> +    if (resp->nlmsg_type != RTM_NEWROUTE)
>>> +        return ret;
>>> +
>>> +    /* Extract a few attributes */
>>> +    for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
>>> +        switch (rta->rta_type) {
>>> +        case RTA_OIF:
>>> +            oif = *(int *)RTA_DATA(rta);
>>> +
>>> +            if (!(ifname = virNetDevGetName(oif)))
>>> +                goto error;
>>> +            break;
>>
>> Did you really mean to break from the for loop if ifname is set?  This
>> breaks only from the switch/case.  Of course Coverity doesn't know much
>> more than you'd be going back to the top of the for loop and could
>> overwrite ifname again. It proclaims a resource leak...
> 
> In my dev version I was also getting the RTA_DST attribute to print some
> debugging message that I removed in the submitted version. The break was
> here between the switch cases.
> 
> In the current case I don't really care if we break out of the loop or not.
> As there aren't that many attributes in an rtnetlink message to loop over,
> breaking wouldn't gain a lot of cycles.
> 
> What I don't get though is what this break is actually doing? isn't it
> for the switch case even though there is no other case after it?
> 
> --
> Cedric

So should this be changed to:

        if (rta->rta_type == RTA_OIF) {
            oif = *(int *)RTA_DATA(rta);

            if (!(ifname = virNetDevGetName(oif)))
                goto error;
            break;
        }

leaving two questions in my mind

  1. Can there be more than one RTA_OIF
  2. If we don't finish the loop (e.g. we break out), then does one of
the subsequent checks fail?

Is it more of a problem that we find *two* RTA_OIF's and thus should add a:

    if (ifname) {
        VIR_DEBUG("some sort of message");
        goto error;
    }

prior to the virNetDevGetName call and then remove the break;?

John (who doesn't know the answer!)

BTW: The issue from patch4 is resolved by my 22 patch bomb from yesterday

> 
>> John
>>> +        }
>>> +    }
>>> +
>>> +    /* No need to do anything else for non RA routes */
>>> +    if (rtmsg->rtm_protocol != RTPROT_RA)
>>> +        goto cleanup;
>>> +
>>> +    data->hasRARoutes = true;
>>> +
>>> +    /* Check the accept_ra value for the interface */
>>> +    accept_ra = virNetDevIPGetAcceptRA(ifname);
>>> +    VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra);
>>> +
>>> +    if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0)
>>> +        goto error;
>>> +
>>> + cleanup:
>>> +    VIR_FREE(ifname);
>>> +    return ret;
>>> +
>>> + error:
>>> +    ret = -1;
>>> +    goto cleanup;
>>> +}
>>
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, 2017-03-23 at 12:09 -0400, John Ferlan wrote:
> 
> On 03/23/2017 04:44 AM, Cedric Bosdonnat wrote:
> > On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
> > > [...]
> > > 
> > > > +
> > > > +static int
> > > > +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> > > > +                                       void *opaque)
> > > > +{
> > > > +    struct rtmsg *rtmsg = NLMSG_DATA(resp);
> > > > +    int accept_ra = -1;
> > > > +    struct rtattr *rta;
> > > > +    char *ifname = NULL;
> > > > +    struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> > > > +    int ret = 0;
> > > > +    int len = RTM_PAYLOAD(resp);
> > > > +    int oif = -1;
> > > > +
> > > > +    /* Ignore messages other than route ones */
> > > > +    if (resp->nlmsg_type != RTM_NEWROUTE)
> > > > +        return ret;
> > > > +
> > > > +    /* Extract a few attributes */
> > > > +    for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> > > > +        switch (rta->rta_type) {
> > > > +        case RTA_OIF:
> > > > +            oif = *(int *)RTA_DATA(rta);
> > > > +
> > > > +            if (!(ifname = virNetDevGetName(oif)))
> > > > +                goto error;
> > > > +            break;
> > > 
> > > Did you really mean to break from the for loop if ifname is set?  This
> > > breaks only from the switch/case.  Of course Coverity doesn't know much
> > > more than you'd be going back to the top of the for loop and could
> > > overwrite ifname again. It proclaims a resource leak...
> > 
> > In my dev version I was also getting the RTA_DST attribute to print some
> > debugging message that I removed in the submitted version. The break was
> > here between the switch cases.
> > 
> > In the current case I don't really care if we break out of the loop or not.
> > As there aren't that many attributes in an rtnetlink message to loop over,
> > breaking wouldn't gain a lot of cycles.
> > 
> > What I don't get though is what this break is actually doing? isn't it
> > for the switch case even though there is no other case after it?
> > 
> > --
> > Cedric
> 
> So should this be changed to:
> 
>         if (rta->rta_type == RTA_OIF) {
>             oif = *(int *)RTA_DATA(rta);
> 
>             if (!(ifname = virNetDevGetName(oif)))
>                 goto error;
>             break;
>         }
> 
> leaving two questions in my mind
> 
>   1. Can there be more than one RTA_OIF
>   2. If we don't finish the loop (e.g. we break out), then does one of
> the subsequent checks fail?

IMHO getting more than one RTA_OIF per message would be a bug from the
kernel: a route is associated to one device. Think of these messages like
the lines printed by 'ip r'

> Is it more of a problem that we find *two* RTA_OIF's and thus should add a:
> 
>     if (ifname) {
>         VIR_DEBUG("some sort of message");
>         goto error;
>     }
> 
> prior to the virNetDevGetName call and then remove the break;?

I can remove the break without that for sure.

> John (who doesn't know the answer!)
> 
> BTW: The issue from patch4 is resolved by my 22 patch bomb from yesterday

Yep, seen that one: thanks a lot
--
Cedric

> > 
> > > John
> > > > +        }
> > > > +    }
> > > > +
> > > > +    /* No need to do anything else for non RA routes */
> > > > +    if (rtmsg->rtm_protocol != RTPROT_RA)
> > > > +        goto cleanup;
> > > > +
> > > > +    data->hasRARoutes = true;
> > > > +
> > > > +    /* Check the accept_ra value for the interface */
> > > > +    accept_ra = virNetDevIPGetAcceptRA(ifname);
> > > > +    VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra);
> > > > +
> > > > +    if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0)
> > > > +        goto error;
> > > > +
> > > > + cleanup:
> > > > +    VIR_FREE(ifname);
> > > > +    return ret;
> > > > +
> > > > + error:
> > > > +    ret = -1;
> > > > +    goto cleanup;
> > > > +}
> > > 
> > > 
> 
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list