[PATCH 0/5] network: fix regression in firewalld zone setting
commit v10.7.0-76-g1a72b83d56 improperly assumed that reloading firewalld wouldn't reset the firewalld zone of libvirt-managed bridge devices. This resulted in loss of networking to guests when something on the host triggered a reload of firewalld rules, reported here: https://issues.redhat.com/browse/RHEL-61576 This new series of patches, reverts that commit, along with commit v10.7.0-78-g200f60b2e1, then reimplements their functionality assuming that a firewalld reload *will* reset the zone of all libvirt-managed bridge devices. Laine Stump (5): Revert "network: *un*set the firewalld zone while shutting down a network" Revert "network: support setting firewalld zone for bridge device of open networks" network: call network(Add|Remove)FirewallRules() for forward mode='open' network: a different way of supporting firewalld zone for mode='open' networks network: a different implementation of *un*setting firewalld zone when network is destroyed src/network/bridge_driver.c | 34 +++---- src/network/bridge_driver_linux.c | 140 ++++++++++++++++----------- src/network/bridge_driver_nop.c | 19 ---- src/network/bridge_driver_platform.h | 4 - src/util/virfirewalld.c | 16 +-- 5 files changed, 102 insertions(+), 111 deletions(-) -- 2.46.1
This reverts commit 200f60b2e12e68d618f6d59f0173bb507b678838. The same functionality will be re-added in a different way in an upcoming patch. Signed-off-by: Laine Stump Signed-off-by: Laine Stump <laine@redhat.com> --- 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 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e09fb98596..cafb41166b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2452,7 +2452,6 @@ virFirewallDGetPolicies; virFirewallDGetVersion; virFirewallDGetZones; virFirewallDInterfaceSetZone; -virFirewallDInterfaceUnsetZone; virFirewallDIsRegistered; virFirewallDPolicyExists; virFirewallDSynchronize; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 74ba59b4e9..c9c6fcbccc 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2127,8 +2127,6 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, def->forward.type != VIR_NETWORK_FORWARD_OPEN) networkRemoveFirewallRules(obj); - networkUnsetBridgeZone(def); - virNetworkObjUnrefMacMap(obj); ignore_value(virNetDevBridgeDelete(def->bridge)); @@ -2167,8 +2165,6 @@ 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 3b3608c085..af758d4f3d 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -392,20 +392,6 @@ 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) { - 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 831a5a5010..20c7a2a595 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -51,12 +51,6 @@ networkSetBridgeZone(virNetworkDef *def) } -void -networkUnsetBridgeZone(virNetworkDef *def G_GNUC_UNUSED) -{ -} - - 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 a0291532a1..02abdc197f 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -38,6 +38,4 @@ 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 4aec33ac45..827e201dbb 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -449,29 +449,6 @@ 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 0dbe66d435..0e94d3507b 100644 --- a/src/util/virfirewalld.h +++ b/src/util/virfirewalld.h @@ -46,6 +46,4 @@ int virFirewallDApplyRule(virFirewallLayer layer, int virFirewallDInterfaceSetZone(const char *iface, const char *zone); -int virFirewallDInterfaceUnsetZone(const char *iface); - void virFirewallDSynchronize(void); -- 2.46.1
This reverts commit 1a72b83d566df952033529001b0f88a66d7f4393. That patch had made the incorrect assumption that the firewalld zone of a bridge would not be changed/removed when firewalld reloaded its rules (e.g. with "killall -HUP firewalld"). It turns out my memory was faulty, and this *does* remove the bridge interface's zone, which results in guest networking failure after a firewalld reload, until the virtual network is restarted. The functionality reverted as a result of this patch reversion will be added back in an upcoming patch that keeps the zone setting in networkAddFirewallRules() (rather than moving it into a separate function) so that it is called every time the network's firewall rules are reloaded (including the reload that happens in response to a reload notification from firewalld). Signed-off-by: Laine Stump Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 4 -- src/network/bridge_driver_linux.c | 61 ++++++++++++---------------- src/network/bridge_driver_nop.c | 13 ------ src/network/bridge_driver_platform.h | 2 - 4 files changed, 26 insertions(+), 54 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c9c6fcbccc..fe053f423a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1999,10 +1999,6 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, if (networkSetIPv6Sysctls(obj) < 0) goto error; - /* set the firewall zone for the bridge device on the host */ - if (networkSetBridgeZone(def) < 0) - goto error; - /* Add "once per network" rules */ if (def->forward.type != VIR_NETWORK_FORWARD_OPEN && networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0) { diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index af758d4f3d..5981e3bd19 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -333,8 +333,28 @@ int networkCheckRouteCollision(virNetworkDef *def) int -networkSetBridgeZone(virNetworkDef *def) +networkAddFirewallRules(virNetworkDef *def, + virFirewallBackend firewallBackend, + virFirewall **fwRemoval) { + + networkSetupPrivateChains(firewallBackend, false); + + if (errInitV4 && + (virNetworkDefGetIPByIndex(def, AF_INET, 0) || + virNetworkDefGetRouteByIndex(def, AF_INET, 0))) { + virSetError(errInitV4); + return -1; + } + + if (errInitV6 && + (virNetworkDefGetIPByIndex(def, AF_INET6, 0) || + virNetworkDefGetRouteByIndex(def, AF_INET6, 0) || + def->ipv6nogw)) { + virSetError(errInitV6); + return -1; + } + if (def->bridgeZone) { /* if a firewalld zone has been specified, fail/log an error @@ -350,14 +370,12 @@ networkSetBridgeZone(virNetworkDef *def) if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0) return -1; - } else if (def->forward.type != VIR_NETWORK_FORWARD_OPEN) { + } else { - /* if firewalld is active, try to set the "libvirt" zone by - * default (forward mode='open' networks have no zone set by - * default, but we honor it if one is specified). This is - * desirable (for consistency) if firewalld is using the - * iptables backend, but is necessary (for basic network - * connectivity) if firewalld is using the nftables backend + /* if firewalld is active, try to set the "libvirt" zone. This is + * desirable (for consistency) if firewalld is using the iptables + * backend, but is necessary (for basic network connectivity) if + * firewalld is using the nftables backend */ if (virFirewallDIsRegistered() == 0) { @@ -388,33 +406,6 @@ networkSetBridgeZone(virNetworkDef *def) } } - return 0; -} - - -int -networkAddFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend, - virFirewall **fwRemoval) -{ - - networkSetupPrivateChains(firewallBackend, false); - - if (errInitV4 && - (virNetworkDefGetIPByIndex(def, AF_INET, 0) || - virNetworkDefGetRouteByIndex(def, AF_INET, 0))) { - virSetError(errInitV4); - return -1; - } - - if (errInitV6 && - (virNetworkDefGetIPByIndex(def, AF_INET6, 0) || - virNetworkDefGetRouteByIndex(def, AF_INET6, 0) || - def->ipv6nogw)) { - virSetError(errInitV6); - return -1; - } - switch (firewallBackend) { case VIR_FIREWALL_BACKEND_NONE: virReportError(VIR_ERR_NO_SUPPORT, "%s", diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c index 20c7a2a595..8bf3367bff 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -38,19 +38,6 @@ int networkCheckRouteCollision(virNetworkDef *def G_GNUC_UNUSED) return 0; } - -int -networkSetBridgeZone(virNetworkDef *def) -{ - if (def->bridgeZone) { - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("This platform does not support setting the bridge device zone")); - return -1; - } - return 0; -} - - 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..cd2e3fa7b5 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -32,8 +32,6 @@ void networkPostReloadFirewallRules(bool startup); int networkCheckRouteCollision(virNetworkDef *def); -int networkSetBridgeZone(virNetworkDef *def); - int networkAddFirewallRules(virNetworkDef *def, virFirewallBackend firewallBackend, virFirewall **fwRemoval); -- 2.46.1
Previously networkAddFirewallRules() and networkRemoveFirewallRules() were only called if the forward mode was none, 'route', or 'nat', so those functions didn't check the forward mode. Although their current contents shouldn't be executed for forward mode='open', soon they will have extra functionality that should be executed for all the current forward modes and also mode='open'. This patch modifies all places either of the functions are called to make sure they are called for mode='open' in addition to current modes (by either adding 'case ..._OPEN:' to the case of a switch statement, or just removing an 'if (mode != ...OPEN)' around the calls; to balance out for that, it puts the entirety of the contents of both functions inside if (mode != ...OPEN) to retain current behavior. (an upcoming patch will add code outside that if clause). debug log messages were also added to make it easier to test that the right thing is being done in all cases. Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 26 ++--- src/network/bridge_driver_linux.c | 175 +++++++++++++++++------------- 2 files changed, 110 insertions(+), 91 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fe053f423a..f604b2695c 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1735,10 +1735,15 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj, case VIR_NETWORK_FORWARD_NONE: case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: - /* Only three of the L3 network types that are configured by - * libvirt need to have iptables rules reloaded. The 4th L3 - * network type, forward='open', doesn't need this because it - * has no iptables rules. + case VIR_NETWORK_FORWARD_OPEN: + /* even 'open' forward type networks need to call + * networkAdd/RemoveFirewallRules() in spite of the fact + * that, by definition, libvirt doesn't add any firewall + * rules for those networks.. This is because libvirt + * *does* support explicitly naming (in the config) a + * firewalld zone the network's bridge should be added to, + * and this functionality is also handled by + * networkAdd/RemoveFirewallRules() */ networkRemoveFirewallRules(obj); ignore_value(networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval)); @@ -1746,7 +1751,6 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj, saveStatus = true; break; - case VIR_NETWORK_FORWARD_OPEN: case VIR_NETWORK_FORWARD_BRIDGE: case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: @@ -2000,10 +2004,8 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, goto error; /* Add "once per network" rules */ - if (def->forward.type != VIR_NETWORK_FORWARD_OPEN && - networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0) { + if (networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0) goto error; - } virNetworkObjSetFwRemoval(obj, fwRemoval); firewalRulesAdded = true; @@ -2119,8 +2121,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, if (devOnline) ignore_value(virNetDevSetOnline(def->bridge, false)); - if (firewalRulesAdded && - def->forward.type != VIR_NETWORK_FORWARD_OPEN) + if (firewalRulesAdded) networkRemoveFirewallRules(obj); virNetworkObjUnrefMacMap(obj); @@ -2158,8 +2159,7 @@ networkShutdownNetworkVirtual(virNetworkObj *obj) ignore_value(virNetDevSetOnline(def->bridge, false)); - if (def->forward.type != VIR_NETWORK_FORWARD_OPEN) - networkRemoveFirewallRules(obj); + networkRemoveFirewallRules(obj); ignore_value(virNetDevBridgeDelete(def->bridge)); @@ -3307,6 +3307,7 @@ networkUpdate(virNetworkPtr net, case VIR_NETWORK_FORWARD_NONE: case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: + case VIR_NETWORK_FORWARD_OPEN: switch (section) { case VIR_NETWORK_SECTION_FORWARD: case VIR_NETWORK_SECTION_FORWARD_INTERFACE: @@ -3325,7 +3326,6 @@ networkUpdate(virNetworkPtr net, } break; - case VIR_NETWORK_FORWARD_OPEN: case VIR_NETWORK_FORWARD_BRIDGE: case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 5981e3bd19..31feec9c9f 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -337,90 +337,101 @@ networkAddFirewallRules(virNetworkDef *def, virFirewallBackend firewallBackend, virFirewall **fwRemoval) { + if (def->forward.type == VIR_NETWORK_FORWARD_OPEN) { - networkSetupPrivateChains(firewallBackend, false); + VIR_DEBUG("No firewall rules to add for mode='open' network '%s'", def->name); - if (errInitV4 && - (virNetworkDefGetIPByIndex(def, AF_INET, 0) || - virNetworkDefGetRouteByIndex(def, AF_INET, 0))) { - virSetError(errInitV4); - return -1; - } + } else { - if (errInitV6 && - (virNetworkDefGetIPByIndex(def, AF_INET6, 0) || - virNetworkDefGetRouteByIndex(def, AF_INET6, 0) || - def->ipv6nogw)) { - virSetError(errInitV6); - return -1; - } + VIR_DEBUG("Adding firewall rules for mode='%s' network '%s' using %s", + virNetworkForwardTypeToString(def->forward.type), + def->name, + virFirewallBackendTypeToString(firewallBackend)); - if (def->bridgeZone) { + networkSetupPrivateChains(firewallBackend, false); - /* if a firewalld zone has been specified, fail/log an error - * if we can't honor it - */ - if (virFirewallDIsRegistered() < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("zone %1$s requested for network %2$s but firewalld is not active"), - def->bridgeZone, def->name); + if (errInitV4 && + (virNetworkDefGetIPByIndex(def, AF_INET, 0) || + virNetworkDefGetRouteByIndex(def, AF_INET, 0))) { + virSetError(errInitV4); return -1; } - if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0) + if (errInitV6 && + (virNetworkDefGetIPByIndex(def, AF_INET6, 0) || + virNetworkDefGetRouteByIndex(def, AF_INET6, 0) || + def->ipv6nogw)) { + virSetError(errInitV6); return -1; + } - } else { + if (def->bridgeZone) { - /* if firewalld is active, try to set the "libvirt" zone. This is - * desirable (for consistency) if firewalld is using the iptables - * backend, but is necessary (for basic network connectivity) if - * firewalld is using the nftables backend - */ - if (virFirewallDIsRegistered() == 0) { - - /* if the "libvirt" zone exists, then set it. If not, and - * if firewalld is using the nftables backend, then we - * need to log an error because the combination of - * nftables + default zone means that traffic cannot be - * forwarded (and even DHCP and DNS from guest to host - * will probably no be permitted by the default zone - * - * Routed networks use a different zone and policy which we also - * need to verify exist. Probing for the policy guarantees the - * running firewalld has support for policies (firewalld >= 0.9.0). + /* if a firewalld zone has been specified, fail/log an error + * if we can't honor it */ - if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE && - virFirewallDPolicyExists("libvirt-routed-out") && - virFirewallDZoneExists("libvirt-routed")) { - if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) - return -1; - } else if (virFirewallDZoneExists("libvirt")) { - if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) - return -1; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("firewalld can't find the 'libvirt' zone that should have been installed with libvirt")); + if (virFirewallDIsRegistered() < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("zone %1$s requested for network %2$s but firewalld is not active"), + def->bridgeZone, def->name); return -1; } + + if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0) + return -1; + + } else { + + /* if firewalld is active, try to set the "libvirt" zone. This is + * desirable (for consistency) if firewalld is using the iptables + * backend, but is necessary (for basic network connectivity) if + * firewalld is using the nftables backend + */ + if (virFirewallDIsRegistered() == 0) { + + /* if the "libvirt" zone exists, then set it. If not, and + * if firewalld is using the nftables backend, then we + * need to log an error because the combination of + * nftables + default zone means that traffic cannot be + * forwarded (and even DHCP and DNS from guest to host + * will probably no be permitted by the default zone + * + * Routed networks use a different zone and policy which we also + * need to verify exist. Probing for the policy guarantees the + * running firewalld has support for policies (firewalld >= 0.9.0). + */ + if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE && + virFirewallDPolicyExists("libvirt-routed-out") && + virFirewallDZoneExists("libvirt-routed")) { + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) + return -1; + } else if (virFirewallDZoneExists("libvirt")) { + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) + return -1; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("firewalld can't find the 'libvirt' zone that should have been installed with libvirt")); + return -1; + } + } } - } - switch (firewallBackend) { - case VIR_FIREWALL_BACKEND_NONE: - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("No firewall backend is available")); - return -1; + switch (firewallBackend) { + case VIR_FIREWALL_BACKEND_NONE: + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("No firewall backend is available")); + return -1; - case VIR_FIREWALL_BACKEND_IPTABLES: - return iptablesAddFirewallRules(def, fwRemoval); + case VIR_FIREWALL_BACKEND_IPTABLES: + return iptablesAddFirewallRules(def, fwRemoval); - case VIR_FIREWALL_BACKEND_NFTABLES: - return nftablesAddFirewallRules(def, fwRemoval); + case VIR_FIREWALL_BACKEND_NFTABLES: + return nftablesAddFirewallRules(def, fwRemoval); - case VIR_FIREWALL_BACKEND_LAST: - virReportEnumRangeError(virFirewallBackend, firewallBackend); - return -1; + case VIR_FIREWALL_BACKEND_LAST: + virReportEnumRangeError(virFirewallBackend, firewallBackend); + return -1; + } } return 0; } @@ -429,21 +440,29 @@ networkAddFirewallRules(virNetworkDef *def, void networkRemoveFirewallRules(virNetworkObj *obj) { + virNetworkDef *def = virNetworkObjGetDef(obj); virFirewall *fw; - if ((fw = virNetworkObjGetFwRemoval(obj)) == NULL) { - /* No information about firewall rules in the network status, - * so we assume the old iptables-based rules from 10.2.0 and - * earlier. + if (def->forward.type == VIR_NETWORK_FORWARD_OPEN) { + + VIR_DEBUG("No firewall rules to remove for mode='open' network '%s'", def->name); + + } else { + + if ((fw = virNetworkObjGetFwRemoval(obj)) == 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 status of network '%s', assuming old-style iptables", def->name); + iptablesRemoveFirewallRules(def); + return; + } + + /* fwRemoval info was stored in the network status, so use that to + * remove the firewall */ - VIR_DEBUG("No firewall info in network status, assuming old-style iptables"); - iptablesRemoveFirewallRules(virNetworkObjGetDef(obj)); - return; + VIR_DEBUG("Removing firewall rules of network '%s' using commands saved in status", def->name); + virFirewallApply(fw); } - - /* 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 status"); - virFirewallApply(fw); } -- 2.46.1
Now that networkAddFirewallRules and networkRemoveFirewallRules() are being called for mode='open' networks, we just need to move the code that sets the zone outside of the if (mode != ...OPEN) clause, so that it's done for all forward modes, with the exception of setting the implied 'libvirt*' zones, which are set when no zone is specified for all forward modes *except* 'open'. This was previously done in commit v10.7.0-76-g1a72b83d56, but in a manner that caused the zone to be unset whenever firewalld reloaded its rules. That patch was reverted, and this new better patch takes its place. Replaces: 1a72b83d566df952033529001b0f88a66d7f4393 Resolves: https://issues.redhat.com/browse/RHEL-61576 Re-Resolves: https://gitlab.com/libvirt/libvirt/-/issues/215 Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver_linux.c | 111 ++++++++++++++++-------------- 1 file changed, 60 insertions(+), 51 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 31feec9c9f..8956d38ab1 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -337,6 +337,64 @@ networkAddFirewallRules(virNetworkDef *def, virFirewallBackend firewallBackend, virFirewall **fwRemoval) { + /* If firewalld is running on the system, a firewalld zone is + * always set for the bridge device of all bridge-based managed + * networks of all forward modes *except* 'open', which is only + * set if specifically requested in the config. + */ + if (def->bridgeZone) { + + /* if a firewalld zone has been specified, fail/log an error + * if we can't honor it + */ + if (virFirewallDIsRegistered() < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("zone %1$s requested for network %2$s but firewalld is not active"), + def->bridgeZone, def->name); + return -1; + } + + if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0) + return -1; + + } else if (def->forward.type != VIR_NETWORK_FORWARD_OPEN) { + + /* if firewalld is active, try to set the "libvirt" zone by + * default (forward mode='open' networks have no zone set by + * default, but we honor it if one is specified). This is + * desirable (for consistency) if firewalld is using the + * iptables backend, but is necessary (for basic network + * connectivity) if firewalld is using the nftables backend + */ + if (virFirewallDIsRegistered() == 0) { + + /* if the "libvirt" zone exists, then set it. If not, and + * if firewalld is using the nftables backend, then we + * need to log an error because the combination of + * nftables + default zone means that traffic cannot be + * forwarded (and even DHCP and DNS from guest to host + * will probably no be permitted by the default zone + * + * Routed networks use a different zone and policy which we also + * need to verify exist. Probing for the policy guarantees the + * running firewalld has support for policies (firewalld >= 0.9.0). + */ + if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE && + virFirewallDPolicyExists("libvirt-routed-out") && + virFirewallDZoneExists("libvirt-routed")) { + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) + return -1; + } else if (virFirewallDZoneExists("libvirt")) { + if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) + return -1; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("firewalld can't find the 'libvirt' zone that should have been installed with libvirt")); + return -1; + } + } + } + if (def->forward.type == VIR_NETWORK_FORWARD_OPEN) { VIR_DEBUG("No firewall rules to add for mode='open' network '%s'", def->name); @@ -348,6 +406,7 @@ networkAddFirewallRules(virNetworkDef *def, def->name, virFirewallBackendTypeToString(firewallBackend)); + /* one-time (per system boot) initialization */ networkSetupPrivateChains(firewallBackend, false); if (errInitV4 && @@ -365,57 +424,7 @@ networkAddFirewallRules(virNetworkDef *def, return -1; } - if (def->bridgeZone) { - - /* if a firewalld zone has been specified, fail/log an error - * if we can't honor it - */ - if (virFirewallDIsRegistered() < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("zone %1$s requested for network %2$s but firewalld is not active"), - def->bridgeZone, def->name); - return -1; - } - - if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0) - return -1; - - } else { - - /* if firewalld is active, try to set the "libvirt" zone. This is - * desirable (for consistency) if firewalld is using the iptables - * backend, but is necessary (for basic network connectivity) if - * firewalld is using the nftables backend - */ - if (virFirewallDIsRegistered() == 0) { - - /* if the "libvirt" zone exists, then set it. If not, and - * if firewalld is using the nftables backend, then we - * need to log an error because the combination of - * nftables + default zone means that traffic cannot be - * forwarded (and even DHCP and DNS from guest to host - * will probably no be permitted by the default zone - * - * Routed networks use a different zone and policy which we also - * need to verify exist. Probing for the policy guarantees the - * running firewalld has support for policies (firewalld >= 0.9.0). - */ - if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE && - virFirewallDPolicyExists("libvirt-routed-out") && - virFirewallDZoneExists("libvirt-routed")) { - if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0) - return -1; - } else if (virFirewallDZoneExists("libvirt")) { - if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) - return -1; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("firewalld can't find the 'libvirt' zone that should have been installed with libvirt")); - return -1; - } - } - } - + /* now actually add the rules */ switch (firewallBackend) { case VIR_FIREWALL_BACKEND_NONE: virReportError(VIR_ERR_NO_SUPPORT, "%s", -- 2.46.1
(this is a remake of commit v10.7.0-78-g200f60b2e1, which was reverted due to a regression in another patch it was dependent on. The new implementation just adds the call to virFirewallDInterfaceUnsetZone() into the existing networkRemoveFirewallRules() (but only if we had set a zone when the network was first started). Replaces: 200f60b2e12e68d618f6d59f0173bb507b678838 Resolves: https://issues.redhat.com/browse/RHEL-61576 Signed-off-by: Laine Stump <laine@redhat.com> --- src/libvirt_private.syms | 1 + src/network/bridge_driver_linux.c | 29 +++++++++++++++++++++++------ src/util/virfirewalld.c | 23 +++++++++++++++++++++++ src/util/virfirewalld.h | 2 ++ 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cafb41166b..e09fb98596 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2452,6 +2452,7 @@ virFirewallDGetPolicies; virFirewallDGetVersion; virFirewallDGetZones; virFirewallDInterfaceSetZone; +virFirewallDInterfaceUnsetZone; virFirewallDIsRegistered; virFirewallDPolicyExists; virFirewallDSynchronize; diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 8956d38ab1..bafa9e26f9 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -459,19 +459,36 @@ networkRemoveFirewallRules(virNetworkObj *obj) } else { if ((fw = virNetworkObjGetFwRemoval(obj)) == 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 status of network '%s', assuming old-style iptables", def->name); iptablesRemoveFirewallRules(def); - return; + + } else { + + /* fwRemoval info was stored in the network status, so use that to + * remove the firewall + */ + VIR_DEBUG("Removing firewall rules of network '%s' using commands saved in status", def->name); + virFirewallApply(fw); } + } - /* fwRemoval info was stored in the network status, so use that to - * remove the firewall - */ - VIR_DEBUG("Removing firewall rules of network '%s' using commands saved in status", def->name); - virFirewallApply(fw); + /* all forward modes could have had a zone set, even 'open' mode + * iff it was specified in the config. firewalld preserves the + * name of an interface in a zone's list even after the interface + * has been deleted, which is problematic if the next use of that + * same interface name wants *no* zone set. To avoid this, we must + * "unset" the zone if we set it when the network was started. + */ + if (virFirewallDIsRegistered() == 0 + && !(def->forward.type == VIR_NETWORK_FORWARD_OPEN && def->bridgeZone == NULL)) { + + VIR_DEBUG("unsetting zone for '%s' (current zone is '%s')", + def->bridge, def->bridgeZone); + virFirewallDInterfaceUnsetZone(def->bridge); } } diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index 827e201dbb..ca61ed5ac0 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.1
On Mon, Oct 07, 2024 at 00:19:41 -0400, Laine Stump wrote:
(this is a remake of commit v10.7.0-78-g200f60b2e1, which was reverted due to a regression in another patch it was dependent on. The new implementation just adds the call to virFirewallDInterfaceUnsetZone() into the existing networkRemoveFirewallRules() (but only if we had set a zone when the network was first started).
Replaces: 200f60b2e12e68d618f6d59f0173bb507b678838 Resolves: https://issues.redhat.com/browse/RHEL-61576 Signed-off-by: Laine Stump <laine@redhat.com> --- src/libvirt_private.syms | 1 + src/network/bridge_driver_linux.c | 29 +++++++++++++++++++++++------ src/util/virfirewalld.c | 23 +++++++++++++++++++++++ src/util/virfirewalld.h | 2 ++ 4 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cafb41166b..e09fb98596 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2452,6 +2452,7 @@ virFirewallDGetPolicies; virFirewallDGetVersion; virFirewallDGetZones; virFirewallDInterfaceSetZone; +virFirewallDInterfaceUnsetZone; virFirewallDIsRegistered; virFirewallDPolicyExists; virFirewallDSynchronize; diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 8956d38ab1..bafa9e26f9 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -459,19 +459,36 @@ networkRemoveFirewallRules(virNetworkObj *obj) } else {
if ((fw = virNetworkObjGetFwRemoval(obj)) == 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 status of network '%s', assuming old-style iptables", def->name); iptablesRemoveFirewallRules(def); - return; + + } else { + + /* fwRemoval info was stored in the network status, so use that to + * remove the firewall + */ + VIR_DEBUG("Removing firewall rules of network '%s' using commands saved in status", def->name); + virFirewallApply(fw); } + }
- /* fwRemoval info was stored in the network status, so use that to - * remove the firewall - */ - VIR_DEBUG("Removing firewall rules of network '%s' using commands saved in status", def->name); - virFirewallApply(fw); + /* all forward modes could have had a zone set, even 'open' mode + * iff it was specified in the config. firewalld preserves the + * name of an interface in a zone's list even after the interface + * has been deleted, which is problematic if the next use of that + * same interface name wants *no* zone set. To avoid this, we must + * "unset" the zone if we set it when the network was started. + */ + if (virFirewallDIsRegistered() == 0 + && !(def->forward.type == VIR_NETWORK_FORWARD_OPEN && def->bridgeZone == NULL)) {
We put LF after &&. Also personally I would find if (virFirewallDIsRegistered() == 0 && (def->forward.type != VIR_NETWORK_FORWARD_OPEN || def->bridgeZone)) { easier to read without having to think about it too much :-)
+ + VIR_DEBUG("unsetting zone for '%s' (current zone is '%s')", + def->bridge, def->bridgeZone); + virFirewallDInterfaceUnsetZone(def->bridge); } }
Jirka
On Mon, Oct 07, 2024 at 00:19:36 -0400, Laine Stump wrote:
commit v10.7.0-76-g1a72b83d56 improperly assumed that reloading firewalld wouldn't reset the firewalld zone of libvirt-managed bridge devices. This resulted in loss of networking to guests when something on the host triggered a reload of firewalld rules, reported here:
https://issues.redhat.com/browse/RHEL-61576
This new series of patches, reverts that commit, along with commit v10.7.0-78-g200f60b2e1, then reimplements their functionality assuming that a firewalld reload *will* reset the zone of all libvirt-managed bridge devices.
Laine Stump (5): Revert "network: *un*set the firewalld zone while shutting down a network" Revert "network: support setting firewalld zone for bridge device of open networks" network: call network(Add|Remove)FirewallRules() for forward mode='open' network: a different way of supporting firewalld zone for mode='open' networks network: a different implementation of *un*setting firewalld zone when network is destroyed
src/network/bridge_driver.c | 34 +++---- src/network/bridge_driver_linux.c | 140 ++++++++++++++++----------- src/network/bridge_driver_nop.c | 19 ---- src/network/bridge_driver_platform.h | 4 - src/util/virfirewalld.c | 16 +-- 5 files changed, 102 insertions(+), 111 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
participants (2)
-
Jiri Denemark -
Laine Stump