From nobody Sat Nov 23 17:57:02 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1723051222562872.0306831471031; Wed, 7 Aug 2024 10:20:22 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 7F81E11B0; Wed, 7 Aug 2024 13:20:21 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id 008FA12DC; Wed, 7 Aug 2024 13:16:18 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 9AA49C92; Wed, 7 Aug 2024 13:16:10 -0400 (EDT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id 9F89CC17 for ; Wed, 7 Aug 2024 13:16:09 -0400 (EDT) Received: from mx-prod-mc-05.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-672-8TOI5S_JNHqfvV81GJBZlg-1; Wed, 07 Aug 2024 13:16:07 -0400 Received: from mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.17]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-05.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id E3D8B1955F45 for ; Wed, 7 Aug 2024 17:16:06 +0000 (UTC) Received: from vhost3.router.laine.org (unknown [10.22.32.31]) by mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 899E519560A3 for ; Wed, 7 Aug 2024 17:16:06 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1723050969; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=pvSKYGnWVs/YAjCtw1PJy50TWutzIE9Gs0L30P8fpY0=; b=QRGNq4kPXxwdeOanDwv0uOVDEcSRqnHSKRrw8qLPLIJgXNPNMy8qXa4O+uKFwjJRw66Bcs HPgBURl8SP6nnz+8GAIqJ30VQo6X+cNAT38s9puVa4Cj0G4itfVzz8iC6iJxNLS+f8In7b wQMKaUuiLVZ/pekgIL+TAxuNnMeYZqc= X-MC-Unique: 8TOI5S_JNHqfvV81GJBZlg-1 From: Laine Stump To: devel@lists.libvirt.org Subject: [PATCH 3/7] network: reorganize the check for route collisions Date: Wed, 7 Aug 2024 13:15:59 -0400 Message-ID: <20240807171603.218784-4-laine@redhat.com> In-Reply-To: <20240807171603.218784-1-laine@redhat.com> References: <20240807171603.218784-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.17 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Message-ID-Hash: YCKI2M36ENUCLBC6J7CR3XSQSK4OY5PH X-Message-ID-Hash: YCKI2M36ENUCLBC6J7CR3XSQSK4OY5PH X-MailFrom: laine@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1723051223578116600 Content-Type: text/plain; charset="utf-8"; x-default="true" 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 --- 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, } =20 =20 +/* 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 =3D 0; (ipdef =3D virNetworkDefGetIPByIndex(def, AF_INET, i)); = i++) { + unsigned int prefix =3D virNetworkIPDefPrefix(ipdef); + const char *iface =3D networkSysRoutesTableFind(routes, &ipdef->ad= dress, prefix); + + if (iface) { + g_autofree char *addrStr =3D virSocketAddrFormat(&ipdef->addre= ss); + + 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 =3D 0; (routedef =3D virNetworkDefGetRouteByIndex(def, AF_INET,= i)); i++) { + virSocketAddr *addr =3D virNetDevIPRouteGetAddress(routedef); + int prefix =3D virNetDevIPRouteGetPrefix(routedef); + const char *iface =3D networkSysRoutesTableFind(routes, addr, pref= ix); + + if (iface) { + g_autofree char *addrStr =3D 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 *d= river, bool firewalRulesAdded =3D false; virSocketAddr *dnsServer =3D NULL; virFirewall *fwRemoval =3D NULL; + g_autoptr(GHashTable) routes =3D networkSysRoutesTableRead(); =20 - /* 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 rout= es */ + if (networkCheckRouteCollision(def, routes) < 0) + return -1; + } =20 /* 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_GN= UC_UNUSED) } =20 =20 -/* 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 =3D key; + const char *valueval =3D value; + + VIR_INFO("key: %016llx value: %s", (unsigned long long)*keyval, valuev= al); +} +/** + * 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 =3D NULL; /* allow for up to 100000 routes (each line is 128 bytes) */ enum {MAX_ROUTE_SIZE =3D 128*100000}; + g_autoptr(GHashTable) routes =3D g_hash_table_new_full(g_int64_hash, g= _int64_equal, g_free, g_free); + =20 /* Read whole routing table into memory */ if ((len =3D virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0) - return 0; + return g_steal_pointer(&routes); =20 /* 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); =20 if (!STRPREFIX(buf, "Iface")) - return 0; + return g_steal_pointer(&routes); =20 /* First line is just headings, skip it */ cur =3D strchr(buf, '\n'); @@ -245,11 +277,9 @@ int networkCheckRouteCollision(virNetworkDef *def) =20 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 =3D g_new(gint64, 1); =20 /* NUL-terminate the line, so sscanf doesn't go beyond a newline. = */ char *nl =3D strchr(cur, '\n'); @@ -275,60 +305,48 @@ int networkCheckRouteCollision(virNetworkDef *def) continue; } =20 - addr_val &=3D 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 =3D ((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); =20 - for (i =3D 0; - (ipdef =3D virNetworkDefGetIPByIndex(def, AF_INET, i)); - i++) { + g_hash_table_replace(routes, g_steal_pointer(&key), g_strdup(iface= )); + } =20 - unsigned int net_dest; - virSocketAddr netmask; + VIR_INFO("Full Route Hash Table: sizeof(gint64): %d sizeof(long long u= nsigned): %d", + (int)sizeof(gint64), (int)sizeof(long long unsigned)); + g_hash_table_foreach(routes, networkPrintRouteTableEntry, NULL); =20 - if (virNetworkIPDefNetmask(ipdef, &netmask) < 0) { - VIR_WARN("Failed to get netmask of '%s'", - def->bridge); - continue; - } + return g_steal_pointer(&routes); +} =20 - net_dest =3D (ipdef->address.data.inet4.sin_addr.s_addr & - netmask.data.inet4.sin_addr.s_addr); =20 - if ((net_dest =3D=3D addr_val) && - (netmask.data.inet4.sin_addr.s_addr =3D=3D 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; =20 - for (i =3D 0; - (routedef =3D virNetworkDefGetRouteByIndex(def, AF_INET, i)); - i++) { - - virSocketAddr r_mask, r_addr; - virSocketAddr *tmp_addr =3D virNetDevIPRouteGetAddress(routede= f); - int r_prefix =3D 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 =3D=3D addr_val) && - (r_mask.data.inet4.sin_addr.s_addr =3D=3D mask_val)) { - g_autofree char *addr_str =3D virSocketAddrFormat(&r_addr); - if (!addr_str) - virResetLastError(); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Route address '%1$s' conflicts with IP a= ddress for '%2$s'"), - NULLSTR(addr_str), iface); - return -1; - } - } - } + if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) + return false; =20 - return 0; + if (virSocketAddrMask(addr, &netmask, &netaddr) < 0) + return false; + + compare =3D ((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); } =20 =20 diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_no= p.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) { } =20 - -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_UN= USED, 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_driv= er_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, =20 void networkPostReloadFirewallRules(bool startup); =20 -int networkCheckRouteCollision(virNetworkDef *def); +GHashTable *networkSysRoutesTableRead(void); +const char *networkSysRoutesTableFind(GHashTable *routesTable, + virSocketAddr *addr, + unsigned int prefix); =20 int networkAddFirewallRules(virNetworkDef *def, virFirewallBackend firewallBackend, --=20 2.45.2