From d7703d11a44505d1a17001d8cfd36bf74d20b710 Mon Sep 17 00:00:00 2001

Message-Id: <d7703d11a44505d1a17001d8cfd36bf74d20b710@dist-git>

From: Laine Stump <laine@redhat.com>

Date: Fri, 15 Jan 2021 22:51:46 -0500

Subject: [PATCH] util/tests: enable locking on iptables/ebtables commandlines

 by default

iptables and ip6tables have had a "-w" commandline option to grab a

systemwide lock that prevents two iptables invocations from modifying

the iptables chains since 2013 (upstream commit 93587a04 in

iptables-1.4.20).  Similarly, ebtables has had a "--concurrent"

commandline option for the same purpose since 2011 (in the upstream

ebtables commit f9b4bcb93, which was present in ebtables-2.0.10.4).

Libvirt added code to conditionally use the commandline option for

iptables/ip6tables in upstream commit ba95426d6f (libvirt-1.2.0,

November 2013), and for ebtables in upstream commit dc33e6e4a5

(libvirt-1.2.11, November 2014) (the latter actually *re*-added the

locking for iptables/ip6tables, as it had accidentally been removed

during a refactor of firewall code in the interim).

I say "conditionally" because a check was made during firewall module

initialization that tried executing a test command with the

-w/--concurrent option, and only continued using it for actual

commands if that test command completed successfully. At the time the

code was added this was a reasonable thing to do, as it had been less

than a year since introduction of -w to iptables, so many distros

supported by libvirt were still using iptables (and possibly even

ebtables) versions too old to have the new commandline options.

It is now 2020, and as far as I can discern from repology.org (and

manually examining a RHEL7.9 system), every version of every distro

that is supported by libvirt now uses new enough versions of both

iptables and ebtables that they all have support for -w/--concurrent.

That means we can finally remove the conditional code and simply

always use them.

https://bugzilla.redhat.com/1607929

Signed-off-by: Laine Stump <laine@redhat.com>

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

(cherry picked from commit 0a867cd895f06134d24eb27070285bb4b50c088f)

Message-Id: <20210116035151.1066734-4-laine@redhat.com>

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/libvirt_private.syms         |  1 -

 src/util/virfirewall.c           | 64 ++------------------------------

 src/util/virfirewall.h           |  2 -

 tests/networkxml2firewalltest.c  |  2 -

 tests/nwfilterebiptablestest.c   |  2 -

 tests/nwfilterxml2firewalltest.c |  2 -

 tests/virfirewalltest.c          |  2 -

 7 files changed, 3 insertions(+), 72 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms

index d6598c2514..edc53ce899 100644

--- a/src/libvirt_private.syms

+++ b/src/libvirt_private.syms

@@ -2089,7 +2089,6 @@ virFirewallRuleAddArgList;

 virFirewallRuleAddArgSet;

 virFirewallRuleGetArgCount;

 virFirewallSetBackend;

-virFirewallSetLockOverride;

 virFirewallStartRollback;

 virFirewallStartTransaction;

diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c

index c2de2bccae..2e3b02402e 100644

--- a/src/util/virfirewall.c

+++ b/src/util/virfirewall.c

@@ -97,59 +97,6 @@ virFirewallOnceInit(void)

 VIR_ONCE_GLOBAL_INIT(virFirewall);

-static bool iptablesUseLock;

-static bool ip6tablesUseLock;

-static bool ebtablesUseLock;

-static bool lockOverride; /* true to avoid lock probes */

-

-void

-virFirewallSetLockOverride(bool avoid)

-{

-    lockOverride = avoid;

-    if (avoid) {

-        /* add the lock option to all commands */

-        iptablesUseLock = true;

-        ip6tablesUseLock = true;

-        ebtablesUseLock = true;

-    }

-}

-

-static void

-virFirewallCheckUpdateLock(bool *lockflag,

-                           const char *const*args)

-{

-    int status; /* Ignore failed commands without logging them */

-    g_autoptr(virCommand) cmd = virCommandNewArgs(args);

-    if (virCommandRun(cmd, &status) < 0 || status) {

-        VIR_INFO("locking not supported by %s", args[0]);

-    } else {

-        VIR_INFO("using locking for %s", args[0]);

-        *lockflag = true;

-    }

-}

