On Thu, 2017-03-16 at 21:12 -0400, Laine Stump wrote: > 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... The only automatic thing that we could do is remember the RA routes that are set before enabling ipv6 forwarding and reset them after having lost them. IMHO automatically changing the accept_ra parameter for the user could lead to unknown (possibly buggy) cases. Before this patch, the network is started, but the host may loose routes (the default one in my case). After, it keeps the host routes, fails to start the network and tells the user to fix his host network config. -- Cedric > > > --- > > 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
