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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux