[libvirt] [PATCH] Re-add use of locking with iptables/ip6tables/ebtables
A previous commit introduced use of locking with invocation of iptables in the viriptables.c module commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862 Author: Serge Hallyn <serge.hallyn@ubuntu.com> Date: Fri Nov 1 12:36:59 2013 -0500 util: use -w flag when calling iptables This only ever had effect with the virtual network driver, as it was not wired up into the nwfilter driver. Unfortunately in the firewall refactoring the use of the -w flag was accidentally lost. This patch introduces it to the virfirewall.c module so that both the virtual network and nwfilter drivers will be using it. It also ensures that the equivalent --concurrent flag to ebtables is used. --- src/util/virfirewall.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++--- src/util/viriptables.c | 2 -- 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index bab1634..c83fdc6 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -104,6 +104,44 @@ virFirewallOnceInit(void) VIR_ONCE_GLOBAL_INIT(virFirewall) +static bool iptablesUseLock; +static bool ip6tablesUseLock; +static bool ebtablesUseLock; + +static void +virFirewallCheckUpdateLock(bool *lockflag, + const char *const*args) +{ + virCommandPtr cmd = virCommandNewArgs(args); + if (virCommandRun(cmd, NULL) < 0) { + VIR_INFO("locking not supported by %s", args[0]); + } else { + VIR_INFO("using locking for %s", args[0]); + *lockflag = true; + } + virCommandFree(cmd); +} + +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, + }; + virFirewallCheckUpdateLock(&iptablesUseLock, + iptablesArgs); + virFirewallCheckUpdateLock(&ip6tablesUseLock, + ip6tablesArgs); + virFirewallCheckUpdateLock(&ebtablesUseLock, + ebtablesArgs); +} + static int virFirewallValidateBackend(virFirewallBackend backend) { @@ -161,6 +199,9 @@ virFirewallValidateBackend(virFirewallBackend backend) } currentBackend = backend; + + virFirewallCheckUpdateLocking(); + return 0; } @@ -201,6 +242,9 @@ virFirewallPtr virFirewallNew(void) { virFirewallPtr firewall; + if (virFirewallInitialize() < 0) + return NULL; + if (VIR_ALLOC(firewall) < 0) return NULL; @@ -321,6 +365,23 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, rule->queryOpaque = opaque; rule->ignoreErrors = ignoreErrors; + switch (rule->layer) { + case VIR_FIREWALL_LAYER_ETHERNET: + if (ebtablesUseLock) + ADD_ARG(rule, "--concurrent"); + break; + case VIR_FIREWALL_LAYER_IPV4: + if (iptablesUseLock) + ADD_ARG(rule, "-w"); + break; + case VIR_FIREWALL_LAYER_IPV6: + if (ip6tablesUseLock) + ADD_ARG(rule, "-w"); + break; + case VIR_FIREWALL_LAYER_LAST: + break; + } + while ((str = va_arg(args, char *)) != NULL) { ADD_ARG(rule, str); } @@ -840,8 +901,8 @@ virFirewallApplyGroup(virFirewallPtr firewall, bool ignoreErrors = (group->actionFlags & VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); size_t i; - VIR_INFO("Starting transaction for %p flags=%x", - group, group->actionFlags); + VIR_INFO("Starting transaction for firewall=%p group=%p flags=%x", + firewall, group, group->actionFlags); firewall->currentGroup = idx; group->addingRollback = false; for (i = 0; i < group->naction; i++) { @@ -879,8 +940,6 @@ virFirewallApply(virFirewallPtr firewall) int ret = -1; virMutexLock(&ruleLock); - if (virFirewallInitialize() < 0) - goto cleanup; if (!firewall || firewall->err == ENOMEM) { virReportOOMError(); diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 4f3ac9c..46b4017 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -52,8 +52,6 @@ VIR_LOG_INIT("util.iptables"); -bool iptables_supports_xlock = false; - #define VIR_FROM_THIS VIR_FROM_NONE enum { -- 2.1.0
Quoting Daniel P. Berrange (berrange@redhat.com):
A previous commit introduced use of locking with invocation of iptables in the viriptables.c module
commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862 Author: Serge Hallyn <serge.hallyn@ubuntu.com> Date: Fri Nov 1 12:36:59 2013 -0500
util: use -w flag when calling iptables
This only ever had effect with the virtual network driver, as it was not wired up into the nwfilter driver. Unfortunately in the firewall refactoring the use of the -w flag was accidentally lost.
This patch introduces it to the virfirewall.c module so that both the virtual network and nwfilter drivers will be using it. It also ensures that the equivalent --concurrent flag to ebtables is used. ---
Thanks, that looks very nice. Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
src/util/virfirewall.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++--- src/util/viriptables.c | 2 -- 2 files changed, 63 insertions(+), 6 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index bab1634..c83fdc6 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -104,6 +104,44 @@ virFirewallOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virFirewall)
+static bool iptablesUseLock; +static bool ip6tablesUseLock; +static bool ebtablesUseLock; + +static void +virFirewallCheckUpdateLock(bool *lockflag, + const char *const*args) +{ + virCommandPtr cmd = virCommandNewArgs(args); + if (virCommandRun(cmd, NULL) < 0) { + VIR_INFO("locking not supported by %s", args[0]); + } else { + VIR_INFO("using locking for %s", args[0]); + *lockflag = true; + } + virCommandFree(cmd); +} + +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, + }; + virFirewallCheckUpdateLock(&iptablesUseLock, + iptablesArgs); + virFirewallCheckUpdateLock(&ip6tablesUseLock, + ip6tablesArgs); + virFirewallCheckUpdateLock(&ebtablesUseLock, + ebtablesArgs); +} + static int virFirewallValidateBackend(virFirewallBackend backend) { @@ -161,6 +199,9 @@ virFirewallValidateBackend(virFirewallBackend backend) }
currentBackend = backend; + + virFirewallCheckUpdateLocking(); + return 0; }
@@ -201,6 +242,9 @@ virFirewallPtr virFirewallNew(void) { virFirewallPtr firewall;
+ if (virFirewallInitialize() < 0) + return NULL; + if (VIR_ALLOC(firewall) < 0) return NULL;
@@ -321,6 +365,23 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, rule->queryOpaque = opaque; rule->ignoreErrors = ignoreErrors;
+ switch (rule->layer) { + case VIR_FIREWALL_LAYER_ETHERNET: + if (ebtablesUseLock) + ADD_ARG(rule, "--concurrent"); + break; + case VIR_FIREWALL_LAYER_IPV4: + if (iptablesUseLock) + ADD_ARG(rule, "-w"); + break; + case VIR_FIREWALL_LAYER_IPV6: + if (ip6tablesUseLock) + ADD_ARG(rule, "-w"); + break; + case VIR_FIREWALL_LAYER_LAST: + break; + } + while ((str = va_arg(args, char *)) != NULL) { ADD_ARG(rule, str); } @@ -840,8 +901,8 @@ virFirewallApplyGroup(virFirewallPtr firewall, bool ignoreErrors = (group->actionFlags & VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); size_t i;
- VIR_INFO("Starting transaction for %p flags=%x", - group, group->actionFlags); + VIR_INFO("Starting transaction for firewall=%p group=%p flags=%x", + firewall, group, group->actionFlags); firewall->currentGroup = idx; group->addingRollback = false; for (i = 0; i < group->naction; i++) { @@ -879,8 +940,6 @@ virFirewallApply(virFirewallPtr firewall) int ret = -1;
virMutexLock(&ruleLock); - if (virFirewallInitialize() < 0) - goto cleanup;
if (!firewall || firewall->err == ENOMEM) { virReportOOMError(); diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 4f3ac9c..46b4017 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -52,8 +52,6 @@
VIR_LOG_INIT("util.iptables");
-bool iptables_supports_xlock = false; - #define VIR_FROM_THIS VIR_FROM_NONE
enum { -- 2.1.0
On Tue, Nov 11, 2014 at 12:42:46PM +0000, Daniel P. Berrange wrote:
A previous commit introduced use of locking with invocation of iptables in the viriptables.c module
commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862 Author: Serge Hallyn <serge.hallyn@ubuntu.com> Date: Fri Nov 1 12:36:59 2013 -0500
util: use -w flag when calling iptables
This only ever had effect with the virtual network driver, as it was not wired up into the nwfilter driver. Unfortunately in the firewall refactoring the use of the -w flag was accidentally lost.
This patch introduces it to the virfirewall.c module so that both the virtual network and nwfilter drivers will be using it. It also ensures that the equivalent --concurrent flag to ebtables is used. --- src/util/virfirewall.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++--- src/util/viriptables.c | 2 -- 2 files changed, 63 insertions(+), 6 deletions(-)
ACK, Martin
On 11/11/2014 05:42 AM, Daniel P. Berrange wrote:
A previous commit introduced use of locking with invocation of iptables in the viriptables.c module
commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862 Author: Serge Hallyn <serge.hallyn@ubuntu.com> Date: Fri Nov 1 12:36:59 2013 -0500
util: use -w flag when calling iptables
This only ever had effect with the virtual network driver, as it was not wired up into the nwfilter driver. Unfortunately in the firewall refactoring the use of the -w flag was accidentally lost.
This patch introduces it to the virfirewall.c module so that both the virtual network and nwfilter drivers will be using it. It also ensures that the equivalent --concurrent flag to ebtables is used. --- src/util/virfirewall.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++--- src/util/viriptables.c | 2 -- 2 files changed, 63 insertions(+), 6 deletions(-)
With this patch applied, and testing on Fedora 20, I see the following messages at startup of libvirtd: 2014-11-18 13:22:49.979+0000: 31329: info : libvirt version: 1.2.11 2014-11-18 13:22:49.979+0000: 31329: error : virCommandWait:2532 : internal error: Child process (/usr/sbin/iptables -w -L -n) unexpected exit status 2: iptables v1.4.19.1: unknown option "-w" Try `iptables -h' or 'iptables --help' for more information. 2014-11-18 13:22:49.982+0000: 31329: error : virCommandWait:2532 : internal error: Child process (/usr/sbin/ip6tables -w -L -n) unexpected exit status 2: ip6tables v1.4.19.1: unknown option "-w" Try `ip6tables -h' or 'ip6tables --help' for more information. Do we need a followup patch to avoid logging failures when probing for whether -w works?
+ +static void +virFirewallCheckUpdateLock(bool *lockflag, + const char *const*args) +{ + virCommandPtr cmd = virCommandNewArgs(args); + if (virCommandRun(cmd, NULL) < 0) { + VIR_INFO("locking not supported by %s", args[0]);
Generally, it would be done by passing a non-NULL parameter to virCommandRun and checking the status ourselves (since virCommandRun with a NULL argument logs all non-zero exit). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Nov 18, 2014 at 06:26:54AM -0700, Eric Blake wrote:
On 11/11/2014 05:42 AM, Daniel P. Berrange wrote:
A previous commit introduced use of locking with invocation of iptables in the viriptables.c module
commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862 Author: Serge Hallyn <serge.hallyn@ubuntu.com> Date: Fri Nov 1 12:36:59 2013 -0500
util: use -w flag when calling iptables
This only ever had effect with the virtual network driver, as it was not wired up into the nwfilter driver. Unfortunately in the firewall refactoring the use of the -w flag was accidentally lost.
This patch introduces it to the virfirewall.c module so that both the virtual network and nwfilter drivers will be using it. It also ensures that the equivalent --concurrent flag to ebtables is used. --- src/util/virfirewall.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++--- src/util/viriptables.c | 2 -- 2 files changed, 63 insertions(+), 6 deletions(-)
With this patch applied, and testing on Fedora 20, I see the following messages at startup of libvirtd:
2014-11-18 13:22:49.979+0000: 31329: info : libvirt version: 1.2.11 2014-11-18 13:22:49.979+0000: 31329: error : virCommandWait:2532 : internal error: Child process (/usr/sbin/iptables -w -L -n) unexpected exit status 2: iptables v1.4.19.1: unknown option "-w" Try `iptables -h' or 'iptables --help' for more information.
2014-11-18 13:22:49.982+0000: 31329: error : virCommandWait:2532 : internal error: Child process (/usr/sbin/ip6tables -w -L -n) unexpected exit status 2: ip6tables v1.4.19.1: unknown option "-w" Try `ip6tables -h' or 'ip6tables --help' for more information.
Do we need a followup patch to avoid logging failures when probing for whether -w works?
+ +static void +virFirewallCheckUpdateLock(bool *lockflag, + const char *const*args) +{ + virCommandPtr cmd = virCommandNewArgs(args); + if (virCommandRun(cmd, NULL) < 0) { + VIR_INFO("locking not supported by %s", args[0]);
Generally, it would be done by passing a non-NULL parameter to virCommandRun and checking the status ourselves (since virCommandRun with a NULL argument logs all non-zero exit).
Ok, I'll look at doing that. I think that's actually what the original code did before it got accidentally lost. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/11/2014 01:42 PM, Daniel P. Berrange wrote:
A previous commit introduced use of locking with invocation of iptables in the viriptables.c module
commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862 Author: Serge Hallyn <serge.hallyn@ubuntu.com> Date: Fri Nov 1 12:36:59 2013 -0500
util: use -w flag when calling iptables
This only ever had effect with the virtual network driver, as it was not wired up into the nwfilter driver. Unfortunately in the firewall refactoring the use of the -w flag was accidentally lost.
This patch introduces it to the virfirewall.c module so that both the virtual network and nwfilter drivers will be using it. It also ensures that the equivalent --concurrent flag to ebtables is used. --- src/util/virfirewall.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++--- src/util/viriptables.c | 2 -- 2 files changed, 63 insertions(+), 6 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index bab1634..c83fdc6 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -104,6 +104,44 @@ virFirewallOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virFirewall)
+static bool iptablesUseLock; +static bool ip6tablesUseLock; +static bool ebtablesUseLock; + +static void +virFirewallCheckUpdateLock(bool *lockflag, + const char *const*args) +{ + virCommandPtr cmd = virCommandNewArgs(args); + if (virCommandRun(cmd, NULL) < 0) { + VIR_INFO("locking not supported by %s", args[0]); + } else { + VIR_INFO("using locking for %s", args[0]); + *lockflag = true; + } + virCommandFree(cmd); +} + +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, + }; + virFirewallCheckUpdateLock(&iptablesUseLock, + iptablesArgs); + virFirewallCheckUpdateLock(&ip6tablesUseLock, + ip6tablesArgs); + virFirewallCheckUpdateLock(&ebtablesUseLock, + ebtablesArgs); +} + static int virFirewallValidateBackend(virFirewallBackend backend) { @@ -161,6 +199,9 @@ virFirewallValidateBackend(virFirewallBackend backend) }
currentBackend = backend; + + virFirewallCheckUpdateLocking(); + return 0; }
@@ -201,6 +242,9 @@ virFirewallPtr virFirewallNew(void) { virFirewallPtr firewall;
+ if (virFirewallInitialize() < 0) + return NULL; + if (VIR_ALLOC(firewall) < 0) return NULL;
@@ -321,6 +365,23 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, rule->queryOpaque = opaque; rule->ignoreErrors = ignoreErrors;
+ switch (rule->layer) { + case VIR_FIREWALL_LAYER_ETHERNET: + if (ebtablesUseLock) + ADD_ARG(rule, "--concurrent"); + break; + case VIR_FIREWALL_LAYER_IPV4: + if (iptablesUseLock) + ADD_ARG(rule, "-w"); + break; + case VIR_FIREWALL_LAYER_IPV6: + if (ip6tablesUseLock) + ADD_ARG(rule, "-w"); + break; + case VIR_FIREWALL_LAYER_LAST: + break; + } + By adding these parameters dynamically based on the above added support checking logic will the network filter tests still work without any code change?
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 11/25/2014 03:20 PM, Boris Fiuczynski wrote:
On 11/11/2014 01:42 PM, Daniel P. Berrange wrote:
A previous commit introduced use of locking with invocation of iptables in the viriptables.c module
commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862 Author: Serge Hallyn <serge.hallyn@ubuntu.com> Date: Fri Nov 1 12:36:59 2013 -0500
util: use -w flag when calling iptables
This only ever had effect with the virtual network driver, as it was not wired up into the nwfilter driver. Unfortunately in the firewall refactoring the use of the -w flag was accidentally lost.
This patch introduces it to the virfirewall.c module so that both the virtual network and nwfilter drivers will be using it. It also ensures that the equivalent --concurrent flag to ebtables is used. --- src/util/virfirewall.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++--- src/util/viriptables.c | 2 -- 2 files changed, 63 insertions(+), 6 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index bab1634..c83fdc6 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -104,6 +104,44 @@ virFirewallOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virFirewall)
+static bool iptablesUseLock; +static bool ip6tablesUseLock; +static bool ebtablesUseLock; + +static void +virFirewallCheckUpdateLock(bool *lockflag, + const char *const*args) +{ + virCommandPtr cmd = virCommandNewArgs(args); + if (virCommandRun(cmd, NULL) < 0) { + VIR_INFO("locking not supported by %s", args[0]); + } else { + VIR_INFO("using locking for %s", args[0]); + *lockflag = true; + } + virCommandFree(cmd); +} + +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, + }; + virFirewallCheckUpdateLock(&iptablesUseLock, + iptablesArgs); + virFirewallCheckUpdateLock(&ip6tablesUseLock, + ip6tablesArgs); + virFirewallCheckUpdateLock(&ebtablesUseLock, + ebtablesArgs); +} + static int virFirewallValidateBackend(virFirewallBackend backend) { @@ -161,6 +199,9 @@ virFirewallValidateBackend(virFirewallBackend backend) }
currentBackend = backend; + + virFirewallCheckUpdateLocking(); + return 0; }
@@ -201,6 +242,9 @@ virFirewallPtr virFirewallNew(void) { virFirewallPtr firewall;
+ if (virFirewallInitialize() < 0) + return NULL; + if (VIR_ALLOC(firewall) < 0) return NULL;
@@ -321,6 +365,23 @@ virFirewallAddRuleFullV(virFirewallPtr firewall, rule->queryOpaque = opaque; rule->ignoreErrors = ignoreErrors;
+ switch (rule->layer) { + case VIR_FIREWALL_LAYER_ETHERNET: + if (ebtablesUseLock) + ADD_ARG(rule, "--concurrent"); + break; + case VIR_FIREWALL_LAYER_IPV4: + if (iptablesUseLock) + ADD_ARG(rule, "-w"); + break; + case VIR_FIREWALL_LAYER_IPV6: + if (ip6tablesUseLock) + ADD_ARG(rule, "-w"); + break; + case VIR_FIREWALL_LAYER_LAST: + break; + } + By adding these parameters dynamically based on the above added support checking logic will the network filter tests still work without any code change?
OK, just saw that a fix was posted today. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (5)
-
Boris Fiuczynski -
Daniel P. Berrange -
Eric Blake -
Martin Kletzander -
Serge Hallyn