These options are added to network.conf and virNetworkDriverConfig object: autoaddr_start - start of the range of subnets to search (def: "192.168.122.0") autoaddr_end - end of the range of subnets (def: "192.168.255.0") autoaddr_prefix - prefix of these subnets (def: 24) They will be used by the network driver when looking for unused subnets to assign to networks that have "autoaddr='yes'" in one of their <ip> elements. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- meson.build | 14 +++++- meson_options.txt | 4 ++ src/network/bridge_driver_conf.c | 61 ++++++++++++++++++++++++ src/network/bridge_driver_conf.h | 4 ++ src/network/libvirtd_network.aug | 8 +++- src/network/meson.build | 6 +++ src/network/network.conf.in | 11 +++++ src/network/test_libvirtd_network.aug.in | 3 ++ 8 files changed, 109 insertions(+), 2 deletions(-) diff --git a/meson.build b/meson.build index 06d88ad1f3..71da7db741 100644 --- a/meson.build +++ b/meson.build @@ -1646,6 +1646,15 @@ endif if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD') conf.set('WITH_NETWORK', 1) + autoaddr_start = get_option('autoaddr_start') + conf.set_quoted('AUTOADDR_START', autoaddr_start) + + autoaddr_end = get_option('autoaddr_end') + conf.set_quoted('AUTOADDR_END', autoaddr_end) + + autoaddr_prefix = get_option('autoaddr_prefix') + conf.set('AUTOADDR_PREFIX', autoaddr_prefix) + firewall_backend_priority = get_option('firewall_backend_priority') if firewall_backend_priority.length() == 0 if host_machine.system() == 'linux' @@ -2406,7 +2415,10 @@ misc_summary = { } if conf.has('WITH_NETWORK') misc_summary += { - 'firewall backends': firewall_backend_priority, + 'autoaddr_start': autoaddr_start, + 'autoaddr_end': autoaddr_end, + 'autoaddr_prefix': autoaddr_prefix, + 'firewall backends': firewall_backend_priority, } endif summary(misc_summary, section: 'Miscellaneous', bool_yn: true, list_sep: ' ') diff --git a/meson_options.txt b/meson_options.txt index 2d440c63d8..7a1a86211e 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -118,6 +118,10 @@ option('firewalld', type: 'feature', value: 'auto', description: 'firewalld supp # dep:firewalld option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether to install firewalld libvirt zone') option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], value: [], description: 'order in which to try firewall backends') +option('autoaddr_start', type: 'string', value: '192.168.122.0', description: 'Start of range of IPv4 subnets to choose an unused subnet from') +option('autoaddr_end', type: 'string', value: '192.168.255.0', description: 'End of range of IPv4 subnets to choose an unused subnet from') +option('autoaddr_prefix', type: 'integer', value: 24, description: 'prefix of IPv4 subnets to choose an unused subnet from') + option('host_validate', type: 'feature', value: 'auto', description: 'build virt-host-validate') option('init_script', type: 'combo', choices: ['systemd', 'openrc', 'check', 'none'], value: 'check', description: 'Style of init script to install') option('loader_nvram', type: 'string', value: '', description: 'Pass list of pairs of <loader>:<nvram> paths. Both pairs and list items are separated by a colon.') diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c index 9da5e790b7..cb2915550f 100644 --- a/src/network/bridge_driver_conf.c +++ b/src/network/bridge_driver_conf.c @@ -65,6 +65,13 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, const char *filename) { g_autoptr(virConf) conf = NULL; + + const char *autoaddrStart = "192.168.122.0"; + const char *autoaddrEnd = "192.168.255.0"; + unsigned int autoaddrPrefix = 24; + g_autofree char *autoaddrStartFromConf = NULL; + g_autofree char *autoaddrEndFromConf = NULL; + g_autofree char *fwBackendStr = NULL; bool fwBackendSelected = false; size_t i; @@ -88,6 +95,36 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, /* use virConfGetValue*(conf, ...) functions to read any settings into cfg */ + if (virConfGetValueString(conf, "autoaddr_start", &autoaddrStartFromConf) < 0) + return -1; + if (autoaddrStartFromConf) { + VIR_DEBUG("autoaddr_start setting requested from config file %s: '%s'", + filename, autoaddrStartFromConf); + autoaddrStart = autoaddrStartFromConf; + } + + if (virConfGetValueString(conf, "autoaddr_end", &autoaddrEndFromConf) < 0) + return -1; + if (autoaddrEndFromConf) { + VIR_DEBUG("autoaddr_end setting requested from config file %s: '%s'", + filename, autoaddrEndFromConf); + autoaddrEnd = autoaddrEndFromConf; + } + + switch (virConfGetValueUInt(conf, "autoaddr_prefix", &autoaddrPrefix)) { + case 1: + VIR_DEBUG("autoaddr_prefix setting requested from config file %s: '%u'", + filename, autoaddrPrefix); + break; + + case 0: + break; + + case -1: + default: + return -1; + } + if (virConfGetValueString(conf, "firewall_backend", &fwBackendStr) < 0) return -1; @@ -106,6 +143,30 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, } } + if (virSocketAddrParse(&cfg->autoaddrStart, autoaddrStart, AF_INET) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid autoaddr_start '%1$s' in network driver config file %2$s, must be numeric IPv4 network address"), + autoaddrStart, filename); + return -1; + } + + if (virSocketAddrParse(&cfg->autoaddrEnd, autoaddrEnd, AF_INET) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid autoaddr_end '%1$s' in network driver config file %2$s, must be numeric IPv4 network address"), + autoaddrStart, filename); + return -1; + } + + if ((cfg->autoaddrPrefix = autoaddrPrefix) > 32) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid IPv4 prefix '%1$u' in network driver config file %2$s"), + autoaddrPrefix, filename); + return -1; + } + + VIR_INFO("autoaddr_start='%s' autoaddr_end='%s' autoaddr_prefix='%u'", + autoaddrStart, autoaddrEnd, autoaddrPrefix); + for (i = 0; i < nFwBackends && !fwBackendSelected; i++) { switch ((virFirewallBackend)fwBackends[i]) { diff --git a/src/network/bridge_driver_conf.h b/src/network/bridge_driver_conf.h index 8f221f391e..7ebf77037d 100644 --- a/src/network/bridge_driver_conf.h +++ b/src/network/bridge_driver_conf.h @@ -39,6 +39,10 @@ struct _virNetworkDriverConfig { char *pidDir; char *dnsmasqStateDir; + virSocketAddr autoaddrStart; + virSocketAddr autoaddrEnd; + unsigned int autoaddrPrefix; + virFirewallBackend firewallBackend; }; diff --git a/src/network/libvirtd_network.aug b/src/network/libvirtd_network.aug index 5d6d72dd92..5212505e1f 100644 --- a/src/network/libvirtd_network.aug +++ b/src/network/libvirtd_network.aug @@ -22,10 +22,16 @@ module Libvirtd_network = let int_entry (kw:string) = [ key kw . value_sep . int_val ] let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ] + let autoaddr_entry = str_entry "autoaddr_start" + | str_entry "autoaddr_end" + | int_entry "autoaddr_prefix" + let firewall_backend_entry = str_entry "firewall_backend" (* Each entry in the config is one of the following *) - let entry = firewall_backend_entry + let entry = autoaddr_entry + | firewall_backend_entry + let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/network/meson.build b/src/network/meson.build index 07cd5cda55..8faff6eb1c 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -51,6 +51,9 @@ if conf.has('WITH_NETWORK') } network_options_conf = configuration_data({ + 'AUTOADDR_START': autoaddr_start, + 'AUTOADDR_END': autoaddr_end, + 'AUTOADDR_PREFIX': autoaddr_prefix, 'FIREWALL_BACKEND_PRIORITY': ', '.join(firewall_backend_priority), 'FIREWALL_BACKEND': firewall_backend_priority[0], }) @@ -62,6 +65,9 @@ if conf.has('WITH_NETWORK') ) network_options_hack_conf = configuration_data({ + 'AUTOADDR_START': autoaddr_start, + 'AUTOADDR_END': autoaddr_end, + 'AUTOADDR_PREFIX': autoaddr_prefix, 'FIREWALL_BACKEND_PRIORITY': ', '.join(firewall_backend_priority), 'FIREWALL_BACKEND': firewall_backend_priority[0], # This hack is necessary because the output file is going to be diff --git a/src/network/network.conf.in b/src/network/network.conf.in index 5ed64a04a5..83a27df04c 100644 --- a/src/network/network.conf.in +++ b/src/network/network.conf.in @@ -27,3 +27,14 @@ # reloaded using the new backend.) # #firewall_backend = "@FIREWALL_BACKEND@" +# +# autoaddr_start +# autoaddr_end +# autoaddr_prefix +# +# These three setting specify the range of subnets that should be used +# for networks that have "autoaddr='yes'" +# +#autoaddr_start = "@AUTOADDR_START@" +#autoaddr_end = "@AUTOADDR_END@" +#autoaddr_prefix = @AUTOADDR_PREFIX@ diff --git a/src/network/test_libvirtd_network.aug.in b/src/network/test_libvirtd_network.aug.in index 9e29a9192f..6edcbaed62 100644 --- a/src/network/test_libvirtd_network.aug.in +++ b/src/network/test_libvirtd_network.aug.in @@ -3,3 +3,6 @@ module Test_libvirtd_network = test Libvirtd_network.lns get conf = { "firewall_backend" = "@FIREWALL_BACKEND@" } +{ "autoaddr_start" = "@AUTOADDR_START@" } +{ "autoaddr_end" = "@AUTOADDR_END@" } +{ "autoaddr_prefix" = "@AUTOADDR_PREFIX@" } -- 2.45.2

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(a)redhat.com&gt; --- 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

