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
