[libvirt] [PATCH 4/4] util: check accept_ra for all nexthop interfaces of multipath routes

[Erik Skultety]

On Wed, Jan 09, 2019 at 12:43:15PM -0500, Laine Stump wrote:
> When checking the setting of accept_ra, we have assumed that all
> routes have a single nexthop, so the interface of the route would be
> in the RTA_OIF attribute of the netlink RTM_NEWROUTE message. But
> multipath routes don't have an RTA_OIF; instead, they have an
> RTA_MULTIPATH attribute, which is an array of rtnexthop, with each
> rtnexthop having an interface. This patch adds a loop to look at the
> setting of accept_ra of the interface for every rtnexthop in the
> array.
>
> Signed-off-by: Laine Stump <laine at laine.org>
> ---
>  src/util/virnetdevip.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index c032ecacfc..e0965bcc1d 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -566,6 +566,43 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp,
>          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));

I was fine until                      ...                              ^here,
you lost me there, can you elaborate on that witchcraft?

> +
> +        while (len >= sizeof(*nh) && len >= nh->rtnh_len) {
> +            /* check accept_ra for the interface of each nexthop */
> +
> +            ifname = virNetDevGetName(nh->rtnh_ifindex);
> +
> +            if (ifname)
> +                accept_ra = virNetDevIPGetAcceptRA(ifname);
> +
> +            VIR_DEBUG("Checking multipath route nexthop device %s (%d), accept_ra: %d",
> +                      ifname, nh->rtnh_ifindex, accept_ra);
> +
> +            if (!ifname ||
> +                (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)) {
> +                return -1;
> +            }
> +
> +            VIR_FREE(ifname); /* in case it wasn't added to the array */

                                I'd drop ^this commentary, otherwise it just
leaves the reader wondering what happens with free() in the other case, IOW
VIR_APPEND_ELEMENT happens :)

Erik

> +            data->hasRARoutes = true;
> +
> +            len -= NLMSG_ALIGN(nh->rtnh_len);
> +            nh = RTNH_NEXT(nh);
> +        }
> +    }
> +