There has been a problem for several years with libvirt's default virtual network conflicting with the host physical network connection on new installs, particularly when the "host" is actually a virtual machine that is, itself, connected to the libvirt default network on its respective host. If the two default networks use the same subnet, and if the nested host's libvirt happens to start its network before the system networking connects to the L0 host, then network connectivity to the L1 guest is just silently non-working. We've tried several things over the years to eliminate this problem, including: 1) Checking for conflicting routes/interfaces during installation of the libvirt-daemon-config-network package (the one containing the default network config file) which tries different subnets until it finds one that doesn't conflict. This helps in some cases, but fails on 2 points: a) if the installation environment is different from the environment where the system is actually run (e.g. a "live CD" image of a distro, which is built in a container by the distro maintainers, then could later run in any number of places, and b) it modifies the installed files during the rpm installation %post script, which is now discouraged because people building container images don't like dealing with that. 2) When starting a libvirt network, we now check for any route or interface that conflicts with the new network's IP's and routes. This doesn't fix the problem, but does notify the user of the problem *as long as libvirt starts its networks after the host has started its system networks*. 3) New code (in the commits immediately previous to this one) add support for an "autoaddr" attribute in each virtual network <ip> element; when autoaddr is set, the network driver goes one step beyond (2) and actually finds an unused subnet and sets the new virtual network's addresses accordingly. These are all nice in their own ways, but none of them helps in the situation where libvirt's networks are started first (before the host's own network connections are all up). This led to this patch, which does the following: 4) Using a NetworkManager facility (dispatcher.d pscripts, which are run whenever any interface is brought up or down), check for any libvirt networks that conflict with a newly started NetworkManager interface, and if a conflict is found then log a message and destroy the libvirt network. Most usefully, though, if this destroyed network has autoaddr='yes' then the script will immediately restart the network, which will find a new, unused subnet. Once this is in place, the only issues are: 1) It only works with NetworkManager. But of course almost all of the cases where this problem has been an issue, networking is managed by NetworkManager. 2) If there are guests already running and connected to the network, they will be disconnected, and won't be reconnected until libvirtd/virtqemud is restarted (one of the things the QEMU driver does when rereading the status of active guests is to make sure all their interfaces are connected to their respective networks). Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- libvirt.spec.in | 2 + src/network/meson.build | 6 + src/network/nm-dispatcher-check-nets.py | 196 ++++++++++++++++++++++++ 3 files changed, 204 insertions(+) create mode 100755 src/network/nm-dispatcher-check-nets.py diff --git a/libvirt.spec.in b/libvirt.spec.in index 29101e74fe..51cecfa598 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -603,6 +603,7 @@ Network filter configuration files for cleaning guest traffic Summary: Network driver plugin for the libvirtd daemon Requires: libvirt-daemon-common = %{version}-%{release} Requires: libvirt-libs = %{version}-%{release} +Requires: python3-libxml2 Requires: dnsmasq >= 2.41 %if %{prefer_nftables} Requires: nftables @@ -2151,6 +2152,7 @@ exit 0 %dir %attr(0755, root, root) %{_localstatedir}/lib/libvirt/dnsmasq/ %attr(0755, root, root) %{_libexecdir}/libvirt_leaseshelper %{_libdir}/libvirt/connection-driver/libvirt_driver_network.so +%{_prefix}/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets %{_mandir}/man8/virtnetworkd.8* %if %{with_firewalld_zone} %{_prefix}/lib/firewalld/zones/libvirt.xml diff --git a/src/network/meson.build b/src/network/meson.build index 8faff6eb1c..f620407759 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -169,4 +169,10 @@ if conf.has('WITH_NETWORK') rename: [ 'libvirt-routed-in.xml' ], ) endif + + install_data( + 'nm-dispatcher-check-nets.py', + install_dir: prefix / 'lib' / 'NetworkManager' / 'dispatcher.d', + rename: [ '50-libvirt-check-nets' ], + ) endif diff --git a/src/network/nm-dispatcher-check-nets.py b/src/network/nm-dispatcher-check-nets.py new file mode 100755 index 0000000000..454c434c88 --- /dev/null +++ b/src/network/nm-dispatcher-check-nets.py @@ -0,0 +1,196 @@ +#!/usr/bin/env python3 +# +# Copyright (C) 2012-2019 Red Hat, Inc. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library. If not, see +# <http://www.gnu.org/licenses/>;. + +import libvirt +import sys +import os +import libxml2 +from ipaddress import ip_network + +# This script should be installed in +# /usr/lib/NetworkManager/dispatcher.d/50-libvirt-check-nets. It will be +# called by NetworkManager every time a network interface is taken up +# or down. When a new network comes up, it checks the libvirt virtual +# networks to see if their IP address(es) (including any static +# routes) are in conflict with the IP address(es) (or static routes) +# of the newly added interface. If so, the libvirt network is +# disabled. It is assumed that the user will notice that their guests +# no longer have network connectvity (and/or the message logged by +# this script), see that the network has been disabled, and then +# realize the conflict when they try to restart it. +# +# set checkDefaultOnly=False to check *all* active libvirt networks +# for conflicts with the new interface. Set to True to check only the +# libvirt default network (since most networks other than the default +# network are added post-install at a time when all of the hosts other +# networks are already active, it may be overkill to check all of the +# libvirt networks for conflict here (and instead just add more +# needless overheard to bringing up a new host interface). +# +checkDefaultOnly = False + +# NB: since this file is installed in /usr/lib, it really shouldn't be +# modified by the user, but instead should be copied to +# /etc/NetworkManager/dispatcher.d, where it will override the copy in +# /usr/lib. Even that isn't a proper solution though - if we're going +# to actually have this config knob, perhaps we should check for it in +# the environment, and if someone wants to modify it they can put a +# short script in /etc that exports and environment variable and then +# calls this script? Just thinking out loud here. + + +def checkconflict(conn, netname, hostnets, hostif): + + # ignore if the network has been brought down or removed since we + # got the list + try: + net = conn.networkLookupByName(netname) + except libvirt.libvirtError: + return + + if not net.isActive(): + return + + xml = net.XMLDesc() + doc = libxml2.parseDoc(xml) + ctx = doc.xpathNewContext() + + # see if NetworkManager is informing us that this libvirt network + # itself is coming online + bridge = ctx.xpathEval("/network/bridge/@name") + if bridge and bridge[0].content == hostif: + return + + # check *all* the addresses of this network + addrs = ctx.xpathEval("/network/*[@address]") + for ip in addrs: + ctx.setContextNode(ip) + address = ctx.xpathEval("@address") + prefix = ctx.xpathEval("@prefix") + netmask = ctx.xpathEval("@netmask") + autoaddr = ctx.xpathEval("@autoaddr") + + isAutoaddr = False + if autoaddr and len(autoaddr[0].content): + isAutoaddr = (autoaddr[0].content == "yes") + + if not (address and len(address[0].content)): + continue + + addrstr = address[0].content + if not (prefix and len(prefix[0].content)): + # check for a netmask + if not (netmask and len(netmask[0].content)): + # this element has address, but no prefix or netmask + # probably it is <mac address ...> so we can ignore it + continue + # convert netmask to prefix + prefixstr = str(ip_network("0.0.0.0/%s" % netmask[0].content).prefixlen) + else: + prefixstr = prefix[0].content + + virtnetaddress = ip_network("%s/%s" % (addrstr, prefixstr), strict=False) + + for hostnet in hostnets: + if virtnetaddress == hostnet: + # There is a conflict with this libvirt network and the specified + # net, so we need to disable the libvirt network + print("Stopping libvirt network '%s' because its subnet %s conflicts with newly started interface '%s'')" + % (netname, str(hostnet), hostif)) + try: + net.destroy() + except libvirt.libvirtError: + print("Failed to destroy network %s" % netname) + return + + if isAutoaddr: + print("Restarting autoaddr libvirt network '%s'with new subnet" % (netname)) + try: + net.create() + except libvirt.libvirtError: + print("Failed to restart network '%s'" % netname) + return + return + + +def addHostNets(hostnets, countenv, addrenv): + + count = os.getenv(countenv) + if not count or count == 0: + return + + for num in range(int(count)): + addrstr = os.getenv("%s_%d" % (addrenv, num)) + if not addrstr or addrstr == "": + continue + + net = ip_network(addrstr.split()[0], strict=False) + if net: + hostnets.append(net) + return + + +############################################################ + +if sys.argv[2] != "up": + sys.exit(0) + +hostif = sys.argv[1] + +try: + conn = libvirt.open(None) +except libvirt.libvirtError: + print('Failed to open connection to the hypervisor') + sys.exit(0) + +if checkDefaultOnly: + nets = [] + net = conn.networkLookupByName("default") + if not (net and net.isActive()): + sys.exit(0) + nets.append(net) +else: + nets = conn.listAllNetworks(libvirt.VIR_CONNECT_LIST_NETWORKS_ACTIVE) + if not nets: + sys.exit(0) + +# We have at least one active network. Build a list of all network +# routes added by the new interface, and compare that list to the list +# of all networks used by each active libvirt network. If any are an +# exact match, then we have a conflict and need to shut down the +# libvirt network to avoid killing host networking. + +# When NetworkManager calls scripts in /etc/NetworkManager/dispatcher.d +# it will have all IP addresses and routes associated with the interface +# that is going up or down in the following environment variables: +# +# IP4_NUM_ADDRESSES - number of IPv4 addresses +# IP4_ADDRESS_N - one variable for each address, starting at _0 +# IP4_NUM_ROUTES - number of IPv5 routes +# IP4_ROUTE_N - one for each route, starting at _0 +# (replace "IP4" with "IP6" and repeat) +# +hostnets = [] +addHostNets(hostnets, "IP4_NUM_ADDRESSES", "IP4_ADDRESS") +addHostNets(hostnets, "IP4_NUM_ROUTES", "IP4_ROUTE") +addHostNets(hostnets, "IP6_NUM_ADDRESSES", "IP6_ADDRESS") +addHostNets(hostnets, "IP6_NUM_ROUTES", "IP6_ROUTE") + +for net in nets: + + checkconflict(conn, net.name(), hostnets, hostif) -- 2.45.2

