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