This is about the same number of code lines, but is simpler, and more consistent with what will be added to check another attribute in a coming patch. As a side effect, it Resolves: https://bugzilla.redhat.com/1583131 Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/util/virnetdevip.c | 53 ++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 72048e4b45..c032ecacfc 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -529,49 +529,42 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, void *opaque) { struct rtmsg *rtmsg = NLMSG_DATA(resp); - int accept_ra = -1; - struct rtattr *rta; struct virNetDevIPCheckIPv6ForwardingData *data = opaque; - int len = RTM_PAYLOAD(resp); - int oif = -1; + struct rtattr *rta_attr; + int accept_ra = -1; + int ifindex = -1; VIR_AUTOFREE(char *) ifname = NULL; /* Ignore messages other than route ones */ if (resp->nlmsg_type != RTM_NEWROUTE) return 0; - /* Extract a device ID attribute */ - VIR_WARNINGS_NO_CAST_ALIGN - for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { - VIR_WARNINGS_RESET - if (rta->rta_type == RTA_OIF) { - oif = *(int *)RTA_DATA(rta); - - /* Should never happen: netlink message would be broken */ - if (ifname) { - VIR_AUTOFREE(char *) ifname2 = virNetDevGetName(oif); - VIR_WARN("Single route has unexpected 2nd interface " - "- '%s' and '%s'", ifname, ifname2); - break; - } - - if (!(ifname = virNetDevGetName(oif))) - return -1; - } - } - /* No need to do anything else for non RA routes */ if (rtmsg->rtm_protocol != RTPROT_RA) return 0; - data->hasRARoutes = true; + 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); - /* 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 (ifname)

+ accept_ra = virNetDevIPGetAcceptRA(ifname); - if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) - return -1; + VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d", + ifname, ifindex, accept_ra); + + if (!ifname ||

+ (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)) { + return -1; + } + + data->hasRARoutes = true; + return 0;

On 1/10/19 9:09 AM, Erik Skultety wrote: > On Wed, Jan 09, 2019 at 12:43:14PM -0500, Laine Stump wrote: > > This is about the same number of code lines, but is simpler, and more > > consistent with what will be added to check another attribute in a > > coming patch. > > > > As a side effect, it > > > > Resolves: https://bugzilla.redhat.com/1583131 > > Signed-off-by: Laine Stump <laine(a)laine.org&gt; > > --- > > src/util/virnetdevip.c | 53 ++++++++++++++++++------------------------ > > 1 file changed, 23 insertions(+), 30 deletions(-) > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > > index 72048e4b45..c032ecacfc 100644 > > --- a/src/util/virnetdevip.c > > +++ b/src/util/virnetdevip.c > > @@ -529,49 +529,42 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, > > void *opaque) > > { > > struct rtmsg *rtmsg = NLMSG_DATA(resp); > > - int accept_ra = -1; > > - struct rtattr *rta; > > struct virNetDevIPCheckIPv6ForwardingData *data = opaque; > > - int len = RTM_PAYLOAD(resp); > > - int oif = -1; > > + struct rtattr *rta_attr; > > + int accept_ra = -1; > > + int ifindex = -1; > > VIR_AUTOFREE(char *) ifname = NULL; > > > > /* Ignore messages other than route ones */ > > if (resp->nlmsg_type != RTM_NEWROUTE) > > return 0; > > > > - /* Extract a device ID attribute */ > > - VIR_WARNINGS_NO_CAST_ALIGN > > - for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { > > - VIR_WARNINGS_RESET > > - if (rta->rta_type == RTA_OIF) { > > - oif = *(int *)RTA_DATA(rta); > > - > > - /* Should never happen: netlink message would be broken */ > > - if (ifname) { > > - VIR_AUTOFREE(char *) ifname2 = virNetDevGetName(oif); > > - VIR_WARN("Single route has unexpected 2nd interface " > > - "- '%s' and '%s'", ifname, ifname2); > > - break; > > - } > > - > > - if (!(ifname = virNetDevGetName(oif))) > > - return -1; > > - } > > - } > > - > > /* No need to do anything else for non RA routes */ > > if (rtmsg->rtm_protocol != RTPROT_RA) > > return 0; > > > > - data->hasRARoutes = true; > > + 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); > > > > - /* 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 (ifname) > I'd put > > if (!ifname) > return -1; > > ^ here instead, since having (null) in the DEBUG output doesn't really help > anyone and... I disagree with that. Having a null ifname means that the ifindex sent as RTA_OIF couldn't be resolved to a proper name. Allowing the code to make it through to the VIR_DEBUG and print out the offending ifindex (along with "(null)") will give us more info to further investigate.

> > > + accept_ra = virNetDevIPGetAcceptRA(ifname); > > > > - if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) > > - return -1; > > + VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d", > > + ifname, ifindex, accept_ra); > > + > > + if (!ifname || > ... we'd return failure here anyway. > > > + (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)) { > > + return -1; > > + } > > + > > + data->hasRARoutes = true; > > + return 0; > I think ^this return should be part of the next patch where it IMHO makes more > sense. I included it here because the code is still correct with it in, and it makes the next patch more self-contained (the only code added is the code directly related to checking the nexthop interfaces).

