Re: [PATCH 5/5] network: *un*set the firewalld zone while shutting down a network

On Thu, Sep 05, 2024 at 01:07:59PM -0400, Laine Stump wrote: >When a bridge device for a virtual network had been placed in a >firewalld zone while starting the network, then even after the network >is shut down and the bridge device is deleted, its name will show up in >the list of interfaces for whichever zone it had been in. > >Usually this isn't a problem, but in the case of forward mode='open', >someone might start the network once with a zone specified, then >shut down the network, remove vthe zone from its config, and start it >again; in this case the bridge device would come up using the zone >from the previous time it was started. > >The solution to this is to remove the interface from whatever zone it >is in as the network is being shut down. There is no downside to doing >this, since the device is going to be deleted anyway. Note that >forward mode='bridge' uses a bridge device that was created outside of >libvirt, and libvirt won't be deleting that bridge, so we take care to >not unset the zone in that case. > >Signed-off-by: Laine Stump <laine(a)redhat.com&gt; >--- > src/libvirt_private.syms | 1 + > src/network/bridge_driver.c | 4 ++++ > src/network/bridge_driver_linux.c | 14 ++++++++++++++ > src/network/bridge_driver_nop.c | 6 ++++++ > src/network/bridge_driver_platform.h | 2 ++ > src/util/virfirewalld.c | 23 +++++++++++++++++++++++ > src/util/virfirewalld.h | 2 ++ > 7 files changed, 52 insertions(+) > >diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >index af40e5dca3..f15d16c292 100644 >--- a/src/libvirt_private.syms >+++ b/src/libvirt_private.syms >@@ -2451,6 +2451,7 @@ virFirewallDGetPolicies; > virFirewallDGetVersion; > virFirewallDGetZones; > virFirewallDInterfaceSetZone; >+virFirewallDInterfaceUnsetZone; > virFirewallDIsRegistered; > virFirewallDPolicyExists; > virFirewallDSynchronize; >diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >index 3504d512a0..e457c3bf5e 100644 >--- a/src/network/bridge_driver.c >+++ b/src/network/bridge_driver.c >@@ -2085,6 +2085,8 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, > def->forward.type != VIR_NETWORK_FORWARD_OPEN) > networkRemoveFirewallRules(obj); > >+ networkUnsetBridgeZone(def); >+ > virNetworkObjUnrefMacMap(obj); > > ignore_value(virNetDevBridgeDelete(def->bridge)); >@@ -2123,6 +2125,8 @@ networkShutdownNetworkVirtual(virNetworkObj *obj) > if (def->forward.type != VIR_NETWORK_FORWARD_OPEN) > networkRemoveFirewallRules(obj); > >+ networkUnsetBridgeZone(def); >+ > ignore_value(virNetDevBridgeDelete(def->bridge)); > > /* See if its still alive and really really kill it */ >diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c >index af758d4f3d..3b3608c085 100644 >--- a/src/network/bridge_driver_linux.c >+++ b/src/network/bridge_driver_linux.c >@@ -392,6 +392,20 @@ networkSetBridgeZone(virNetworkDef *def) > } > > >+void >+networkUnsetBridgeZone(virNetworkDef *def) >+{ >+ /* If there is a libvirt-managed bridge device remove it from any >+ * zone it had been placed in as a part of deleting the bridge. >+ * DO NOT CALL THIS FOR 'bridge' forward mode, since that >+ * bridge is not managed by libvirt. >+ */ >+ if (def->bridge && def->forward.type != VIR_NETWORK_FORWARD_BRIDGE >+ && virFirewallDIsRegistered() == 0) { This only unsets the zone if def->bridge is set, but for various scenarios it might not be set by default. For example if def->forward.type != VIR_NETWORK_FORWARD_OPEN and def->bridge is NULL we still set the zone to "libvirt-routed" for VIR_NETWORK_FORWARD_ROUTE and "libvirt" for all the others. At least that's how it looks when reading networkSetBridgeZone() after patch 3/5 or networkAddFirewallRules() in current master.

>+ virFirewallDInterfaceUnsetZone(def->bridge); >+ } >+} >+ > int > networkAddFirewallRules(virNetworkDef *def, > virFirewallBackend firewallBackend, >diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c >index 20c7a2a595..180ff30134 100644 >--- a/src/network/bridge_driver_nop.c >+++ b/src/network/bridge_driver_nop.c >@@ -51,6 +51,12 @@ networkSetBridgeZone(virNetworkDef *def) > } > > >+void >+networkUnsetBridgeZone(virNetworkDef *def) >+{ >+} >+ >+ > int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, > virFirewallBackend firewallBackend, > virFirewall **fwRemoval G_GNUC_UNUSED) >diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h >index 02abdc197f..a0291532a1 100644 >--- a/src/network/bridge_driver_platform.h >+++ b/src/network/bridge_driver_platform.h >@@ -38,4 +38,6 @@ int networkAddFirewallRules(virNetworkDef *def, > virFirewallBackend firewallBackend, > virFirewall **fwRemoval); > >+void networkUnsetBridgeZone(virNetworkDef *def); >+ > void networkRemoveFirewallRules(virNetworkObj *obj); >diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c >index 827e201dbb..4aec33ac45 100644 >--- a/src/util/virfirewalld.c >+++ b/src/util/virfirewalld.c >@@ -449,6 +449,29 @@ virFirewallDInterfaceSetZone(const char *iface, > } > > >+int >+virFirewallDInterfaceUnsetZone(const char *iface) >+{ >+ GDBusConnection *sysbus = virGDBusGetSystemBus(); >+ g_autoptr(GVariant) message = NULL; >+ >+ if (!sysbus) >+ return -1; >+ >+ message = g_variant_new("(ss)", "", iface); >+ >+ return virGDBusCallMethod(sysbus, >+ NULL, >+ NULL, >+ NULL, >+ VIR_FIREWALL_FIREWALLD_SERVICE, >+ "/org/fedoraproject/FirewallD1", >+ "org.fedoraproject.FirewallD1.zone", >+ "removeInterface", >+ message); >+} >+ >+ > void > virFirewallDSynchronize(void) > { >diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h >index 0e94d3507b..0dbe66d435 100644 >--- a/src/util/virfirewalld.h >+++ b/src/util/virfirewalld.h >@@ -46,4 +46,6 @@ int virFirewallDApplyRule(virFirewallLayer layer, > int virFirewallDInterfaceSetZone(const char *iface, > const char *zone); > >+int virFirewallDInterfaceUnsetZone(const char *iface); >+ > void virFirewallDSynchronize(void); >-- >2.46.0 >