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

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