The existing code was checking all ip addresses and routes of the
network as each line of /proc/net/route was read. This does not lend
itself well to the new autoaddr networks, as each ip address will need
to be checked against all routes in /proc/net/route (i.e. the opposite
order of nesting the loops) in order to determine if the address must
be changed to something unused (and then change it and recheck against
all routes until an unused subnet is found).
Now there is a separate function (still in bridge_driver_linux.c) that
reads all the entries in /proc/net/route and adds them to a hash
table, and another that checks an IP/prefix against said hash table.
These two functions are called by networkCheckIPAddrCollision() and
networkCheckRouteCollision() (split apart and moved to
bridge_driver.c) which then iterates through each IP address/route of
the virtual network, checking each against all existing routes before
moving on to the next.
Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
---
src/network/bridge_driver.c | 72 ++++++++++++++-
src/network/bridge_driver_linux.c | 132 +++++++++++++++------------
src/network/bridge_driver_nop.c | 22 +++--
src/network/bridge_driver_platform.h | 5 +-
4 files changed, 164 insertions(+), 67 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 32572c755f..b8e752f20d 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1898,6 +1898,65 @@ networkAddRouteToBridge(virNetworkObj *obj,
}
+/* 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
+networkCheckIPAddrCollision(virNetworkDef *def, GHashTable *routes)
+{
+ size_t i;
+ virNetworkIPDef *ipdef;
+
+ /* check all IPv4 addresses of the network for conflict with
+ * existing routes
+ */
+ for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_INET, i)); i++) {
+ unsigned int prefix = virNetworkIPDefPrefix(ipdef);
+ const char *iface = networkSysRoutesTableFind(routes, &ipdef->address, prefix);
+
+ if (iface) {
+ g_autofree char *addrStr = virSocketAddrFormat(&ipdef->address);
+
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("requested subnet %1$s/%2$u is already in use by interface %3$s"),
+ NULLSTR(addrStr), prefix, iface);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
+static int
+networkCheckRouteCollision(virNetworkDef *def, GHashTable *routes)
+{
+ size_t i;
+ virNetDevIPRoute *routedef;
+
+ /* check all IPv4 host routes that will be added for this network
+ * for conflict with existing host routes
+ */
+ for (i = 0; (routedef = virNetworkDefGetRouteByIndex(def, AF_INET, i)); i++) {
+ virSocketAddr *addr = virNetDevIPRouteGetAddress(routedef);
+ int prefix = virNetDevIPRouteGetPrefix(routedef);
+ const char *iface = networkSysRoutesTableFind(routes, addr, prefix);
+
+ if (iface) {
+ g_autofree char *addrStr = virSocketAddrFormat(addr);
+
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("requested route for %1$s/%2$u is already in use by interface %3$s"),
+ NULLSTR(addrStr), prefix, iface);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
static int
networkStartNetworkVirtual(virNetworkDriverState *driver,
virNetworkObj *obj)
@@ -1914,10 +1973,17 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
bool firewalRulesAdded = false;
virSocketAddr *dnsServer = NULL;
virFirewall *fwRemoval = NULL;
+ g_autoptr(GHashTable) routes = networkSysRoutesTableRead();
- /* Check to see if any network IP collides with an existing route */
- if (networkCheckRouteCollision(def) < 0)
- return -1;
+ if (routes) {
+ /* Check to see if any network IP collides with an existing route on the host */
+ if (networkCheckIPAddrCollision(def, routes) < 0)
+ return -1;
+
+ /* also check for requested routes that collide with existing routes */
+ if (networkCheckRouteCollision(def, routes) < 0)
+ return -1;
+ }
/* Create and configure the bridge device */
if (!def->bridge) {
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index fe7c6e193c..1d6635743d 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -213,21 +213,53 @@ void networkPostReloadFirewallRules(bool startup G_GNUC_UNUSED)
}
-/* 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 void
+networkPrintRouteTableEntry(gpointer key,
+ gpointer value,
+ gpointer user_data G_GNUC_UNUSED)
+{
+ const gint64 *keyval = key;
+ const char *valueval = value;
+
+ VIR_INFO("key: %016llx value: %s", (unsigned long long)*keyval, valueval);
+}
+/**
+ * networkSysRoutesTableRead::
+ *
+ * Returns a GHashTable that contains an entry for each IPv4 route in
+ * the host system routing table.
+ *
+ * The key for each entry is the network address and prefix for the
+ * route, and the "data" is the name of the interface associated with
+ * the route (which is only used for error reporting if a conflict is
+ * found). This will later be used to determine if a given subnet is
+ * already "in use" (which for our purposes is defined as "there is a
+ * route in the system routing table with exactly the same subnet +
+ * prefix/netmask").
+ *
+ * In the case that no routes are found (e.g. due to some unexpected
+ * changed in the format of /proc/net/route) an empty GHashTable will
+ * be returned, in order to be as non-disruptive as possible.
+ *
+ * The returned GHashTable must be g_hash_table_destroy()ed when the
+ * caller is finished with it. This can be done automatically by
+ * defining it as g_autoptr(GHashTable).
+ *
*/
-int networkCheckRouteCollision(virNetworkDef *def)
+GHashTable *
+networkSysRoutesTableRead(void)
{
int len;
char *cur;
g_autofree char *buf = NULL;
/* allow for up to 100000 routes (each line is 128 bytes) */
enum {MAX_ROUTE_SIZE = 128*100000};
+ g_autoptr(GHashTable) routes = g_hash_table_new_full(g_int64_hash, g_int64_equal, g_free, g_free);
+
/* Read whole routing table into memory */
if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0)
- return 0;
+ return g_steal_pointer(&routes);
/* Dropping the last character shouldn't hurt */
if (len > 0)
@@ -236,7 +268,7 @@ int networkCheckRouteCollision(virNetworkDef *def)
VIR_DEBUG("%s output:\n%s", PROC_NET_ROUTE, buf);
if (!STRPREFIX(buf, "Iface"))
- return 0;
+ return g_steal_pointer(&routes);
/* First line is just headings, skip it */
cur = strchr(buf, '\n');
@@ -245,11 +277,9 @@ int networkCheckRouteCollision(virNetworkDef *def)
while (cur) {
char iface[17], dest[128], mask[128];
- unsigned int addr_val, mask_val;
- virNetworkIPDef *ipdef;
- virNetDevIPRoute *routedef;
int num;
- size_t i;
+ unsigned int addr_val, mask_val;
+ g_autofree gint64 *key = g_new(gint64, 1);
/* NUL-terminate the line, so sscanf doesn't go beyond a newline. */
char *nl = strchr(cur, '\n');
@@ -275,60 +305,48 @@ int networkCheckRouteCollision(virNetworkDef *def)
continue;
}
- addr_val &= mask_val;
+ /* NB: addr_val and mask_val both appear in the file as
+ * hexadecimal strings that, when converted into integers on
+ * little-endian hardware, will already be in network byte
+ * order! (e.g., the IP address 1.2.3.4 will show up as
+ * "04030201" in the file). When we are later looking up an
+ * address in the table, we will also be using network byte
+ * order, so we don't need to do any htonl() here.
+ */
+ *key = ((gint64)(addr_val & mask_val) << 32) + mask_val;
+ VIR_DEBUG("addr_val: %08u mask_val: %08u key: %016llx iface: %s",
+ addr_val, mask_val, (unsigned long long)*key, iface);
- for (i = 0;
- (ipdef = virNetworkDefGetIPByIndex(def, AF_INET, i));
- i++) {
+ g_hash_table_replace(routes, g_steal_pointer(&key), g_strdup(iface));
+ }
- unsigned int net_dest;
- virSocketAddr netmask;
+ VIR_INFO("Full Route Hash Table: sizeof(gint64): %d sizeof(long long unsigned): %d",
+ (int)sizeof(gint64), (int)sizeof(long long unsigned));
+ g_hash_table_foreach(routes, networkPrintRouteTableEntry, NULL);
- if (virNetworkIPDefNetmask(ipdef, &netmask) < 0) {
- VIR_WARN("Failed to get netmask of '%s'",
- def->bridge);
- continue;
- }
+ return g_steal_pointer(&routes);
+}
- net_dest = (ipdef->address.data.inet4.sin_addr.s_addr &
- netmask.data.inet4.sin_addr.s_addr);
- if ((net_dest == addr_val) &&
- (netmask.data.inet4.sin_addr.s_addr == mask_val)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Network is already in use by interface %1$s"),
- iface);
- return -1;
- }
- }
+const char *
+networkSysRoutesTableFind(GHashTable *routesTable,
+ virSocketAddr *addr,
+ unsigned int prefix)
+{
+ virSocketAddr netaddr, netmask;
+ gint64 compare;
- for (i = 0;
- (routedef = virNetworkDefGetRouteByIndex(def, AF_INET, i));
- i++) {
-
- virSocketAddr r_mask, r_addr;
- virSocketAddr *tmp_addr = virNetDevIPRouteGetAddress(routedef);
- int r_prefix = virNetDevIPRouteGetPrefix(routedef);
-
- if (!tmp_addr ||
- virSocketAddrMaskByPrefix(tmp_addr, r_prefix, &r_addr) < 0 ||
- virSocketAddrPrefixToNetmask(r_prefix, &r_mask, AF_INET) < 0)
- continue;
-
- if ((r_addr.data.inet4.sin_addr.s_addr == addr_val) &&
- (r_mask.data.inet4.sin_addr.s_addr == mask_val)) {
- g_autofree char *addr_str = virSocketAddrFormat(&r_addr);
- if (!addr_str)
- virResetLastError();
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Route address '%1$s' conflicts with IP address for '%2$s'"),
- NULLSTR(addr_str), iface);
- return -1;
- }
- }
- }
+ if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0)
+ return false;
- return 0;
+ if (virSocketAddrMask(addr, &netmask, &netaddr) < 0)
+ return false;
+
+ compare = ((gint64)netaddr.data.inet4.sin_addr.s_addr << 32)
+ + netmask.data.inet4.sin_addr.s_addr;
+
+ VIR_DEBUG("Searching for key: %016llx", (unsigned long long)compare);
+ return g_hash_table_lookup(routesTable, &compare);
}
diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
index 8bf3367bff..d46aa99a5a 100644
--- a/src/network/bridge_driver_nop.c
+++ b/src/network/bridge_driver_nop.c
@@ -32,12 +32,6 @@ void networkPostReloadFirewallRules(bool startup G_GNUC_UNUSED)
{
}
-
-int networkCheckRouteCollision(virNetworkDef *def G_GNUC_UNUSED)
-{
- return 0;
-}
-
int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
virFirewallBackend firewallBackend,
virFirewall **fwRemoval G_GNUC_UNUSED)
@@ -59,3 +53,19 @@ int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
void networkRemoveFirewallRules(virNetworkObj *obj G_GNUC_UNUSED)
{
}
+
+
+GHashTable *
+networkSysRoutesTableRead(void)
+{
+ return NULL;
+}
+
+
+const char *
+networkSysRoutesTableFind(GHashTable *routesTable G_GNUC_UNUSED,
+ virSocketAddr *addr G_GNUC_UNUSED,
+ unsigned int prefix G_GNUC_UNUSED)
+{
+ return NULL;
+}
diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
index cd2e3fa7b5..67c3db7dca 100644
--- a/src/network/bridge_driver_platform.h
+++ b/src/network/bridge_driver_platform.h
@@ -30,7 +30,10 @@ void networkPreReloadFirewallRules(virNetworkDriverState *driver,
void networkPostReloadFirewallRules(bool startup);
-int networkCheckRouteCollision(virNetworkDef *def);
+GHashTable *networkSysRoutesTableRead(void);
+const char *networkSysRoutesTableFind(GHashTable *routesTable,
+ virSocketAddr *addr,
+ unsigned int prefix);
int networkAddFirewallRules(virNetworkDef *def,
virFirewallBackend firewallBackend,
--
2.45.2
