[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH v2] network: Only check kernel added routes in virNetDevIPCheckIPv6Forwarding



Hi Ian,

ACK. I've seen a commented VIR_DEBUG though that you surely want to
remove before pushing. I also really like the verbose commit message
explaining all the reasoning behind the change, thanks for the hard
work.

--
Cedric

On Fri, 2020-09-11 at 21:34 +1000, Ian Wienand wrote:
> The original motivation for adding virNetDevIPCheckIPv6Forwarding
> (00d28a78b5d1f6eaf79f06ac59e31c568af9da37) was that networking routes
> would disappear when ipv6 forwarding was enabled for an interface.
> 
> This is a fairly undocumented side-effect of the "accept_ra" sysctl
> for an interface.  1 means the interface will accept_ra's if not
> forwarding, 2 means always accept_RAs; but it is not explained that
> enabling forwarding when accept_ra==1 will also clear any kernel RA
> assigned routes, very likely breaking your networking.
> 
> The check to warn about this currently uses netlink to go through all
> the routes and then look at the accept_ra status of the interfaces.
> 
> However, it has been noticed that this problem does not affect systems
> where IPv6 RA configuration is handled in userspace, e.g. via tools
> such as NetworkManager.  In this case, the error message from libvirt
> is spurious, and modifying the forwarding state will not affect the RA
> state or disable your networking.
> 
> If you refer to the function rt6_purge_dflt_routers() in the kernel,
> we can see that the routes being purged are only those with the
> kernel's RTF_ADDRCONF flag set; that is, routes added by the kernel's
> RA handling.  Why does it do this?  I think this is a Linux
> implementation decision; it has always been like that and there are
> some comments suggesting that it is because a router should be
> statically configured, rather than accepting external configurations.
> 
> The solution implemented here is to convert the existing check into a
> walk of /proc/net/ipv6_route and look for routes with this flag set.
> We then check the accept_ra status for the interface, and if enabling
> forwarding would break things raise an error.
> 
> This should hopefully avoid "interactive" users, who are likely to be
> using NetworkManager and the like, having false warnings when enabling
> IPv6, but retain the error check for users relying on kernel-based
> IPv6 interface auto-configuration.
> 
> Signed-off-by: Ian Wienand <iwienand redhat com>
> ---
>  src/util/virnetdevip.c | 323 ++++++++++++++++-------------------------
>  1 file changed, 124 insertions(+), 199 deletions(-)
> 
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index 7bd5a75f85..e208089bd6 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -43,8 +43,11 @@
>  #ifdef __linux__
>  # include <linux/sockios.h>
>  # include <linux/if_vlan.h>
> +# include <linux/ipv6_route.h>
>  #endif
>  
> +#define PROC_NET_IPV6_ROUTE "/proc/net/ipv6_route"
> +
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
>  VIR_LOG_INIT("util.netdevip");
> @@ -370,203 +373,6 @@ virNetDevIPRouteAdd(const char *ifname,
>  }
>  
>  
> -static int
> -virNetDevIPGetAcceptRA(const char *ifname)
> -{
> -    g_autofree char *path = NULL;
> -    g_autofree char *buf = NULL;
> -    char *suffix;
> -    int accept_ra = -1;
> -
> -    path = g_strdup_printf("/proc/sys/net/ipv6/conf/%s/accept_ra",
> -                           ifname ? ifname : "all");
> -
> -    if ((virFileReadAll(path, 512, &buf) < 0) ||
> -        (virStrToLong_i(buf, &suffix, 10, &accept_ra) < 0))
> -        return -1;
> -
> -    return accept_ra;
> -}
> -
> -struct virNetDevIPCheckIPv6ForwardingData {
> -    bool hasRARoutes;
> -
> -    /* Devices with conflicting accept_ra */
> -    char **devices;
> -    size_t ndevices;
> -};
> -
> -
> -static int
> -virNetDevIPCheckIPv6ForwardingAddIF(struct virNetDevIPCheckIPv6ForwardingData *data,
> -                                    char **ifname)
> -{
> -    size_t i;
> -
> -    /* add ifname to the array if it's not already there
> -     * (ifname is char** so VIR_APPEND_ELEMENT() will move the
> -     * original pointer out of the way and avoid having it freed)
> -     */
> -    for (i = 0; i < data->ndevices; i++) {
> -        if (STREQ(data->devices[i], *ifname))
> -            return 0;
> -    }
> -    return VIR_APPEND_ELEMENT(data->devices, data->ndevices, *ifname);
> -}
> -
> -
> -static int
> -virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp,
> -                                       void *opaque)
> -{
> -    struct rtmsg *rtmsg = NLMSG_DATA(resp);
> -    struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> -    struct rtattr *rta_attr;
> -    int accept_ra = -1;
> -    int ifindex = -1;
> -    g_autofree char *ifname = NULL;
> -
> -    /* Ignore messages other than route ones */
> -    if (resp->nlmsg_type != RTM_NEWROUTE)
> -        return 0;
> -
> -    /* No need to do anything else for non RA routes */
> -    if (rtmsg->rtm_protocol != RTPROT_RA)
> -        return 0;
> -
> -    rta_attr = (struct rtattr *)nlmsg_find_attr(resp, sizeof(struct rtmsg), RTA_OIF);
> -    if (rta_attr) {
> -        /* This is a single path route, with interface used to reach
> -         * nexthop in the RTA_OIF attribute.
> -         */
> -        ifindex = *(int *)RTA_DATA(rta_attr);
> -        ifname = virNetDevGetName(ifindex);
> -
> -        if (!ifname)
> -           return -1;
> -
> -        accept_ra = virNetDevIPGetAcceptRA(ifname);
> -
> -        VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d",
> -                  ifname, ifindex, accept_ra);
> -
> -        if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
> -            return -1;
> -
> -        data->hasRARoutes = true;
> -        return 0;
> -    }
> -
> -    /* if no RTA_OIF was found, see if this is a multipath route (one
> -     * which has an array of nexthops, each with its own interface)
> -     */
> -
> -    rta_attr = (struct rtattr *)nlmsg_find_attr(resp, sizeof(struct rtmsg), RTA_MULTIPATH);
> -    if (rta_attr) {
> -        /* The data of the attribute is an array of rtnexthop */
> -        struct rtnexthop *nh = RTA_DATA(rta_attr);
> -        size_t len = RTA_PAYLOAD(rta_attr);
> -
> -        /* validate the attribute array length */
> -        len = MIN(len, ((char *)resp + NLMSG_PAYLOAD(resp, 0) - (char *)rta_attr));
> -
> -        while (len >= sizeof(*nh) && len >= nh->rtnh_len) {
> -            /* check accept_ra for the interface of each nexthop */
> -
> -            ifname = virNetDevGetName(nh->rtnh_ifindex);
> -
> -            if (!ifname)
> -                return -1;
> -
> -            accept_ra = virNetDevIPGetAcceptRA(ifname);
> -
> -            VIR_DEBUG("Checking multipath route nexthop device %s (%d), accept_ra: %d",
> -                      ifname, nh->rtnh_ifindex, accept_ra);
> -
> -            if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
> -                return -1;
> -
> -            VIR_FREE(ifname);
> -            data->hasRARoutes = true;
> -
> -            len -= NLMSG_ALIGN(nh->rtnh_len);
> -            VIR_WARNINGS_NO_CAST_ALIGN
> -            nh = RTNH_NEXT(nh);
> -            VIR_WARNINGS_RESET
> -        }
> -    }
> -
> -    return 0;
> -}
> -
> -bool
> -virNetDevIPCheckIPv6Forwarding(void)
> -{
> -    bool valid = false;
> -    struct rtgenmsg genmsg;
> -    size_t i;
> -    struct virNetDevIPCheckIPv6ForwardingData data = {
> -        .hasRARoutes = false,
> -        .devices = NULL,
> -        .ndevices = 0
> -    };
> -    g_autoptr(virNetlinkMsg) nlmsg = NULL;
> -
> -
> -    /* 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) {
> -        g_auto(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));
> -    }
> -
> - cleanup:
> -    virStringListFreeCount(data.devices, data.ndevices);
> -    return valid;
> -}
> -
>  #else /* defined(__linux__) && defined(WITH_LIBNL) */
>  
>  
> @@ -688,15 +494,134 @@ virNetDevIPRouteAdd(const char *ifname,
>      return 0;
>  }
>  
> +#endif /* defined(__linux__) && defined(HAVE_LIBNL) */
> +
> +
> +#if defined(__linux__)
> +
> +static int
> +virNetDevIPGetAcceptRA(const char *ifname)
> +{
> +    g_autofree char *path = NULL;
> +    g_autofree char *buf = NULL;
> +    char *suffix;
> +    int accept_ra = -1;
> +
> +    path = g_strdup_printf("/proc/sys/net/ipv6/conf/%s/accept_ra",
> +                           ifname ? ifname : "all");
> +
> +    if ((virFileReadAll(path, 512, &buf) < 0) ||
> +        (virStrToLong_i(buf, &suffix, 10, &accept_ra) < 0))
> +        return -1;
> +
> +    return accept_ra;
> +}
> +
> +/**
> + * virNetDevIPCheckIPv6Forwarding
> + *
> + * This function checks if IPv6 routes have the RTF_ADDRCONF flag set,
> + * indicating they have been created by the kernel's RA configuration
> + * handling.  These routes are subject to being flushed when ipv6
> + * forwarding is enabled unless accept_ra is explicitly set to "2".
> + * This will most likely result in ipv6 networking being broken.
> + *
> + * Returns: true if it is safe to enable forwarding, or false if
> + *          breakable routes are found.
> + *
> + **/
> +bool
> +virNetDevIPCheckIPv6Forwarding(void)
> +{
> +    int len;
> +    char *cur;
> +    g_autofree char *buf = NULL;
> +    /* lines are 150 chars */
> +    enum {MAX_ROUTE_SIZE = 150*100000};
> +
> +    /* This is /proc/sys/net/ipv6/conf/all/accept_ra */
> +    int all_accept_ra = virNetDevIPGetAcceptRA(NULL);
> +
> +    /* Read ipv6 routes */
> +    if ((len = virFileReadAll(PROC_NET_IPV6_ROUTE,
> +                              MAX_ROUTE_SIZE, &buf)) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to read %s "
> +                         "for ipv6 forwarding checks"), PROC_NET_IPV6_ROUTE);
> +        return false;
> +    }
> +
> +    /* VIR_DEBUG("%s output:\n%s", PROC_NET_IPV6_ROUTE, buf); */
> +
> +    /* Dropping the last character to stop the loop */
> +    if (len > 0)
> +        buf[len-1] = '\0';
> +
> +    cur = buf;
> +    while (cur) {
> +        char route[33], flags[9], iface[9];
> +        unsigned int flags_val;
> +        char *iface_val;
> +        int num;
> +        char *nl = strchr(cur, '\n');
> +
> +        if (nl)
> +            *nl++ = '\0';
> +
> +        num = sscanf(cur, "%32s %*s %*s %*s %*s %*s %*s %*s %8s %8s",
> +                     route, flags, iface);
> +
> +        cur = nl;
> +        if (num != 3) {
> +            VIR_DEBUG("Failed to parse route line: %s", cur);
> +            continue;
> +        }
> +
> +        if (virStrToLong_ui(flags, NULL, 16, &flags_val)) {
> +            VIR_DEBUG("Failed to parse flags: %s", flags);
> +            continue;
> +        }
> +
> +        /* This is right justified, strip leading spaces */
> +        iface_val = &iface[0];
> +        while (*iface_val && g_ascii_isspace(*iface_val))
> +            iface_val++;
> +
> +        VIR_DEBUG("%s iface %s flags %s : RTF_ADDRCONF %sset",
> +                  route, iface_val, flags,
> +                  (flags_val & RTF_ADDRCONF ? "" : "not "));
> +
> +        if (flags_val & RTF_ADDRCONF) {
> +            int ret = virNetDevIPGetAcceptRA(iface_val);
> +            VIR_DEBUG("%s reports accept_ra of %d",
> +                      iface_val, ret);
> +            /* If the interface for this autoconfigured route
> +             * has accept_ra == 1, or it is default and the "all"
> +             * value of accept_ra == 1, it will be subject to
> +             * flushing if forwarding is enabled.
> +             */
> +            if (ret == 1 || (ret == 0 && all_accept_ra == 1)) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Check the host setup: interface %s has kernel "
> +                                 "autoconfigured IPv6 routes and enabling forwarding "
> +                                 " without accept_ra set to 2 will cause the kernel "
> +                                 "to flush them, breaking networking."), iface_val);
> +                return false;
> +            }
> +        }
> +    }
> +    return true;
> +}
> +#else
>  
>  bool
>  virNetDevIPCheckIPv6Forwarding(void)
>  {
> -    VIR_WARN("built without libnl: unable to check if IPv6 forwarding can be safely enabled");
> +    VIR_DEBUG("No checks for IPv6 forwarding issues on non-Linux systems", flags);
>      return true;
>  }
>  
> -#endif /* defined(__linux__) && defined(WITH_LIBNL) */
> +#endif /* defined(__linux__) */
>  
>  
>  /**


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]