[libvirt] [PATCH 4/4] network: check accept_ra before enabling ipv6 forwarding

[Laine Stump]

On 03/03/2017 10:00 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 loosing routes on their hosts, check

s/loosing/losing/

> 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.

Is there any safe way to deal with it automatically? (apologies if I missed the end of any discussion - I recall your initial inquiry on the subject, but nothing after that)
> ---
>  src/network/bridge_driver.c | 178 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 173 insertions(+), 5 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 3f6561055..1ac837f7f 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"
> @@ -2067,6 +2068,168 @@ networkReloadFirewallRules(virNetworkDriverStatePtr driver)
>                               NULL);
>  }
>  
> +static int
> +networkGetAcceptRA(const char *ifname)
> +{
> +    char *path = NULL;
> +    char *buf = NULL;
> +    char *suffix;
> +    int accept_ra = -1;
> +
> +    if (virAsprintf(&path, SYSCTL_PATH "/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;
> +}

Although there is some  sysfs-specific stuff in the network directory already, I think I'd prefer to have this in util/virnetdevip.c.

> +
> +#if defined(__linux__) && defined(HAVE_LIBNL)
> +struct networkIPv6CheckData {
> +    bool hasRARoutes;
> +
> +    /* Devices with conflicting accept_ra */
> +    char **devices;
> +    size_t ndevices;
> +};
> +
> +static int
> +networkCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> +                                   void *opaque)
> +{
> +    struct rtmsg *rtmsg = NLMSG_DATA(resp);
> +    int accept_ra = -1;
> +    struct rtattr *rta;
> +    char *ifname = NULL;
> +    char name[IFNAMSIZ];
> +    struct networkIPv6CheckData *data = opaque;
> +    int ret = 0;

I understand why you defaulted ret = 0 instead of our normal -1. It does force me to actually think about each goto cleanup though...

> +    int len = RTM_PAYLOAD(resp);
> +    int oif = -1;
> +
> +    /* Ignore messages other than route ones */
> +    if (resp->nlmsg_type != RTM_NEWROUTE)
> +        return ret;
> +
> +    memset(&name, 0, sizeof(name));
> +
> +    /* 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 (!if_indextoname(oif, name) || VIR_STRDUP(ifname, name) < 0)
> +                VIR_DEBUG("Failed to convert OIF to a device name");

Do you really want to stop looking, but also still return success? Maybe it should fail? Or maybe it's okay to succeed, but the VIR_DEBUG() should be a VIR_WARN. (I'm not sure, that's why I'm asking :-)

(The one situation where I've seen an ifindex from the kernel fail to resolve into an interface name was when something "went wrong" in the kernel and macvtap interfaces showed themselves as attached to an ifindex that was no longer visible as a named network device. We never did figure out why (it only happened on a host that was part of a financial hosting service, so our customer wasn't allowed to give us access, and also couldn't install debug builds, so our troubleshooting capabilities were *very* limited), but the one certain thing is that the networking that involved these "nameless" interfaces didn't work (although other interfaces on the system did).

Also, if VIR_STRDUP() fails you will have logged an error but still returned success. Maybe you should check that separately and return error (assuming

Finally, maybe we should wrap if_indextoname() in a virNetDevGetName() (...FromIndex() ?) both for symmetry with virNetDevGetIndex() and for possible future use by other functions.

> +            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 = networkGetAcceptRA(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)
> +        ret = -1;

Okay, so this callback is gathering a list of all interfaces that have a route but accept_ra != 2.

> +
> + cleanup:
> +    VIR_FREE(ifname);
> +    return ret;
> +}
> +
> +static bool
> +networkCheckIPv6Forwarding(void)
> +{
> +    struct nl_msg *nlmsg = NULL;
> +    bool valid = false;
> +    struct rtgenmsg genmsg;
> +    size_t i;
> +    struct networkIPv6CheckData 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, networkCheckIPv6ForwardingCallback, 0, 0,
> +                              NETLINK_ROUTE, 0, &data) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Failed to loop over 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 = networkGetAcceptRA(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) */
> +
> +static bool
> +networkCheckIPv6Forwarding(void)
> +{
> +    VIR_WARN("built without libnl: unable to check if IPv6 forwarding can be safely enabled");
> +    return true;
> +}
> +#endif /* defined(__linux__) && defined(HAVE_LIBNL) */
> +
>  /* Enable IP Forwarding. Return 0 for success, -1 for failure. */
>  static int
>  networkEnableIPForwarding(bool enableIPv4, bool enableIPv6)
> @@ -2377,11 +2540,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 (!networkCheckIPv6Forwarding())
> +            goto err3; /* Precise error message already provided */
> +
> +
> +        if (networkEnableIPForwarding(v4present, v6present) < 0) {
> +            virReportSystemError(errno, "%s",
> +                                 _("failed to enable IP forwarding"));
> +            goto err3;
> +        }
>      }
>  
>  

I generally understand the logic, which looks fine. As I said above, I think it would be better to have most of it in virnetdevip.c (yes, I think the existing stuff that plays with ip_forward might also be better moved into utility functions in virnetdevip.c, but that's not your problem :-)