Re: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding

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 :-)