Juan Quintela noticed that when he restarted libvirt he was getting
extra iptables rules added by libvirt even though he didn't have any
libvirt networks that used iptables rules. It turns out this also
happens if the firewalld service is restarted. The extra rules are
just the private chains, and they're sometimes being added
unnecessarily because they are added separately in a global
networkPreReloadFirewallRules() that does the init if there are any
active networks, regardless of whether or not any of those networks
will actually add rules to the host firewall.
The fix is to change the check for "any active networks" to instead
check for "any active networks that add firewall rules".
(NB: although the timing seems suspicious, this isn't a new regression
caused by the recently pushed f5418b427 (which forces recreation of
private chains when firewalld is restarted); it was an existing bug
since iptables rules were first put into private chains, even after
commit c6cbe18771 delayed creation of the private chains. The
implication is that any downstream based on v5.1.0 or later that cares
about these extraneous (but harmless) private chains would want to
backport this patch (along with the other two if they aren't already
there))
Signed-off-by: Laine Stump <laine@redhat.com>
---
src/network/bridge_driver_linux.c | 49 ++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 11 deletions(-)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index b0bd207250..4145411b4b 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -91,28 +91,55 @@ static void networkSetupPrivateChains(void)
static int
-networkHasRunningNetworksHelper(virNetworkObjPtr obj,
+networkHasRunningNetworksWithFWHelper(virNetworkObjPtr obj,
void *opaque)
{
- bool *running = opaque;
+ bool *activeWithFW = opaque;
virObjectLock(obj);
- if (virNetworkObjIsActive(obj))
- *running = true;
+ if (virNetworkObjIsActive(obj)) {
+ virNetworkDefPtr def = virNetworkObjGetDef(obj);
+
+ switch ((virNetworkForwardType) def->forward.type) {
+ case VIR_NETWORK_FORWARD_NONE:
+ case VIR_NETWORK_FORWARD_NAT:
+ case VIR_NETWORK_FORWARD_ROUTE:
+ *activeWithFW = true;
+ break;
+
+ case VIR_NETWORK_FORWARD_OPEN:
+ case VIR_NETWORK_FORWARD_BRIDGE:
+ case VIR_NETWORK_FORWARD_PRIVATE:
+ case VIR_NETWORK_FORWARD_VEPA:
+ case VIR_NETWORK_FORWARD_PASSTHROUGH:
+ case VIR_NETWORK_FORWARD_HOSTDEV:
+ case VIR_NETWORK_FORWARD_LAST:
+ break;
+ }
+ }
+
virObjectUnlock(obj);
+ /*
+ * terminate ForEach early once we find an active network that
+ * adds Firewall rules (return status is ignored)
+ */
+ if (*activeWithFW)
+ return -1;
+
return 0;
}
static bool
-networkHasRunningNetworks(virNetworkDriverStatePtr driver)
+networkHasRunningNetworksWithFW(virNetworkDriverStatePtr driver)
{
- bool running = false;
+ bool activeWithFW = false;
+
virNetworkObjListForEach(driver->networks,
- networkHasRunningNetworksHelper,
- &running);
- return running;
+ networkHasRunningNetworksWithFWHelper,
+ &activeWithFW);
+ return activeWithFW;
}
@@ -150,8 +177,8 @@ networkPreReloadFirewallRules(virNetworkDriverStatePtr driver,
networkSetupPrivateChains();
} else {
- if (!networkHasRunningNetworks(driver)) {
- VIR_DEBUG("Delayed global rule setup as no networks are running");
+ if (!networkHasRunningNetworksWithFW(driver)) {
+ VIR_DEBUG("Delayed global rule setup as no networks with firewall rules are running");
return;
}
--
2.25.4
On 6/5/20 2:56 PM, Laine Stump wrote: > Juan Quintela noticed that when he restarted libvirt he was getting > extra iptables rules added by libvirt even though he didn't have any > libvirt networks that used iptables rules. It turns out this also > happens if the firewalld service is restarted. The extra rules are > just the private chains, and they're sometimes being added > unnecessarily because they are added separately in a global > networkPreReloadFirewallRules() that does the init if there are any > active networks, regardless of whether or not any of those networks > will actually add rules to the host firewall. > > The fix is to change the check for "any active networks" to instead > check for "any active networks that add firewall rules". > > (NB: although the timing seems suspicious, this isn't a new regression > caused by the recently pushed f5418b427 (which forces recreation of > private chains when firewalld is restarted); it was an existing bug > since iptables rules were first put into private chains, even after > commit c6cbe18771 delayed creation of the private chains. The > implication is that any downstream based on v5.1.0 or later that cares > about these extraneous (but harmless) private chains would want to > backport this patch (along with the other two if they aren't already > there)) > > Signed-off-by: Laine Stump <laine@redhat.com> > --- > src/network/bridge_driver_linux.c | 49 ++++++++++++++++++++++++------- > 1 file changed, 38 insertions(+), 11 deletions(-) > > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c > index b0bd207250..4145411b4b 100644 > --- a/src/network/bridge_driver_linux.c > +++ b/src/network/bridge_driver_linux.c > @@ -91,28 +91,55 @@ static void networkSetupPrivateChains(void) > > > static int > -networkHasRunningNetworksHelper(virNetworkObjPtr obj, > +networkHasRunningNetworksWithFWHelper(virNetworkObjPtr obj, > void *opaque) > { > - bool *running = opaque; > + bool *activeWithFW = opaque; > > virObjectLock(obj); > - if (virNetworkObjIsActive(obj)) > - *running = true; > + if (virNetworkObjIsActive(obj)) { > + virNetworkDefPtr def = virNetworkObjGetDef(obj); > + > + switch ((virNetworkForwardType) def->forward.type) { > + case VIR_NETWORK_FORWARD_NONE: > + case VIR_NETWORK_FORWARD_NAT: > + case VIR_NETWORK_FORWARD_ROUTE: > + *activeWithFW = true; > + break; > + What's the rationale of "VIR_NETWORK_FORWARD_NONE" changing firewall rules? Is this a corner case that the NONE type covers? Functions such as networkAddIPSpecificFirewallRules() are operating just with the NAT and ROUTE forward types. (side note: there is no "firewall" string in formatdomain.html.in docs. I think it's a good idea to mention that certain <forward> types will change firewall settings of the host) Thanks, DHB > + case VIR_NETWORK_FORWARD_OPEN: > + case VIR_NETWORK_FORWARD_BRIDGE: > + case VIR_NETWORK_FORWARD_PRIVATE: > + case VIR_NETWORK_FORWARD_VEPA: > + case VIR_NETWORK_FORWARD_PASSTHROUGH: > + case VIR_NETWORK_FORWARD_HOSTDEV: > + case VIR_NETWORK_FORWARD_LAST: > + break; > + } > + } > + > virObjectUnlock(obj); > > + /* > + * terminate ForEach early once we find an active network that > + * adds Firewall rules (return status is ignored) > + */ > + if (*activeWithFW) > + return -1; > + > return 0; > } > > > static bool > -networkHasRunningNetworks(virNetworkDriverStatePtr driver) > +networkHasRunningNetworksWithFW(virNetworkDriverStatePtr driver) > { > - bool running = false; > + bool activeWithFW = false; > + > virNetworkObjListForEach(driver->networks, > - networkHasRunningNetworksHelper, > - &running); > - return running; > + networkHasRunningNetworksWithFWHelper, > + &activeWithFW); > + return activeWithFW; > } > > > @@ -150,8 +177,8 @@ networkPreReloadFirewallRules(virNetworkDriverStatePtr driver, > networkSetupPrivateChains(); > > } else { > - if (!networkHasRunningNetworks(driver)) { > - VIR_DEBUG("Delayed global rule setup as no networks are running"); > + if (!networkHasRunningNetworksWithFW(driver)) { > + VIR_DEBUG("Delayed global rule setup as no networks with firewall rules are running"); > return; > } > >
On 6/8/20 2:39 PM, Daniel Henrique Barboza wrote: > > > On 6/5/20 2:56 PM, Laine Stump wrote: >> Juan Quintela noticed that when he restarted libvirt he was getting >> extra iptables rules added by libvirt even though he didn't have any >> libvirt networks that used iptables rules. It turns out this also >> happens if the firewalld service is restarted. The extra rules are >> just the private chains, and they're sometimes being added >> unnecessarily because they are added separately in a global >> networkPreReloadFirewallRules() that does the init if there are any >> active networks, regardless of whether or not any of those networks >> will actually add rules to the host firewall. >> >> The fix is to change the check for "any active networks" to instead >> check for "any active networks that add firewall rules". >> >> (NB: although the timing seems suspicious, this isn't a new regression >> caused by the recently pushed f5418b427 (which forces recreation of >> private chains when firewalld is restarted); it was an existing bug >> since iptables rules were first put into private chains, even after >> commit c6cbe18771 delayed creation of the private chains. The >> implication is that any downstream based on v5.1.0 or later that cares >> about these extraneous (but harmless) private chains would want to >> backport this patch (along with the other two if they aren't already >> there)) >> >> Signed-off-by: Laine Stump <laine@redhat.com> >> --- >> src/network/bridge_driver_linux.c | 49 ++++++++++++++++++++++++------- >> 1 file changed, 38 insertions(+), 11 deletions(-) >> >> diff --git a/src/network/bridge_driver_linux.c >> b/src/network/bridge_driver_linux.c >> index b0bd207250..4145411b4b 100644 >> --- a/src/network/bridge_driver_linux.c >> +++ b/src/network/bridge_driver_linux.c >> @@ -91,28 +91,55 @@ static void networkSetupPrivateChains(void) >> static int >> -networkHasRunningNetworksHelper(virNetworkObjPtr obj, >> +networkHasRunningNetworksWithFWHelper(virNetworkObjPtr obj, >> void *opaque) >> { >> - bool *running = opaque; >> + bool *activeWithFW = opaque; >> virObjectLock(obj); >> - if (virNetworkObjIsActive(obj)) >> - *running = true; >> + if (virNetworkObjIsActive(obj)) { >> + virNetworkDefPtr def = virNetworkObjGetDef(obj); >> + >> + switch ((virNetworkForwardType) def->forward.type) { >> + case VIR_NETWORK_FORWARD_NONE: >> + case VIR_NETWORK_FORWARD_NAT: >> + case VIR_NETWORK_FORWARD_ROUTE: >> + *activeWithFW = true; >> + break; >> + > > > What's the rationale of "VIR_NETWORK_FORWARD_NONE" changing firewall > rules? Is > this a corner case that the NONE type covers? Functions such as > networkAddIPSpecificFirewallRules() are operating just with the NAT > and ROUTE > forward types. For historical reasons, a libvirt network that has no <forward> element is an "isolated" network, and libvirt adds rules to prevent any traffic from guests connected to that network from being forwarded anywhere else. These include 1) rules to allow incoming dhcp and dns requests (and possibly tftp) from guests on the network to the host, 2) allow traffic between guests on the isolated bridge (this rule would only be necessary in the case that the br_netfilter kernel module is loaded and there was some other lower priority rule that would otherwise block this traffic), and 3) reject forwarding of all packets to/from guests connected to this network and anywhere else outside the network (including a endpoints connected to a different network on the same host). Details are in https://libvirt.org/firewall.html > > (side note: there is no "firewall" string in formatdomain.html.in > docs. I think > it's a good idea to mention that certain <forward> types will change > firewall > settings of the host) Sure. With maybe a pointer from there to firewall.html, which explains this all in excruciating detail. Patches welcome :-) . When libvirt virtual networks were first created, the lore is that part of the idea was to avoid exposing the user to complicated things like iptables and dnsmasq configuration, and we were also a bit more relaxed about what was required in terms of documentation. Around 2011 or so danpb sent an email to the list that ended up being referenced so much that it was saved as firewall.html, but I guess nobody ever thought to put a link from formatdomain.html to that document (maybe because it's not a part of the official API, and so is subject to change; I don't think there was ever any conscious decision that it *shouldn't* be linked from there; it just wasn't. It *does* come up when you do a google search for "libvirt firewall" though...).
On 6/8/20 5:19 PM, Laine Stump wrote: > On 6/8/20 2:39 PM, Daniel Henrique Barboza wrote: >> >> >> On 6/5/20 2:56 PM, Laine Stump wrote: >>> Juan Quintela noticed that when he restarted libvirt he was getting >>> extra iptables rules added by libvirt even though he didn't have any >>> libvirt networks that used iptables rules. It turns out this also >>> happens if the firewalld service is restarted. The extra rules are >>> just the private chains, and they're sometimes being added >>> unnecessarily because they are added separately in a global >>> networkPreReloadFirewallRules() that does the init if there are any >>> active networks, regardless of whether or not any of those networks >>> will actually add rules to the host firewall. >>> >>> The fix is to change the check for "any active networks" to instead >>> check for "any active networks that add firewall rules". >>> >>> (NB: although the timing seems suspicious, this isn't a new regression >>> caused by the recently pushed f5418b427 (which forces recreation of >>> private chains when firewalld is restarted); it was an existing bug >>> since iptables rules were first put into private chains, even after >>> commit c6cbe18771 delayed creation of the private chains. The >>> implication is that any downstream based on v5.1.0 or later that cares >>> about these extraneous (but harmless) private chains would want to >>> backport this patch (along with the other two if they aren't already >>> there)) >>> >>> Signed-off-by: Laine Stump <laine@redhat.com> >>> --- >>> src/network/bridge_driver_linux.c | 49 ++++++++++++++++++++++++------- >>> 1 file changed, 38 insertions(+), 11 deletions(-) >>> >>> diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c >>> index b0bd207250..4145411b4b 100644 >>> --- a/src/network/bridge_driver_linux.c >>> +++ b/src/network/bridge_driver_linux.c >>> @@ -91,28 +91,55 @@ static void networkSetupPrivateChains(void) >>> static int >>> -networkHasRunningNetworksHelper(virNetworkObjPtr obj, >>> +networkHasRunningNetworksWithFWHelper(virNetworkObjPtr obj, >>> void *opaque) >>> { >>> - bool *running = opaque; >>> + bool *activeWithFW = opaque; >>> virObjectLock(obj); >>> - if (virNetworkObjIsActive(obj)) >>> - *running = true; >>> + if (virNetworkObjIsActive(obj)) { >>> + virNetworkDefPtr def = virNetworkObjGetDef(obj); >>> + >>> + switch ((virNetworkForwardType) def->forward.type) { >>> + case VIR_NETWORK_FORWARD_NONE: >>> + case VIR_NETWORK_FORWARD_NAT: >>> + case VIR_NETWORK_FORWARD_ROUTE: >>> + *activeWithFW = true; >>> + break; >>> + >> >> >> What's the rationale of "VIR_NETWORK_FORWARD_NONE" changing firewall rules? Is >> this a corner case that the NONE type covers? Functions such as >> networkAddIPSpecificFirewallRules() are operating just with the NAT and ROUTE >> forward types. > > > For historical reasons, a libvirt network that has no <forward> element is an "isolated" network, and libvirt adds rules to prevent any traffic from guests connected to that network from being forwarded anywhere else. These include 1) rules to allow incoming dhcp and dns requests (and possibly tftp) from guests on the network to the host, 2) allow traffic between guests on the isolated bridge (this rule would only be necessary in the case that the br_netfilter kernel module is loaded and there was some other lower priority rule that would otherwise block this traffic), and 3) reject forwarding of all packets to/from guests connected to this network and anywhere else outside the network (including a endpoints connected to a different network on the same host). Details are in > > > https://libvirt.org/firewall.html Thanks for the explanation! > > >> >> (side note: there is no "firewall" string in formatdomain.html.in docs. I think >> it's a good idea to mention that certain <forward> types will change firewall >> settings of the host) > > > Sure. With maybe a pointer from there to firewall.html, which explains this all in excruciating detail. Patches welcome :-) Patch sent :)
On 6/5/20 2:56 PM, Laine Stump wrote: > Juan Quintela noticed that when he restarted libvirt he was getting > extra iptables rules added by libvirt even though he didn't have any > libvirt networks that used iptables rules. It turns out this also > happens if the firewalld service is restarted. The extra rules are > just the private chains, and they're sometimes being added > unnecessarily because they are added separately in a global > networkPreReloadFirewallRules() that does the init if there are any > active networks, regardless of whether or not any of those networks > will actually add rules to the host firewall. > > The fix is to change the check for "any active networks" to instead > check for "any active networks that add firewall rules". > > (NB: although the timing seems suspicious, this isn't a new regression > caused by the recently pushed f5418b427 (which forces recreation of > private chains when firewalld is restarted); it was an existing bug > since iptables rules were first put into private chains, even after > commit c6cbe18771 delayed creation of the private chains. The > implication is that any downstream based on v5.1.0 or later that cares > about these extraneous (but harmless) private chains would want to > backport this patch (along with the other two if they aren't already > there)) > > Signed-off-by: Laine Stump <laine@redhat.com> > --- Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
© 2016 - 2024 Red Hat, Inc.