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