[libvirt] [PATCH 3/3] network: delay global firewall setup if no networks are running

Jim Fehlig jfehlig at suse.com
Wed May 22 21:59:00 UTC 2019


On 5/22/19 6:29 AM, Daniel P. Berrangé wrote:
> Creating firewall rules for the virtual networks causes the kernel to
> load the conntrack module. This imposes a significant performance
> penalty on Linux network traffic. Thus we want to only take that hit if
> we actually have virtual networks running.
> 
> We need to create global firewall rules during startup in order to
> "upgrade" rules for any running networks created by older libvirt.
> If no running networks are present though, we can safely delay setup
> until the time we actually start a network.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>   src/network/bridge_driver.c             |  2 +-
>   src/network/bridge_driver_linux.c       | 56 ++++++++++++++++++++++---
>   src/network/bridge_driver_nop.c         |  3 +-
>   src/network/bridge_driver_platform.h    |  2 +-
>   tests/networkxml2firewalldata/base.args | 34 +++++++++++++++

syntax-check is complaining about the line wrapping of this file.

>   tests/networkxml2firewalltest.c         | 36 +++++++++++++---
>   6 files changed, 119 insertions(+), 14 deletions(-)
>   create mode 100644 tests/networkxml2firewalldata/base.args
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 0e9bb78c32..c0c026e242 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2122,7 +2122,7 @@ networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
>        * but until we untangle the virt driver that's not viable */
>       if (!driver->privileged)
>           return;
> -    networkPreReloadFirewallRules(startup);
> +    networkPreReloadFirewallRules(driver, startup);
>       virNetworkObjListForEach(driver->networks,
>                                networkReloadFirewallRulesHelper,
>                                NULL);
> diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> index 2b29363f3c..fdd5d7066e 100644
> --- a/src/network/bridge_driver_linux.c
> +++ b/src/network/bridge_driver_linux.c
> @@ -84,16 +84,57 @@ static void networkSetupPrivateChains(void)
>       }
>   }
>   
> -void networkPreReloadFirewallRules(bool startup)
> +
> +static int
> +networkHasRunningNetworksHelper(virNetworkObjPtr obj,
> +                                void *opaque)
>   {
> -    /* We create global rules upfront as we don't want
> -     * the perf hit of conditionally figuring out whether
> -     * to create them each time a network is started.
> +    bool *running = opaque;
> +
> +    virObjectLock(obj);
> +    if (virNetworkObjIsActive(obj))
> +        *running = true;
> +    virObjectUnlock(obj);
> +
> +    return 0;
> +}
> +
> +
> +static bool
> +networkHasRunningNetworks(virNetworkDriverStatePtr driver)
> +{
> +    bool running = false;
> +    virNetworkObjListForEach(driver->networks,
> +                             networkHasRunningNetworksHelper,
> +                             &running);
> +    return running;
> +}
> +
> +
> +void networkPreReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
> +{
> +    /*
> +     * If there are any running networks, we need to
> +     * create the global rules upfront. This allows us
> +     * convert rules created by old libvirt into the new
> +     * format.
> +     *
> +     * If there are not any running networks, then we
> +     * must not create rules, because the rules will
> +     * cause the contrack kernel module to be loaded.

s/contrack/conntrack/

> +     * This imposes a significant performance hit on
> +     * the networking stack. Thus we will only create
> +     * rules if a network is later startup.
>        *
>        * Any errors here are saved to be reported at time
>        * of starting the network though as that makes them
>        * more likely to be seen by a human
>        */
> +    if (!networkHasRunningNetworks(driver)) {
> +        VIR_DEBUG("Delayed global rule setup as no networks are running");
> +        return;
> +    }
> +
>       ignore_value(virOnce(&createdOnce, networkSetupPrivateChains));
>   
>       /*
> @@ -109,8 +150,10 @@ void networkPreReloadFirewallRules(bool startup)
>        * rules will be present. Thus we can safely just tell it
>        * to always delete from the builin chain
>        */
> -    if (startup && createdChains)
> +    if (startup && createdChains) {
> +        VIR_DEBUG("Requesting cleanup of legacy firewall rules");

Perhaps intended for the previous patch?

I tested all the restart scenarios I could think of, including updating from 
"older" libvirt without private chains (with and without running networks). 
Looks good!

Reviewed-by: Jim Fehlig <jfehlig at suse.com>

Regards,
Jim

>           iptablesSetDeletePrivate(false);
> +    }
>   }
>   
>   
> @@ -724,6 +767,9 @@ int networkAddFirewallRules(virNetworkDefPtr def)
>       virFirewallPtr fw = NULL;
>       int ret = -1;
>   
> +    if (virOnce(&createdOnce, networkSetupPrivateChains) < 0)
> +        return -1;
> +
>       if (errInitV4 &&
>           (virNetworkDefGetIPByIndex(def, AF_INET, 0) ||
>            virNetworkDefGetRouteByIndex(def, AF_INET, 0))) {
> diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
> index ea9db338cb..ec7b1ed8b7 100644
> --- a/src/network/bridge_driver_nop.c
> +++ b/src/network/bridge_driver_nop.c
> @@ -19,7 +19,8 @@
>   
>   #include <config.h>
>   
> -void networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
> +void networkPreReloadFirewallRules(virNetworkDriverStatePtr driver ATTRIBUTE_UNUSED,
> +                                   bool startup ATTRIBUTE_UNUSED)
>   {
>   }
>   
> diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
> index 95fd64bdc7..5f534fc132 100644
> --- a/src/network/bridge_driver_platform.h
> +++ b/src/network/bridge_driver_platform.h
> @@ -58,7 +58,7 @@ struct _virNetworkDriverState {
>   typedef struct _virNetworkDriverState virNetworkDriverState;
>   typedef virNetworkDriverState *virNetworkDriverStatePtr;
>   
> -void networkPreReloadFirewallRules(bool startup);
> +void networkPreReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup);
>   void networkPostReloadFirewallRules(bool startup);
>   
>   int networkCheckRouteCollision(virNetworkDefPtr def);
> diff --git a/tests/networkxml2firewalldata/base.args b/tests/networkxml2firewalldata/base.args
> new file mode 100644
> index 0000000000..f76ce87b50
> --- /dev/null
> +++ b/tests/networkxml2firewalldata/base.args
> @@ -0,0 +1,34 @@
> +iptables --table filter --list-rules
> +iptables --table nat --list-rules
> +iptables --table mangle --list-rules
> +iptables --table filter --new-chain LIBVIRT_INP
> +iptables --table filter --insert INPUT --jump LIBVIRT_INP
> +iptables --table filter --new-chain LIBVIRT_OUT
> +iptables --table filter --insert OUTPUT --jump LIBVIRT_OUT
> +iptables --table filter --new-chain LIBVIRT_FWO
> +iptables --table filter --insert FORWARD --jump LIBVIRT_FWO
> +iptables --table filter --new-chain LIBVIRT_FWI
> +iptables --table filter --insert FORWARD --jump LIBVIRT_FWI
> +iptables --table filter --new-chain LIBVIRT_FWX
> +iptables --table filter --insert FORWARD --jump LIBVIRT_FWX
> +iptables --table nat --new-chain LIBVIRT_PRT
> +iptables --table nat --insert POSTROUTING --jump LIBVIRT_PRT
> +iptables --table mangle --new-chain LIBVIRT_PRT
> +iptables --table mangle --insert POSTROUTING --jump LIBVIRT_PRT
> +ip6tables --table filter --list-rules
> +ip6tables --table nat --list-rules
> +ip6tables --table mangle --list-rules
> +ip6tables --table filter --new-chain LIBVIRT_INP
> +ip6tables --table filter --insert INPUT --jump LIBVIRT_INP
> +ip6tables --table filter --new-chain LIBVIRT_OUT
> +ip6tables --table filter --insert OUTPUT --jump LIBVIRT_OUT
> +ip6tables --table filter --new-chain LIBVIRT_FWO
> +ip6tables --table filter --insert FORWARD --jump LIBVIRT_FWO
> +ip6tables --table filter --new-chain LIBVIRT_FWI
> +ip6tables --table filter --insert FORWARD --jump LIBVIRT_FWI
> +ip6tables --table filter --new-chain LIBVIRT_FWX
> +ip6tables --table filter --insert FORWARD --jump LIBVIRT_FWX
> +ip6tables --table nat --new-chain LIBVIRT_PRT
> +ip6tables --table nat --insert POSTROUTING --jump LIBVIRT_PRT
> +ip6tables --table mangle --new-chain LIBVIRT_PRT
> +ip6tables --table mangle --insert POSTROUTING --jump LIBVIRT_PRT
> diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
> index 575b68379a..c25282ebb1 100644
> --- a/tests/networkxml2firewalltest.c
> +++ b/tests/networkxml2firewalltest.c
> @@ -22,6 +22,7 @@
>   #include <config.h>
>   
>   #include "testutils.h"
> +#include "viralloc.h"
>   
>   #if defined (__linux__)
>   
> @@ -57,13 +58,15 @@ testCommandDryRun(const char *const*args ATTRIBUTE_UNUSED,
>   }
>   
>   static int testCompareXMLToArgvFiles(const char *xml,
> -                                     const char *cmdline)
> +                                     const char *cmdline,
> +                                     const char *baseargs)
>   {
>       char *expectargv = NULL;
>       char *actualargv = NULL;
>       virBuffer buf = VIR_BUFFER_INITIALIZER;
>       virNetworkDefPtr def = NULL;
>       int ret = -1;
> +    char *actual;
>   
>       virCommandSetDryRun(&buf, testCommandDryRun, NULL);
>   
> @@ -76,11 +79,18 @@ static int testCompareXMLToArgvFiles(const char *xml,
>       if (virBufferError(&buf))
>           goto cleanup;
>   
> -    actualargv = virBufferContentAndReset(&buf);
> +    actual = actualargv = virBufferContentAndReset(&buf);
>       virTestClearCommandPath(actualargv);
>       virCommandSetDryRun(NULL, NULL, NULL);
>   
> -    if (virTestCompareToFile(actualargv, cmdline) < 0)
> +    /* The first network to be created populates the
> +     * libvirt global chains. We must skip args for
> +     * that if present
> +     */
> +    if (STRPREFIX(actual, baseargs))
> +        actual += strlen(baseargs);
> +
> +    if (virTestCompareToFile(actual, cmdline) < 0)
>           goto cleanup;
>   
>       ret = 0;
> @@ -95,6 +105,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
>   
>   struct testInfo {
>       const char *name;
> +    const char *baseargs;
>   };
>   
>   
> @@ -112,7 +123,7 @@ testCompareXMLToIPTablesHelper(const void *data)
>                       abs_srcdir, info->name, RULESTYPE) < 0)
>           goto cleanup;
>   
> -    result = testCompareXMLToArgvFiles(xml, args);
> +    result = testCompareXMLToArgvFiles(xml, args, info->baseargs);
>   
>    cleanup:
>       VIR_FREE(xml);
> @@ -133,11 +144,13 @@ static int
>   mymain(void)
>   {
>       int ret = 0;
> +    VIR_AUTOFREE(char *)basefile = NULL;
> +    VIR_AUTOFREE(char *)baseargs = NULL;
>   
>   # define DO_TEST(name) \
>       do { \
> -        static struct testInfo info = { \
> -            name, \
> +        struct testInfo info = { \
> +            name, baseargs, \
>           }; \
>           if (virTestRun("Network XML-2-iptables " name, \
>                          testCompareXMLToIPTablesHelper, &info) < 0) \
> @@ -156,6 +169,17 @@ mymain(void)
>           goto cleanup;
>       }
>   
> +    if (virAsprintf(&basefile, "%s/networkxml2firewalldata/base.args",
> +                    abs_srcdir) < 0) {
> +        ret = -1;
> +        goto cleanup;
> +    }
> +
> +    if (virTestLoadFile(basefile, &baseargs) < 0) {
> +        ret = -1;
> +        goto cleanup;
> +    }
> +
>       DO_TEST("nat-default");
>       DO_TEST("nat-tftp");
>       DO_TEST("nat-many-ips");
> 




More information about the libvir-list mailing list