From nobody Sun Feb 8 20:35:04 2026 Delivered-To: importer@patchew.org Received-SPF: none (zohomail.com: 8.43.85.245 is neither permitted nor denied by domain of lists.libvirt.org) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; spf=none (zohomail.com: 8.43.85.245 is neither permitted nor denied by domain of lists.libvirt.org) 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 1713756109673547.6467595200553; Sun, 21 Apr 2024 20:21:49 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 8CAA7213F; Sun, 21 Apr 2024 23:21:48 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id C706D1EDD; Sun, 21 Apr 2024 22:55:13 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id D4C601DC6; Sun, 21 Apr 2024 22:53:52 -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 433B01DF2 for ; Sun, 21 Apr 2024 22:53:41 -0400 (EDT) Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-486-NA7CLnpcPn-zok0vwFkHoA-1; Sun, 21 Apr 2024 22:53:39 -0400 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (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 mimecast-mx02.redhat.com (Postfix) with ESMTPS id 23B3E804C61 for ; Mon, 22 Apr 2024 02:53:39 +0000 (UTC) Received: from vhost3.router.laine.org (unknown [10.22.8.35]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0D9871121306 for ; Mon, 22 Apr 2024 02:53:39 +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=-1.0 required=5.0 tests=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 X-MC-Unique: NA7CLnpcPn-zok0vwFkHoA-1 From: Laine Stump To: devel@lists.libvirt.org Subject: [PATCH v2 20/27] network: use previously saved list of firewall removal commands Date: Sun, 21 Apr 2024 22:53:28 -0400 Message-ID: <20240422025335.923272-21-laine@redhat.com> In-Reply-To: <20240422025335.923272-1-laine@redhat.com> References: <20240422025335.923272-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Message-ID-Hash: DK6WCC44APWNLFULTRYOHSRVUTZYQQTX X-Message-ID-Hash: DK6WCC44APWNLFULTRYOHSRVUTZYQQTX 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: Content-Type: text/plain; charset="utf-8"; x-default="true" Content-Transfer-Encoding: quoted-printable X-ZM-MESSAGEID: 1713756110565100001 When destroying a network, the network driver has always assumed that it knew what firewall rules had been added as the network was started. This was usually correct - I only recall one time in the past that the firewall rules added by libvirt were changed. But if the exact rules used for a network *were* ever changed from one build/version of libvirt to another, then we would end up attempting to remove rules that hadn't been added, and could possibly *not* remove rules that had been added. The solution to this to not make such brash assumptions about the past, but instead to save (in the network status object at network start time) a list of all the rules needed to remove the rules that were added for the network, and then use that saved list during network destroy to remove exactly what was previous added. Beyond making net-destroy more precise, there are other benefits: 1) We can change the details of the rules we add for networks from one build/release of libvirt to another and painlessly upgrade. 2) The user can switch from one firewall backend to another by simply changing the setting in network.conf and restarting libvirtd/virtnetworkd. In both cases, the restarted libvirtd/virtnetworkd will remove all the rules that had been previously added (based on the network status), and then add new rules (saving the new removal commands back into the network status) Signed-off-by: Laine Stump =3D=3D NB: the current implementation saves only the commands necessary to remove the network's firewall, and names this in the status XML. It would be simple to instead save the *entire* virFirewall object (thus also including the commands that were used to add the firewall, as well as the commands needed to remove it) - although very verbose, it's possible it could be useful when debugging a firewall issue (since it's not obvious which rules were added for which network when just looking at the output of "nft list ruleset". Alternately, we could continue to store only the removal commands, but maybe change the name of the element in XML from to (which would leave the door open to expanding what is saved in the future). Any opinions on this? Signed-off-by: Laine Stump Reviewed-by: Daniel P. Berrang=C3=A9 --- src/network/bridge_driver.c | 31 ++++++++++++-------- src/network/bridge_driver_linux.c | 26 +++++++++++++---- src/network/bridge_driver_nop.c | 6 ++-- src/network/bridge_driver_platform.h | 6 ++-- src/network/network_iptables.c | 43 +++++++++++++++++++++++++--- src/network/network_iptables.h | 2 +- tests/networkxml2firewalltest.c | 2 +- 7 files changed, 87 insertions(+), 29 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 38e4ab84ad..c54a595d4d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1696,8 +1696,9 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj, * network type, forward=3D'open', doesn't need this because it * has no iptables rules. */ - networkRemoveFirewallRules(def, cfg->firewallBackend); - ignore_value(networkAddFirewallRules(def, cfg->firewallBackend= )); + networkRemoveFirewallRules(obj); + ignore_value(networkAddFirewallRules(def, cfg->firewallBackend, + virNetworkObjGetFwRemoval= Ptr(obj))); break; =20 case VIR_NETWORK_FORWARD_OPEN: @@ -1949,8 +1950,10 @@ networkStartNetworkVirtual(virNetworkDriverState *dr= iver, =20 /* Add "once per network" rules */ if (def->forward.type !=3D VIR_NETWORK_FORWARD_OPEN && - networkAddFirewallRules(def, cfg->firewallBackend) < 0) + networkAddFirewallRules(def, cfg->firewallBackend, + virNetworkObjGetFwRemovalPtr(obj)) < 0) { goto error; + } =20 firewalRulesAdded =3D true; =20 @@ -2065,7 +2068,7 @@ networkStartNetworkVirtual(virNetworkDriverState *dri= ver, =20 if (firewalRulesAdded && def->forward.type !=3D VIR_NETWORK_FORWARD_OPEN) - networkRemoveFirewallRules(def, cfg->firewallBackend); + networkRemoveFirewallRules(obj); =20 virNetworkObjUnrefMacMap(obj); =20 @@ -2077,8 +2080,7 @@ networkStartNetworkVirtual(virNetworkDriverState *dri= ver, =20 =20 static int -networkShutdownNetworkVirtual(virNetworkObj *obj, - virNetworkDriverConfig *cfg) +networkShutdownNetworkVirtual(virNetworkObj *obj) { virNetworkDef *def =3D virNetworkObjGetDef(obj); pid_t dnsmasqPid; @@ -2104,7 +2106,7 @@ networkShutdownNetworkVirtual(virNetworkObj *obj, ignore_value(virNetDevSetOnline(def->bridge, false)); =20 if (def->forward.type !=3D VIR_NETWORK_FORWARD_OPEN) - networkRemoveFirewallRules(def, cfg->firewallBackend); + networkRemoveFirewallRules(obj); =20 ignore_value(virNetDevBridgeDelete(def->bridge)); =20 @@ -2408,7 +2410,7 @@ networkShutdownNetwork(virNetworkDriverState *driver, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: - ret =3D networkShutdownNetworkVirtual(obj, cfg); + ret =3D networkShutdownNetworkVirtual(obj); break; =20 case VIR_NETWORK_FORWARD_BRIDGE: @@ -3259,7 +3261,7 @@ networkUpdate(virNetworkPtr net, * old rules (and remember to load new ones after the * update). */ - networkRemoveFirewallRules(def, cfg->firewallBackend); + networkRemoveFirewallRules(obj); needFirewallRefresh =3D true; break; default: @@ -3286,16 +3288,21 @@ networkUpdate(virNetworkPtr net, if (virNetworkObjUpdate(obj, command, section, parentIndex, xml, network_driver->xmlopt, flags) < 0) { - if (needFirewallRefresh) - ignore_value(networkAddFirewallRules(def, cfg->firewallBackend= )); + if (needFirewallRefresh) { + ignore_value(networkAddFirewallRules(def, cfg->firewallBackend, + virNetworkObjGetFwRemoval= Ptr(obj))); + } goto cleanup; } =20 /* @def is replaced */ def =3D virNetworkObjGetDef(obj); =20 - if (needFirewallRefresh && networkAddFirewallRules(def, cfg->firewallB= ackend) < 0) + if (needFirewallRefresh && + networkAddFirewallRules(def, cfg->firewallBackend, + virNetworkObjGetFwRemovalPtr(obj)) < 0) { goto cleanup; + } =20 if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { /* save updated persistent config to disk */ diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_= linux.c index 20671e3ec5..61a4a6cdc1 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -333,7 +333,8 @@ int networkCheckRouteCollision(virNetworkDef *def) =20 int networkAddFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend) + virFirewallBackend firewallBackend, + virFirewall **fwRemoval) { =20 networkSetupPrivateChains(firewallBackend, false); @@ -419,13 +420,28 @@ networkAddFirewallRules(virNetworkDef *def, } } =20 - return iptablesAddFirewallRules(def); + return iptablesAddFirewallRules(def, fwRemoval); } =20 =20 void -networkRemoveFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend G_GNUC_UNUSE= D) +networkRemoveFirewallRules(virNetworkObj *obj) { - iptablesRemoveFirewallRules(def); + virFirewall *fw; + + if ((fw =3D virNetworkObjGetFwRemoval(obj)) =3D=3D NULL) { + /* No information about firewall rules in the network status, + * so we assume the old iptables-based rules from 10.2.0 and + * earlier. + */ + VIR_DEBUG("No firewall info in network status, assuming old-style = iptables"); + iptablesRemoveFirewallRules(virNetworkObjGetDef(obj)); + return; + } + + /* fwRemoval info was stored in the network status, so use that to + * remove the firewall + */ + VIR_DEBUG("Removing firewall rules with commands saved in network stat= us"); + virFirewallApply(fw); } diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_no= p.c index 7d9a061e50..537b9234f8 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -37,12 +37,12 @@ int networkCheckRouteCollision(virNetworkDef *def G_GNU= C_UNUSED) } =20 int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, - virFirewallBackend firewallBackend G_GNUC_UNUS= ED) + virFirewallBackend firewallBackend G_GNUC_UNUS= ED, + virFirewall **fwRemoval G_GNUC_UNUSED) { return 0; } =20 -void networkRemoveFirewallRules(virNetworkDef *def G_GNUC_UNUSED, - virFirewallBackend firewallBackend G_GNUC_U= NUSED) +void networkRemoveFirewallRules(virNetworkObj *obj G_GNUC_UNUSED) { } diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driv= er_platform.h index 7443c3129f..cd2e3fa7b5 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -33,7 +33,7 @@ void networkPostReloadFirewallRules(bool startup); int networkCheckRouteCollision(virNetworkDef *def); =20 int networkAddFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend); + virFirewallBackend firewallBackend, + virFirewall **fwRemoval); =20 -void networkRemoveFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend); +void networkRemoveFirewallRules(virNetworkObj *obj); diff --git a/src/network/network_iptables.c b/src/network/network_iptables.c index 467d43c1e9..f774176b3d 100644 --- a/src/network/network_iptables.c +++ b/src/network/network_iptables.c @@ -1591,9 +1591,19 @@ iptablesRemoveIPSpecificFirewallRules(virFirewall *f= w, } =20 =20 -/* Add all rules for all ip addresses (and general rules) on a network */ +/* iptablesAddFirewallrules: + * + * @def - the network that needs an iptables firewall added + * @fwRemoval - if this is not NULL, it points to a pointer + * that should be filled in with a virFirewall object containing + * all the commands needed to remove this firewall at a later time. + * + * Add all rules for all ip addresses (and general rules) on a + * network, and optionally return a virFirewall object containing all + * the rules needed to later remove the firewall that has been added. +*/ int -iptablesAddFirewallRules(virNetworkDef *def) +iptablesAddFirewallRules(virNetworkDef *def, virFirewall **fwRemoval) { size_t i; virNetworkIPDef *ipdef; @@ -1614,10 +1624,35 @@ iptablesAddFirewallRules(virNetworkDef *def) VIR_FIREWALL_TRANSACTION_AUTO_ROLLBAC= K)); iptablesAddChecksumFirewallRules(fw, def); =20 - return virFirewallApply(fw); + if (virFirewallApply(fw) < 0) + return -1; + + if (fwRemoval) { + /* caller wants us to create a virFirewall object that can be + * applied to undo everything that was just done by * virFirewallA= pply() + */ + + if (virFirewallNewFromRollback(fw, fwRemoval) < 0) + return -1; + } + + return 0; } =20 -/* Remove all rules for all ip addresses (and general rules) on a network = */ +/* iptablesRemoveFirewallRules: + * + * @def - the network that needs its iptables firewall rules removed + * + * Remove all rules for all ip addresses (and general rules) on a + * network that is being shut down. + * + * This function assumes the set of iptables rules that were added by + * all versions of libvirt prior to 10.4.0; any libvirt of that + * release or newer may or may not have this same set of rules, and + * should be using the list of commands saved in NetworkObj::fwRemoval + * ( element in the network status XML) to remove the + * network's firewall rules. + */ void iptablesRemoveFirewallRules(virNetworkDef *def) { diff --git a/src/network/network_iptables.h b/src/network/network_iptables.h index cdc143f154..d6ffc15bb0 100644 --- a/src/network/network_iptables.h +++ b/src/network/network_iptables.h @@ -23,7 +23,7 @@ #include "virfirewall.h" #include "network_conf.h" =20 -int iptablesAddFirewallRules(virNetworkDef *def); +int iptablesAddFirewallRules(virNetworkDef *def, virFirewall **fwRemoval); =20 void iptablesRemoveFirewallRules(virNetworkDef *def); =20 diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltes= t.c index e61787daec..93f693a8d7 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -105,7 +105,7 @@ static int testCompareXMLToArgvFiles(const char *xml, if (!(def =3D virNetworkDefParse(NULL, xml, NULL, false))) return -1; =20 - if (networkAddFirewallRules(def, VIR_FIREWALL_BACKEND_IPTABLES) < 0) + if (networkAddFirewallRules(def, VIR_FIREWALL_BACKEND_IPTABLES, NULL) = < 0) return -1; =20 actual =3D actualargv =3D virBufferContentAndReset(&buf); --=20 2.44.0 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org