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@xxxxxxxxxx>
---
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'"),
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list