[libvirt] [PATCH 0/4] Fix check for accept_ra when starting an IPv6 network
These patches fix the crash described in: https://bugzilla.redhat.com/1583131 They also fix a deficiency revealed by that crash - we weren't checking the accept_ra value of interfaces in the "nexthop" elements of multipath RA routes. Laine Stump (4): util: remove const specifier from nlmsghdr arg to virNetlinkDumpCallback() util: add a function to insert new interfaces to IPv6CheckForwarding list util: use nlmsg_find_attr() instead of an open-coded loop util: check accept_ra for all nexthop interfaces of multipath routes src/util/virnetdevip.c | 115 ++++++++++++++++++++++++++++------------- src/util/virnetlink.h | 2 +- 2 files changed, 79 insertions(+), 38 deletions(-) -- 2.20.1
This is problematic if a callback function wants to send the nlmsghdr to a library function that has no "const" in its prototype (e.g. nlmsg_find_attr()) Signed-off-by: Laine Stump <laine@laine.org> --- src/util/virnetdevip.c | 2 +- src/util/virnetlink.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 9d308e440a..4c869739ee 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -506,7 +506,7 @@ struct virNetDevIPCheckIPv6ForwardingData { }; static int -virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp, +virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, void *opaque) { struct rtmsg *rtmsg = NLMSG_DATA(resp); diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index debbd60f80..37442be44c 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -79,7 +79,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups); -typedef int (*virNetlinkDumpCallback)(const struct nlmsghdr *resp, +typedef int (*virNetlinkDumpCallback)(struct nlmsghdr *resp, void *data); int virNetlinkDumpCommand(struct nl_msg *nl_msg, -- 2.20.1
On Wed, Jan 09, 2019 at 12:43:12PM -0500, Laine Stump wrote:
This is problematic if a callback function wants to send the nlmsghdr to a library function that has no "const" in its prototype (e.g. nlmsg_find_attr())
Makes sense, since we can't control the API signatures in external libraries. Reviewed-by: Erik Skultety <eskultet@redhat.com>
This same operation needs to be done in multiple places, so move the inline code into a separate function. Signed-off-by: Laine Stump <laine@laine.org> --- src/util/virnetdevip.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 4c869739ee..72048e4b45 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -505,6 +505,25 @@ struct virNetDevIPCheckIPv6ForwardingData { 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) @@ -515,8 +534,6 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, struct virNetDevIPCheckIPv6ForwardingData *data = opaque; int len = RTM_PAYLOAD(resp); int oif = -1; - size_t i; - bool hasDevice; VIR_AUTOFREE(char *) ifname = NULL; /* Ignore messages other than route ones */ @@ -553,13 +570,7 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp, accept_ra = virNetDevIPGetAcceptRA(ifname); VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra); - hasDevice = false; - for (i = 0; i < data->ndevices && !hasDevice; i++) { - if (STREQ(data->devices[i], ifname)) - hasDevice = true; - } - if (accept_ra != 2 && !hasDevice && - VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0) + if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0) return -1; return 0; -- 2.20.1
On Wed, Jan 09, 2019 at 12:43:13PM -0500, Laine Stump wrote:
This same operation needs to be done in multiple places, so move the inline code into a separate function.
Signed-off-by: Laine Stump <laine@laine.org> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
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@laine.org> --- 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; + } return 0; } -- 2.20.1
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@laine.org> --- 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...
+ 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. Reviewed-by: Erik Skultety <eskultet@redhat.com>
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@laine.org> --- 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 :-)
Reviewed-by: Erik Skultety <eskultet@redhat.com>
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@laine.org> --- 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.
+ 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).
True, I thought of a scenario where someone reads through the git history in which case it looks weird, lacking the "look-ahead" context that there was a follow up patch which cleared it up, but whatever, I think too much.
(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 :-)
One can tell that it was split for purposes of the review :). Your call with the adjustments. Erik
On 1/10/19 10:38 AM, Erik Skultety 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@laine.org> --- 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
On Thu, Jan 10, 2019 at 09:34:35AM -0500, Laine Stump wrote: 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 :-)
+ 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).
True, I thought of a scenario where someone reads through the git history in which case it looks weird, lacking the "look-ahead" context that there was a follow up patch which cleared it up, but whatever, I think too much.
(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 :-)
One can tell that it was split for purposes of the review :).
Your call with the adjustments. Erik
On Thu, Jan 10, 2019 at 11:42:08AM -0500, Laine Stump wrote:
On 1/10/19 10:38 AM, Erik Skultety 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@laine.org> --- 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
On Thu, Jan 10, 2019 at 09:34:35AM -0500, Laine Stump wrote: 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 :-)
It's not a show stopper, please push as is.
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@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)); + + 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); + } + } + return 0; } -- 2.20.1
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@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); + } + } +
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@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?
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.
Erik
+ data->hasRARoutes = true; + + len -= NLMSG_ALIGN(nh->rtnh_len); + nh = RTNH_NEXT(nh); + } + } +
On Thu, Jan 10, 2019 at 11:48:42AM -0500, Laine Stump wrote:
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@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?
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.
So, do we really want to continue processing the data, if the message might have been truncated?
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).
Understood, thanks.
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.
The reason why it caught my eye was very simple, I don't remember any places in libvirt where we'd put such a comment with an identical scenario, so that's why I stopped and thought about it for a second. You have my RB if it's safe and okay for us to continue processing the data of an attribute if the message might be incomplete. Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 1/11/19 2:11 AM, Erik Skultety wrote:
On Thu, Jan 10, 2019 at 11:48:42AM -0500, Laine Stump wrote:
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@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?
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. So, do we really want to continue processing the data, if the message might have been truncated?
Yes. In the very low chance case that the message was truncated (because there were tons and tons of routes, and the entire list was too long for some internal buffer limit somewhere), I don't think we want to just throw our hands up and say "Okay, anything goes!" (or "%*(#( it, *nothing* goes!). We should just check for all the interfaces we can verifiably say do have RA routes associated with them. It's all really just a best effort check anyway (actually at least one person has asked that there be a way to disable it, since there is at least one case when it can give false-positives that make it impossible to start an IPv6 network, but that's a separate issue).
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).
Understood, thanks.
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.
The reason why it caught my eye was very simple, I don't remember any places in libvirt where we'd put such a comment with an identical scenario, so that's why I stopped and thought about it for a second.
You have my RB if it's safe and okay for us to continue processing the data of an attribute if the message might be incomplete.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
...anndddd I pushed it without making the same change you suggested in patch 3/4 (returning immediately if virNetDevGetName() failed) (I forgot I had the same logic in this patch until I noticed it while backporting to an earlier branch :-(). I'll send a patch to correct that now.
participants (2)
-
Erik Skultety -
Laine Stump