[libvirt] [PATCH v3] network: bridge: Don't start network if it collides with host routing
Fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=235961 If using the default virtual network, an easy way to lose guest network connectivity is to install libvirt inside the VM. The autostarted default network inside the guest collides with host virtual network routing. This is a long standing issue that has caused users quite a bit of pain and confusion. On network startup, parse /proc/net/route and compare the requested IP+netmask against host routing destinations: if any matches are found, refuse to start the network. v2: Drop sscanf, fix a comment typo, comment that function could use libnl instead of /proc v3: Consider route netmask. Compare binary data rather than convert to string. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/network/bridge_driver.c | 108 +++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 108 insertions(+), 0 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5d7ef19..7105a58 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -42,6 +42,8 @@ #include <stdio.h> #include <sys/wait.h> #include <sys/ioctl.h> +#include <netinet/in.h> +#include <arpa/inet.h> #include "virterror_internal.h" #include "datatypes.h" @@ -908,6 +910,108 @@ cleanup: return ret; } +#define PROC_NET_ROUTE "/proc/net/route" + +/* XXX: This function can be a lot more exhaustive, there are certainly + * other scenarios where we can ruin host network connectivity. + * XXX: Using a proper library is preferred over parsing /proc + */ +static int networkCheckRouteCollision(virNetworkObjPtr network) +{ + int ret = -1, len; + char *cur, *buf = NULL; + enum {MAX_ROUTE_SIZE = 1024*64}; + struct in_addr inaddress, innetmask; + char netaddr[32]; + + if (!network->def->ipAddress || !network->def->netmask) + return 0; + + if (inet_pton(AF_INET, network->def->ipAddress, &inaddress) <= 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse IP address '%s'"), + network->def->ipAddress); + goto error; + } + if (inet_pton(AF_INET, network->def->netmask, &innetmask) <= 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse netmask '%s'"), + network->def->netmask); + goto error; + } + + inaddress.s_addr &= innetmask.s_addr; + if (!inet_ntop(AF_INET, &inaddress, netaddr, sizeof(netaddr))) { + virReportSystemError(errno, "%s", + _("failed to format network address")); + goto error; + } + + /* Read whole routing table into memory */ + if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0) + goto error; + + /* Dropping the last character shouldn't hurt */ + buf[len-1] = '\0'; + + /* First line is just headings, skip it */ + cur = strchr(buf, '\n'); + + while (cur) { + char *data[8]; + char *dest, *iface, *mask; + unsigned int addr_val, mask_val; + int i; + + cur++; + + /* Delimit interface field */ + for (i = 0; i < sizeof(data); ++i) { + data[i] = cur; + + /* Parse fields and delimit */ + while(*cur > ' ') { + cur++; + } + *cur++ = '\0'; + } + + iface = data[0]; + dest = data[1]; + mask = data[7]; + + if (virStrToLong_ui(dest, NULL, 16, &addr_val) < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert network address %s"), + dest); + goto error; + } + + if (virStrToLong_ui(mask, NULL, 16, &mask_val) < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert network mask %s"), + mask); + goto error; + } + + addr_val &= mask_val; + + if ((inaddress.s_addr == addr_val) && (innetmask.s_addr == mask_val)) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Network %s is already in use by " + "interface %s"), netaddr, iface); + goto error; + } + + cur = strchr(cur, '\n'); + } + + ret = 0; +error: + VIR_FREE(buf); + return ret; +} + static int networkStartNetworkDaemon(struct network_driver *driver, virNetworkObjPtr network) { @@ -919,6 +1023,10 @@ static int networkStartNetworkDaemon(struct network_driver *driver, return -1; } + /* Check to see if network collides with an existing route */ + if (networkCheckRouteCollision(network) < 0) + return -1; + if ((err = brAddBridge(driver->brctl, network->def->bridge))) { virReportSystemError(err, _("cannot create bridge '%s'"), -- 1.6.6.1
Cole Robinson wrote:
Fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=235961
If using the default virtual network, an easy way to lose guest network connectivity is to install libvirt inside the VM. The autostarted default network inside the guest collides with host virtual network routing. This is a long standing issue that has caused users quite a bit of pain and confusion.
On network startup, parse /proc/net/route and compare the requested IP+netmask against host routing destinations: if any matches are found, refuse to start the network.
v2: Drop sscanf, fix a comment typo, comment that function could use libnl instead of /proc
v3: Consider route netmask. Compare binary data rather than convert to string. ... +static int networkCheckRouteCollision(virNetworkObjPtr network) +{ + int ret = -1, len; + char *cur, *buf = NULL; + enum {MAX_ROUTE_SIZE = 1024*64}; + struct in_addr inaddress, innetmask; + char netaddr[32]; + + if (!network->def->ipAddress || !network->def->netmask) + return 0; + + if (inet_pton(AF_INET, network->def->ipAddress, &inaddress) <= 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse IP address '%s'"), + network->def->ipAddress); + goto error; + } + if (inet_pton(AF_INET, network->def->netmask, &innetmask) <= 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse netmask '%s'"), + network->def->netmask); + goto error; + } + + inaddress.s_addr &= innetmask.s_addr; + if (!inet_ntop(AF_INET, &inaddress, netaddr, sizeof(netaddr))) { + virReportSystemError(errno, "%s", + _("failed to format network address")); + goto error; + } + + /* Read whole routing table into memory */ + if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0) + goto error;
+ /* Dropping the last character shouldn't hurt */
Hi Cole, Not sure how much you want to invest in manual parsing, but in case this code ends up staying with us for a while, here are some suggestions. Handle the case in which the file is empty. This will appease static checkers like clang and coverity. Either change the "< 0" test above to "<= 0" or do e.g., if (len > 0)
+ buf[len-1] = '\0';
Future proof it by skipping that first line only if it looks like the heading we see now: if (STRPREFIX (buf, "Iface"))
+ /* First line is just headings, skip it */ + cur = strchr(buf, '\n'); + + while (cur) { + char *data[8]; + char *dest, *iface, *mask; + unsigned int addr_val, mask_val; + int i;
It's slightly better to make "i" unsigned. The following assumes no leading white space, which is currently true at least on F13. Future/portability-proof it by skipping leading white space: while (c_isblank (*cur)) cur++;
+ cur++; + + /* Delimit interface field */ + for (i = 0; i < sizeof(data); ++i) {
Oops. Won't this make "i" iterate from 0 up to 31 or 63, depending on sizeof char*? You probably mean this: for (i = 0; i < ARRAY_CARDINALITY(data); ++i) {
+ data[i] = cur;
Otherwise, this would overrun the 8-element "data" array and clobber the stack.
+ /* Parse fields and delimit */ + while(*cur > ' ') {
Using "> ' '" works as long as each line has 8 or more fields. If there are fewer, it would treat the newline just like a regular field separator and continue getting fields from the next line. If you use c_isblank you have to handle end of line differently, but that's a good thing. How about using c_isblank here, too, rather than relying on ASCII "space" being smaller than "interesting" byte codes? On F13, the fields of /proc/net/route are TAB-delimited. Besides, I think y while (*cur && !c_isblank (*cur)) {
+ cur++; + } + *cur++ = '\0';
You'll want something here to keep "cur" from going more than 1 beyond the end of the buffer, in case the last row has fewer than 8 columns.
+ } + + iface = data[0]; + dest = data[1]; + mask = data[7];
If a line had fewer than 8 columns, at least mask is not valid. Similarly for dest if there are fewer than two columns.
+ if (virStrToLong_ui(dest, NULL, 16, &addr_val) < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert network address %s"), + dest); + goto error; + }
On 05/27/2010 04:54 AM, Jim Meyering wrote:
Cole Robinson wrote:
Fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=235961
If using the default virtual network, an easy way to lose guest network connectivity is to install libvirt inside the VM. The autostarted default network inside the guest collides with host virtual network routing. This is a long standing issue that has caused users quite a bit of pain and confusion.
On network startup, parse /proc/net/route and compare the requested IP+netmask against host routing destinations: if any matches are found, refuse to start the network.
v2: Drop sscanf, fix a comment typo, comment that function could use libnl instead of /proc
v3: Consider route netmask. Compare binary data rather than convert to string. ... +static int networkCheckRouteCollision(virNetworkObjPtr network) +{ + int ret = -1, len; + char *cur, *buf = NULL; + enum {MAX_ROUTE_SIZE = 1024*64}; + struct in_addr inaddress, innetmask; + char netaddr[32]; + + if (!network->def->ipAddress || !network->def->netmask) + return 0; + + if (inet_pton(AF_INET, network->def->ipAddress, &inaddress) <= 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse IP address '%s'"), + network->def->ipAddress); + goto error; + } + if (inet_pton(AF_INET, network->def->netmask, &innetmask) <= 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse netmask '%s'"), + network->def->netmask); + goto error; + } + + inaddress.s_addr &= innetmask.s_addr; + if (!inet_ntop(AF_INET, &inaddress, netaddr, sizeof(netaddr))) { + virReportSystemError(errno, "%s", + _("failed to format network address")); + goto error; + } + + /* Read whole routing table into memory */ + if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0) + goto error;
+ /* Dropping the last character shouldn't hurt */
Hi Cole,
Not sure how much you want to invest in manual parsing, but in case this code ends up staying with us for a while, here are some suggestions.
Handle the case in which the file is empty. This will appease static checkers like clang and coverity.
Either change the "< 0" test above to "<= 0" or do e.g.,
if (len > 0)
+ buf[len-1] = '\0';
Future proof it by skipping that first line only if it looks like the heading we see now:
if (STRPREFIX (buf, "Iface"))
Thanks for the review, I incorporated the above recommendations. However, I went back to using sscanf which greatly simplifies the parsing, and is used in a safe manner AFAICT. Updated patch coming shortly. Thanks, Cole
participants (2)
-
Cole Robinson -
Jim Meyering