(truthfully, this started out as a single patch, and I split it into parts to make it easier to review. I'd be just as happy to turn it back into a single patch :-)

On 1/10/19 10:38 AM, Erik Skultety wrote: > On Thu, Jan 10, 2019 at 09:34:35AM -0500, Laine Stump wrote: > > On 1/10/19 9:09 AM, Erik Skultety wrote: > > > On Wed, Jan 09, 2019 at 12:43:14PM -0500, Laine Stump wrote: > > > > This is about the same number of code lines, but is simpler, and more > > > > consistent with what will be added to check another attribute in a > > > > coming patch. > > > > > > > > As a side effect, it > > > > > > > > Resolves: https://bugzilla.redhat.com/1583131 > > > > Signed-off-by: Laine Stump <laine(a)laine.org&gt; > > > > --- > > > > src/util/virnetdevip.c | 53 ++++++++++++++++++------------------------ > > > > 1 file changed, 23 insertions(+), 30 deletions(-) > > > > > > > > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c > > > > index 72048e4b45..c032ecacfc 100644 > > > > --- a/src/util/virnetdevip.c > > > > +++ b/src/util/virnetdevip.c > > > > @@ -529,49 +529,42 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, > > > > void *opaque) > > > > { > > > > struct rtmsg *rtmsg = NLMSG_DATA(resp); > > > > - int accept_ra = -1; > > > > - struct rtattr *rta; > > > > struct virNetDevIPCheckIPv6ForwardingData *data = opaque; > > > > - int len = RTM_PAYLOAD(resp); > > > > - int oif = -1; > > > > + struct rtattr *rta_attr; > > > > + int accept_ra = -1; > > > > + int ifindex = -1; > > > > VIR_AUTOFREE(char *) ifname = NULL; > > > > > > > > /* Ignore messages other than route ones */ > > > > if (resp->nlmsg_type != RTM_NEWROUTE) > > > > return 0; > > > > > > > > - /* Extract a device ID attribute */ > > > > - VIR_WARNINGS_NO_CAST_ALIGN > > > > - for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) { > > > > - VIR_WARNINGS_RESET > > > > - if (rta->rta_type == RTA_OIF) { > > > > - oif = *(int *)RTA_DATA(rta); > > > > - > > > > - /* Should never happen: netlink message would be broken */ > > > > - if (ifname) { > > > > - VIR_AUTOFREE(char *) ifname2 = virNetDevGetName(oif); > > > > - VIR_WARN("Single route has unexpected 2nd interface " > > > > - "- '%s' and '%s'", ifname, ifname2); > > > > - break; > > > > - } > > > > - > > > > - if (!(ifname = virNetDevGetName(oif))) > > > > - return -1; > > > > - } > > > > - } > > > > - > > > > /* No need to do anything else for non RA routes */ > > > > if (rtmsg->rtm_protocol != RTPROT_RA) > > > > return 0; > > > > > > > > - data->hasRARoutes = true; > > > > + 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); > > > > > > > > - /* 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 (ifname) > > > I'd put > > > > > > if (!ifname) > > > return -1; > > > > > > ^ here instead, since having (null) in the DEBUG output doesn't really help > > > anyone and... > > > > I disagree with that. Having a null ifname means that the ifindex sent as > > RTA_OIF couldn't be resolved to a proper name. Allowing the code to make it > > through to the VIR_DEBUG and print out the offending ifindex (along with > > "(null)") will give us more info to further investigate. > In which case it qualifies as a warning, I am not entirely convinced we want > that information to be lost in the flood of DEBUG messages. If virNetDevGetName() fails to convert, it logs an error. Of course then you just end up with an error message saying that virNetDevGetName() failed to convert ifindex "blah" to a name, and no context about what was happening when that occurred. I still think that additionally having the debug log available regardless of whether or not virNetDevGetName() fails could be useful some day, and it's not taking any extra effort to do so, but if it really rubs you the wrong way, then I'll adjust it :-)

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(a)laine.org&gt; --- 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));

+ + 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 */

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

On 1/10/19 10:44 AM, Erik Skultety wrote: > 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(a)laine.org&gt; > > --- > > 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? The original "len" is the length of the RTA_MULTIPATH as given in the attribute object. If the message has been truncated somehow, using this value without validating it could lead to us trying to interpret garbage as if it were actual date.

The left side of the MIN is giving us the length from the start of the attribute (rta_attr) to the end of the actual message ("resp" is the start of the message, and "NLMSG_PAYLOAD(resp, 0)" is the length of that message).

So, it's just limiting len to be within the bounds of what we actually read from netlink, rather than trusting the length that was advertised by the attribute itself. > > > + > > + 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 :) Yeah, I suppose it should either say more (so that it's more readily obvious that virNetDevIPCheckIPv6ForwardingAddIF could consume the string) or say less (so you don't spend time wondering about it). I can remove the comment.