[libvirt] [PATCH v2] 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 Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/network/bridge_driver.c | 102 +++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 102 insertions(+), 0 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5d7ef19..090bed7 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,102 @@ 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 = NULL; + + 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; + netaddr = strdup(inet_ntoa(inaddress)); + if (!netaddr) { + virReportOOMError(); + 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 *iface, *dest_raw; + char *dest_ip; + struct in_addr in; + unsigned int addr_val; + + cur++; + + /* Delimit interface field */ + iface = cur; + while(*cur > ' ') { + cur++; + } + *cur++ = '\0'; + + /* Delimit destination field */ + dest_raw = cur; + while(*cur > ' ') { + cur++; + } + *cur++ = '\0'; + + if (virStrToLong_ui(dest_raw, NULL, 16, &addr_val) < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert network address %s"), + dest_raw); + goto error; + } + + in.s_addr = addr_val; + dest_ip = inet_ntoa(in); + + if (STREQ(netaddr, dest_ip)) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Network destination %s is already in use " + "by interface %s"), netaddr, iface); + goto error; + } + + cur = strchr(cur, '\n'); + } + + ret = 0; +error: + VIR_FREE(buf); + VIR_FREE(netaddr); + return ret; +} + static int networkStartNetworkDaemon(struct network_driver *driver, virNetworkObjPtr network) { @@ -919,6 +1017,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
A couple items I didn't notice before: On 05/24/2010 02:52 PM, 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
Signed-off-by: Cole Robinson<crobinso@redhat.com> --- src/network/bridge_driver.c | 102 +++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 102 insertions(+), 0 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5d7ef19..090bed7 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,102 @@ 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 = NULL; + + 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; + netaddr = strdup(inet_ntoa(inaddress));
inet_ntoa() isn't threadsafe (it re-uses the same static buffer). strdup reduces the window, but the potential for a bad result is still there. Better to use inet_ntop(), or possibly use virSocketFormatAddr() (although that would have a bit of setup involved). But since the string you're comparing it to below was also originally a binary value (and also converted with inet_ntoa(), maybe the best thing would be to just compare the binary values directly and avoid the conversion.
+ if (!netaddr) { + virReportOOMError(); + 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 *iface, *dest_raw; + char *dest_ip; + struct in_addr in; + unsigned int addr_val; + + cur++; + + /* Delimit interface field */ + iface = cur; + while(*cur> ' ') { + cur++; + } + *cur++ = '\0'; + + /* Delimit destination field */ + dest_raw = cur; + while(*cur> ' ') { + cur++; + } + *cur++ = '\0'; + + if (virStrToLong_ui(dest_raw, NULL, 16,&addr_val)< 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert network address %s"), + dest_raw); + goto error; + }
After seeing the exchange with nDuff in #virt a couple days ago, it occurred to me that, in order to really check for an *exact* match of networks (and thus allow one to be a subset of the other, which can be a valid configuration), you should also parse further on in the line and get the netmask. Then compare netmasks of the two (as well as ANDing addr_val with the netmask to eliminate possibility of a network address that has extra bits set in the bottom).
+ + in.s_addr = addr_val; + dest_ip = inet_ntoa(in);
Again, a non-threadsafe call to inet_ntoa().
+ + if (STREQ(netaddr, dest_ip)) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Network destination %s is already in use " + "by interface %s"), netaddr, iface); + goto error; + } + + cur = strchr(cur, '\n'); + } + + ret = 0; +error: + VIR_FREE(buf); + VIR_FREE(netaddr); + return ret; +} + static int networkStartNetworkDaemon(struct network_driver *driver, virNetworkObjPtr network) { @@ -919,6 +1017,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'"),
On 05/26/2010 02:05 PM, Laine Stump wrote:
A couple items I didn't notice before:
On 05/24/2010 02:52 PM, 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
Signed-off-by: Cole Robinson<crobinso@redhat.com> --- src/network/bridge_driver.c | 102 +++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 102 insertions(+), 0 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5d7ef19..090bed7 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,102 @@ 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 = NULL; + + 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; + netaddr = strdup(inet_ntoa(inaddress));
inet_ntoa() isn't threadsafe (it re-uses the same static buffer). strdup reduces the window, but the potential for a bad result is still there. Better to use inet_ntop(), or possibly use virSocketFormatAddr() (although that would have a bit of setup involved).
Will do.
But since the string you're comparing it to below was also originally a binary value (and also converted with inet_ntoa(), maybe the best thing would be to just compare the binary values directly and avoid the conversion.
Doh, good point. I'll keep one of the inet_ntop conversions though, so it can be used in error messages.
+ if (!netaddr) { + virReportOOMError(); + 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 *iface, *dest_raw; + char *dest_ip; + struct in_addr in; + unsigned int addr_val; + + cur++; + + /* Delimit interface field */ + iface = cur; + while(*cur> ' ') { + cur++; + } + *cur++ = '\0'; + + /* Delimit destination field */ + dest_raw = cur; + while(*cur> ' ') { + cur++; + } + *cur++ = '\0'; + + if (virStrToLong_ui(dest_raw, NULL, 16,&addr_val)< 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to convert network address %s"), + dest_raw); + goto error; + }
After seeing the exchange with nDuff in #virt a couple days ago, it occurred to me that, in order to really check for an *exact* match of networks (and thus allow one to be a subset of the other, which can be a valid configuration), you should also parse further on in the line and get the netmask. Then compare netmasks of the two (as well as ANDing addr_val with the netmask to eliminate possibility of a network address that has extra bits set in the bottom).
Done, updated patch sent now. Thanks, Cole
On Wed, May 26, 2010 at 03:56:17PM -0400, Cole Robinson wrote:
On 05/26/2010 02:05 PM, Laine Stump wrote:
A couple items I didn't notice before:
On 05/24/2010 02:52 PM, 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
Signed-off-by: Cole Robinson<crobinso@redhat.com> --- src/network/bridge_driver.c | 102 +++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 102 insertions(+), 0 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5d7ef19..090bed7 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,102 @@ 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 = NULL; + + 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; + netaddr = strdup(inet_ntoa(inaddress));
inet_ntoa() isn't threadsafe (it re-uses the same static buffer). strdup reduces the window, but the potential for a bad result is still there. Better to use inet_ntop(), or possibly use virSocketFormatAddr() (although that would have a bit of setup involved).
Will do.
None of the inet_* functions should be used as they are all obsolete. Use one of the virSocket* functions in src/util/network.c - if there isn't a suitable one add a new one, based on getnameinfo() + AI_NUMERIC flag. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Laine Stump