From nobody Mon Sep 16 20:17:07 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of redhat.com designates 170.10.133.124 as permitted sender) client-ip=170.10.133.124; envelope-from=libvir-list-bounces@redhat.com; helo=us-smtp-delivery-124.mimecast.com; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1682911274; cv=none; d=zohomail.com; s=zohoarc; b=XR81t2guUXMU+Wwm3mhQk12DDge61lh7LgD6hrPJKXuP6vtOKj9YKq4DXL3uquESzSAP4cDaijVxuo7CPxHCxKnU0g+QPB1aoS3+AfYyJmxE5yFWhPaO2G7qxzfn6y5jXDz8JIoehUCj0Oe1mmwz1Q8yfv5GcpI1QfNp5D8oMHk= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1682911274; h=Content-Type:Content-Transfer-Encoding:Date:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:References:Sender:Subject:To; bh=Xd/qoIGNhi1YLk02bUjeCV81Az82Feva66j+EQWY0HY=; b=cDXsNsnIvOXrWkhk6OMKvNtjByrxvHJg97tQjtMinW8wrkfqiJly+L1FOisIp4qcRKAHAkaWjJvFT2d3aL95KrtHYkYILhMQvDjALUon34QmYAAm9RdC/jjDf/b4/p6dARTP8XuQYiT4dShC9eW6hcsVyz0UrFZcVs4BtD2qR3s= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass header.from= (p=none dis=none) Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.zohomail.com with SMTPS id 1682911274485623.4593548424493; Sun, 30 Apr 2023 20:21:14 -0700 (PDT) Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-627-il70Qz7POYG13NprrAj8FQ-1; Sun, 30 Apr 2023 23:20:30 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D391B3C0F666; Mon, 1 May 2023 03:20:17 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com [10.30.29.100]) by smtp.corp.redhat.com (Postfix) with ESMTP id B47B87D56A; Mon, 1 May 2023 03:20:17 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (localhost [IPv6:::1]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id 96080194E01A; Mon, 1 May 2023 03:20:09 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id D63BB1946A51 for ; Mon, 1 May 2023 03:20:05 +0000 (UTC) Received: by smtp.corp.redhat.com (Postfix) id 29EFE463ED1; Mon, 1 May 2023 03:19:48 +0000 (UTC) Received: from vhost3.router.laine.org (unknown [10.22.8.105]) by smtp.corp.redhat.com (Postfix) with ESMTP id 117E6475026 for ; Mon, 1 May 2023 03:19:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1682911273; h=from:from:sender:sender: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:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=Xd/qoIGNhi1YLk02bUjeCV81Az82Feva66j+EQWY0HY=; b=TpN6qTcHrW3a+VP7sSgAETIWpH7AaJcls9tCQWFBXH0rDEiyBoLY4TD83PptiJG0bCipGX E2e+SjtejlLDOEzv+S/JvY2qjubQJ0mUd7BW6nUEGP/hj6OCiVtvIP0/l9NMS6fdst2jDM H9orL5c9Z4PKj6mcj3OxCaVozaNGqLE= X-MC-Unique: il70Qz7POYG13NprrAj8FQ-1 X-Original-To: libvir-list@listman.corp.redhat.com From: Laine Stump To: libvir-list@redhat.com Subject: [libvirt PATCH 26/28] network: use previously saved list of firewall rules when removing Date: Sun, 30 Apr 2023 23:19:41 -0400 Message-Id: <20230501031943.288145-27-laine@redhat.com> In-Reply-To: <20230501031943.288145-1-laine@redhat.com> References: <20230501031943.288145-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libvir-list-bounces@redhat.com Sender: "libvir-list" X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable X-ZohoMail-DKIM: pass (identity @redhat.com) X-ZM-MESSAGEID: 1682911274921100002 Content-Type: text/plain; charset="utf-8"; x-default="true" 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, 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. As a result of doing this, not only can we change the details of the rules we add for networks from one build/release of libvirt to another and painlessly upgrade, but the user can also switch from one firewall backend to another by simply changing the setting in network.conf and restarting libvirtd/virtnetworkd. Signed-off-by: Laine Stump --- src/network/bridge_driver.c | 34 +++++++++++------ src/network/bridge_driver_linux.c | 56 +++++++++++++++++++++------- src/network/bridge_driver_nop.c | 4 +- src/network/bridge_driver_platform.h | 4 +- tests/networkxml2firewalltest.c | 2 +- 5 files changed, 70 insertions(+), 30 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fb353e449a..9f876d7418 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1698,8 +1698,10 @@ 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, + virNetworkObjGetFwRemoval= Ptr(obj), + cfg->firewallBackend)); break; =20 case VIR_NETWORK_FORWARD_OPEN: @@ -1950,8 +1952,11 @@ 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, + virNetworkObjGetFwRemovalPtr(obj), + cfg->firewallBackend) < 0) { goto error; + } =20 firewalRulesAdded =3D true; =20 @@ -2037,7 +2042,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 @@ -2049,8 +2054,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; @@ -2076,7 +2080,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 @@ -2380,7 +2384,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: @@ -3243,7 +3247,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: @@ -3270,16 +3274,22 @@ 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, + virNetworkObjGetFwRemoval= Ptr(obj), + cfg->firewallBackend)); + } goto cleanup; } =20 /* @def is replaced */ def =3D virNetworkObjGetDef(obj); =20 - if (needFirewallRefresh && networkAddFirewallRules(def, cfg->firewallB= ackend) < 0) + if (needFirewallRefresh && networkAddFirewallRules(def, + virNetworkObjGetFwR= emovalPtr(obj), + cfg->firewallBacken= d) < 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 f6bae334aa..9adf05c05d 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -818,6 +818,7 @@ networkRemoveIPSpecificFirewallRules(virFirewall *fw, /* Add all rules for all ip addresses (and general rules) on a network */ int networkAddFirewallRules(virNetworkDef *def, + virFirewall **fwRemoval, virFirewallBackend firewallBackend) { size_t i; @@ -930,30 +931,59 @@ networkAddFirewallRules(virNetworkDef *def, | VIR_FIREWALL_TRANSACTION_AUTO_ROLLB= ACK)); networkAddChecksumFirewallRules(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 + * virFirewallApply() + */ + if (virFirewallNewFromRollback(fw, fwRemoval) < 0) + return -1; + } + + return 0; } =20 /* Remove all rules for all ip addresses (and general rules) on a network = */ void -networkRemoveFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend) +networkRemoveFirewallRules(virNetworkObj *obj) { size_t i; + virNetworkDef *def =3D virNetworkObjGetDef(obj); virNetworkIPDef *ipdef; - g_autoptr(virFirewall) fw =3D virFirewallNew(firewallBackend); + g_autoptr(virFirewall) fw =3D NULL; + + if ((fw =3D virNetworkObjGetFwRemoval(obj))) { + /* exact list of removal rules was saved + * when the firewall rules were originally added + */ + VIR_DEBUG("Removing exact firewall rules previously saved"); + virNetworkObjSetFwRemoval(obj, NULL); =20 - virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS= ); - networkRemoveChecksumFirewallRules(fw, def); + } else { + /* The firewall rules were added by an older libvirt that + * didn't automatically save removal rules, so we guess + * at what rules were added (NB: any libvirt old enough + * to require this only supported the iptables backend) + */ + VIR_DEBUG("Removing a guess at what firewall rules were previously= saved"); + fw =3D virFirewallNew(VIR_FIREWALL_BACKEND_IPTABLES); =20 - virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS= ); + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ER= RORS); + networkRemoveChecksumFirewallRules(fw, def); =20 - for (i =3D 0; - (ipdef =3D virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); - i++) { - if (networkRemoveIPSpecificFirewallRules(fw, def, ipdef) < 0) - return; + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ER= RORS); + + for (i =3D 0; + (ipdef =3D virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); + i++) { + if (networkRemoveIPSpecificFirewallRules(fw, def, ipdef) < 0) + return; + } + networkRemoveGeneralFirewallRules(fw, def); } - networkRemoveGeneralFirewallRules(fw, def); =20 virFirewallApply(fw); } diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_no= p.c index 7d9a061e50..e73831ccc6 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, + virFirewall **fwRemoval G_GNUC_UNUSED, virFirewallBackend firewallBackend G_GNUC_UNUS= ED) { 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..81305f7a0d 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, + virFirewall **fwRemoval, virFirewallBackend firewallBackend); =20 -void networkRemoveFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend); +void networkRemoveFirewallRules(virNetworkObj *obj); diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltes= t.c index 6e9eca0832..8254fde94e 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -106,7 +106,7 @@ static int testCompareXMLToArgvFiles(const char *xml, if (!(def =3D virNetworkDefParse(NULL, xml, NULL, false))) return -1; =20 - if (networkAddFirewallRules(def, backend) < 0) + if (networkAddFirewallRules(def, NULL, backend) < 0) return -1; =20 actual =3D actualargv =3D virBufferContentAndReset(&buf); --=20 2.39.2