[libvirt PATCH] network: add private chains only if there are networks adding iptables rules
Daniel Henrique Barboza
danielhb413 at gmail.com
Mon Jun 8 18:39:06 UTC 2020
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 at 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;
> }
>
>
More information about the libvir-list
mailing list