On 8/7/24 1:32 PM, Daniel P. Berrangé wrote: Possibly not *exactly* what's in virtnetworkd, since I'm not certain if NM includes all static routes in the list it puts in the environment variables (while they *would* show up in the system routing table that our network driver uses). Of course that sounds like another reason to prefer re-using the existing code! That sounds simpler (and probably quicker), and if we could do it that I would prefer it ("only works with NetworkManager" is #1 on my list of 'issues' above after all :-)), but what would we do in the case that virtnetworkd (or libvirtd) wasn't currently running? Run some virsh command to trigger virtnetworkd to start, and then send the SIGUSR2? (this does I imagine *something* is sent out, and it's definitely worth me investigating. But wouldn't virtnetworkd have to be running all of the time in order to use something like that?

On Wed, Aug 07, 2024 at 01:16:02PM -0400, Laine Stump wrote: What I find unsettling is that we're providing an address + netmask here, along with a DHCP range, but there's no guarantee any of these are within the start+end addresses in network.conf I'm thinking that perhaps autoaddr='yes' should be mutually exclusive with existence of an explicit address + DHCP range. ie only permit <ip autoaddr='yes'> <dhcp/> </ip> on the basis that if someone wants explicit control over the DHCP range, then they probably shouldn't be relying on auto-addr usage. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Wed, Aug 07, 2024 at 02:15:16PM -0400, Laine Stump wrote: Hmmm, it would be wierd for users to have the DHCP range / static hosts under a different network from the primary host IP, but users are known todo wierd things. So if someonme has a config like: <ip autoaddr='yes' address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.42.2' end='192.168.42.254'/> </dhcp> And we detect a clash for 192.168.122.0/24, IIUC, you're saying we'll cyhang the DHCP range to 192.168.123.2->192.168.123.254, even though the DHCP range was on a different subnet originally. That'd be quite susprising to me. I think we should enforce that if you have autoaddr=yes, that all DHCP/static host IPs are on the same subnet as the primary network IP. Yeah, removing the addrs doesn't actually simplify, since we need to then store the exact same info somewhere else. So what you've done is better, if we can define sane semantics when the DHCP/static hosts are on different subnets by forbidding that config. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Since the default network now has autoaddr='yes', there is no need to waste time during %post install looking for an unused network and modifying the config of the default virtual network. (It would be even simpler to just install the files directly to /etc rather than installing to the examples and then copying to /etc, but if we did that then someone who had manually net-undefined the default network would keep getting it reinstalled every time there was an update to libvirt-daemon-config-network. Of course these days someone who wants to do that can simply dnf rm daemon-config-network if they didn't want the stock default network config, but nobody will be expecting that they have to do that, leading to hundreds of bug reports about "I deleted the default network and it comes back every time I update my packages!") Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- libvirt.spec.in | 36 +++++------------------------------- 1 file changed, 5 insertions(+), 31 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 51cecfa598..cb62e0dcdb 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1949,38 +1949,12 @@ exit 0 %libvirt_systemd_config_pre virtnetworkd %post daemon-config-network +# Installing during %post via cp rather than directly as a part of the package manifest +# prevents us from reinstalling on update in cases where the user has net-undefined +# the default network (and so doesn't *want* it to be reinstalled) if test $1 -eq 1 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml ; then - # see if the network used by default network creates a conflict, - # and try to resolve it - # NB: 192.168.122.0/24 is used in the default.xml template file; - # do not modify any of those values here without also modifying - # them in the template. - orig_sub=122 - sub=${orig_sub} - nl=' -' - routes="${nl}$(ip route show | cut -d' ' -f1)${nl}" - case ${routes} in - *"${nl}192.168.${orig_sub}.0/24${nl}"*) - # there was a match, so we need to look for an unused subnet - for new_sub in $(seq 124 254); do - case ${routes} in - *"${nl}192.168.${new_sub}.0/24${nl}"*) - ;; - *) - sub=$new_sub - break; - ;; - esac - done - ;; - *) - ;; - esac - - sed -e "s/${orig_sub}/${sub}/g" \ - < %{_datadir}/libvirt/networks/default.xml \ - > %{_sysconfdir}/libvirt/qemu/networks/default.xml + cp %{_datadir}/libvirt/networks/default.xml \ + %{_sysconfdir}/libvirt/qemu/networks/default.xml ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml # libvirt saves this file with mode 0600 chmod 0600 %{_sysconfdir}/libvirt/qemu/networks/default.xml -- 2.45.2

On 8/7/24 1:54 PM, Daniel P. Berrangé wrote: Not when all of the other patches are also applied - when the network is started the first time and chooses 192.168.123.0, that will be saved in the network's config XML (just like we re-save the config when mac addresses and uuids are changed by libvirt), and so the next time it is started (in your example, the next time the host boots), it would start out by trying 192.168.123.0 again, and only switch to a different network if that was already in use. And with the NM dispatcher script, even if another network starts at 192.168.123.0 after this network starts, this network will switch to yet another network (and, again, save the config with that new address so it will be used again next time). The only remaining issue (which I pointed out in one of the commit logs) is that if guests have already started by the time the NM script forces the network to change, then those guests would have been started with 192.168.123.0, but then the network is changed to 192.168.124.0 Yeah, that's the way it works. Except without a new file to keep track of.