From d7703d11a44505d1a17001d8cfd36bf74d20b710 Mon Sep 17 00:00:00 2001 Message-Id: From: Laine Stump 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 Reviewed-by: Daniel Henrique Barboza (cherry picked from commit 0a867cd895f06134d24eb27070285bb4b50c088f) Message-Id: <20210116035151.1066734-4-laine@redhat.com> Reviewed-by: Jiri Denemark --- 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