-

-static void

-virFirewallCheckUpdateLocking(void)

-{

-    const char *iptablesArgs[] = {

-        IPTABLES_PATH, "-w", "-L", "-n", NULL,

-    };

-    const char *ip6tablesArgs[] = {

-        IP6TABLES_PATH, "-w", "-L", "-n", NULL,

-    };

-    const char *ebtablesArgs[] = {

-        EBTABLES_PATH, "--concurrent", "-L", NULL,

-    };

-    if (lockOverride)

-        return;

-    virFirewallCheckUpdateLock(&iptablesUseLock,

-                               iptablesArgs);

-    virFirewallCheckUpdateLock(&ip6tablesUseLock,

-                               ip6tablesArgs);

-    virFirewallCheckUpdateLock(&ebtablesUseLock,

-                               ebtablesArgs);

-}

-

 static int

 virFirewallValidateBackend(virFirewallBackend backend)

 {

@@ -197,8 +144,6 @@ virFirewallValidateBackend(virFirewallBackend backend)

     currentBackend = backend;

-    virFirewallCheckUpdateLocking();

-

     return 0;

 }

@@ -363,16 +308,13 @@ virFirewallAddRuleFullV(virFirewallPtr firewall,

     switch (rule->layer) {

     case VIR_FIREWALL_LAYER_ETHERNET:

-        if (ebtablesUseLock)

-            ADD_ARG(rule, "--concurrent");

+        ADD_ARG(rule, "--concurrent");

         break;

     case VIR_FIREWALL_LAYER_IPV4:

-        if (iptablesUseLock)

-            ADD_ARG(rule, "-w");

+        ADD_ARG(rule, "-w");

         break;

     case VIR_FIREWALL_LAYER_IPV6:

-        if (ip6tablesUseLock)

-            ADD_ARG(rule, "-w");

+        ADD_ARG(rule, "-w");

         break;

     case VIR_FIREWALL_LAYER_LAST:

         break;

diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h

index 6148f46827..fda3cdec01 100644

--- a/src/util/virfirewall.h

+++ b/src/util/virfirewall.h

@@ -111,6 +111,4 @@ void virFirewallStartRollback(virFirewallPtr firewall,

 int virFirewallApply(virFirewallPtr firewall);

-void virFirewallSetLockOverride(bool avoid);

-

 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFirewall, virFirewallFree);

diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c

index 0ad5e2303b..886b268319 100644

--- a/tests/networkxml2firewalltest.c

+++ b/tests/networkxml2firewalltest.c

@@ -152,8 +152,6 @@ mymain(void)

             ret = -1; \

     } while (0)

-    virFirewallSetLockOverride(true);

-

     if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) {

         if (!hasNetfilterTools()) {

             fprintf(stderr, "iptables/ip6tables/ebtables tools not present");

diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c

index e70f0e2400..adce7430a9 100644

--- a/tests/nwfilterebiptablestest.c

+++ b/tests/nwfilterebiptablestest.c

@@ -510,8 +510,6 @@ mymain(void)

 {

     int ret = 0;

-    virFirewallSetLockOverride(true);

-

     if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) {

         if (!hasNetfilterTools()) {

             fprintf(stderr, "iptables/ip6tables/ebtables tools not present");

diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c

index c97f83b24a..73f7991a96 100644

--- a/tests/nwfilterxml2firewalltest.c

+++ b/tests/nwfilterxml2firewalltest.c

@@ -459,8 +459,6 @@ mymain(void)

             ret = -1; \

     } while (0)

-    virFirewallSetLockOverride(true);

-

     if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) {

         if (!hasNetfilterTools()) {

             fprintf(stderr, "iptables/ip6tables/ebtables tools not present");

diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c

index 195163a985..1ec768d302 100644

--- a/tests/virfirewalltest.c

+++ b/tests/virfirewalltest.c

@@ -1141,8 +1141,6 @@ mymain(void)

     RUN_TEST_DIRECT(name, method)

 # endif /* ! WITH_DBUS */

-    virFirewallSetLockOverride(true);

-

     RUN_TEST("single group", testFirewallSingleGroup);

     RUN_TEST("remove rule", testFirewallRemoveRule);

     RUN_TEST("many groups", testFirewallManyGroups);

-- 

2.30.0