2:34 a.m.
Hi, all.
In the process of fixing a corner case in nwfilter in case of libvirtd
crash I noticed the peculiar way it works for recursive removes/renames
of chains. So I proposing a change which is described in the second patch.
This is an RFC and as such is incomplete:
- only recursive removing is patched and renaming is not
- tests are not fixed
Nikolay Shirokovskiy (2):
utils: make virFirewallAddRuleFullV public
nwfilter: don't query netfilter inside transactions
src/libvirt_private.syms | 1 +
src/nwfilter/nwfilter_ebiptables_driver.c | 154 +++++++++++-----------
src/util/virfirewall.c | 2 +-
src/util/virfirewall.h | 7 +
4 files changed, 84 insertions(+), 80 deletions(-)
--
2.31.1
2:34 a.m.
New subject: [RFC patch 1/2] utils: make virFirewallAddRuleFullV public
---
src/libvirt_private.syms | 1 +
src/util/virfirewall.c | 2 +-
src/util/virfirewall.h | 7 +++++++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6f0d72ca38..c2f43e95ab 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2316,6 +2316,7 @@ virFileCacheSetPriv;
# util/virfirewall.h
virFirewallAddRuleFull;
+virFirewallAddRuleFullV;
virFirewallApply;
virFirewallFree;
virFirewallNew;
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 31a8352d4e..5efa6ffd45 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -186,7 +186,7 @@ void virFirewallFree(virFirewall *firewall)
rule->args[rule->argsLen++] = g_strdup(str); \
} while (0)
-static virFirewallRule *
+virFirewallRule *
virFirewallAddRuleFullV(virFirewall *firewall,
virFirewallLayer layer,
bool ignoreErrors,
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index 7448825dbc..467cc3fb17 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -64,6 +64,13 @@ virFirewallRule *virFirewallAddRuleFull(virFirewall *firewall,
...)
G_GNUC_NULL_TERMINATED;
+virFirewallRule *virFirewallAddRuleFullV(virFirewall *firewall,
+ virFirewallLayer layer,
+ bool ignoreErrors,
+ virFirewallQueryCallback cb,
+ void *opaque,
+ va_list args);
+
void virFirewallRemoveRule(virFirewall *firewall,
virFirewallRule *rule);
--
2.31.1
2:34 a.m.
New subject: [RFC patch 2/2] nwfilter: don't query netfilter inside transactions
In order to delete or rename tree of chains in netfilter we use generic
code that inspect current state of netfilter and traverse the tree
accordingly. I found the way traverse is done a bit hard to think about.
As an example I'll take ebtable rules generated by
ebiptablesTearOldRules for input chains only. Common arguments like '-t
nat' and '--concurrent' are also filtered. To illustrate the point
libvirt-I-vnet1 has one subchain I-vnet1-mac.
Let's look at ebtablesRemoveSubChainsFW[1] work. It make a recursive
deletion of chains. I marked below the ebtables calls that it generates
in the whole list of commands that generates ebiptablesTearOldRules. One
can see that the rules perplex with the ebtablesRenameTmpSubAndRootChainsFW[2]
rules and this looks a bit hard to manage. I'd prefer that the order of
ebtables commands will be aligned with code.
ebtables -D PREROUTING -i vnet1 -j libvirt-I-vnet1
ebtables -L libvirt-I-vnet1 # listing to detect subchains[1]
ebtables -F libvirt-I-vnet1
ebtables -X libvirt-I-vnet1
ebtables -L libvirt-J-vnet1 # [2]
ebtables -E libvirt-J-vnet1 libvirt-I-vnet1 # [2]
ebtables -L I-vnet1-mac # listing to detect subchains[1]
ebtables -F I-vnet1-mac # flushing to unlink subchains[1]
ebtables -X I-vnet1-mac # deleting chain itself[1]
ebtables -L J-vnet1-mac # [2]
ebtables -F I-vnet1-mac # [2]
ebtables -X I-vnet1-mac # [2]
ebtables -E J-vnet1-mac I-vnet1-mac # [2]
Notice also that we need to flush libvirt-I-vnet1 to unlink subchains in
order to be able to delete them. And again code look like reversed:
ebtablesRemoveSubChainsFW(fw, ifname);
ebtablesRemoveRootChainFW(fw, true, ifname);
ebtablesRemoveRootChainFW(fw, false, ifname);
This is because ebtablesRemoveSubChainsFW only adds listing call and
makes actual subchains cleanup in the the process of virFirewallApply.
So to make it more manageble/straightforward I propose here to list and
add resulted commands inplace. Note that in the process we need:
1. Move ebtablesRemoveTmpRootChainFW rules to the _ebtablesRemoveSubChainsFW
to keep correct order - first flush parent and then delete subchains.
2. Avoid using virFirewallStartRollback as otherwise there will be
unnesseary listing call done in ebtablesRemoveTmpSubChainsFW.
The fact we can't use virFirewallStartRollback in this approach can be
seen as a donwside. However it is not a real rollback - you need to code
all the steps to rollback on you own so it can be easily replaced by
same virFirewallApply. Not only in this place but in all the other places.
Now compare the commands order in ebiptablesTearOldRules after patch:
ebtables -L libvirt-I-vnet1 # listing to detect subchains [1]
ebtables -L I-vnet1-mac # listing to detect subchains [1]
ebtables -D PREROUTING -i vnet1 -j libvirt-I-vnet1
ebtables -F libvirt-I-vnet1 # flushing and deleting of chain [1]
ebtables -X libvirt-I-vnet1
ebtables -F I-vnet1-mac # flushing and deleting of subchain [1]
ebtables -X I-vnet1-mac
ebtables -L libvirt-J-vnet1
ebtables -E libvirt-J-vnet1 libvirt-I-vnet1
ebtables -L J-vnet1-mac
ebtables -F I-vnet1-mac
ebtables -X I-vnet1-mac
ebtables -E J-vnet1-mac I-vnet1-mac
Notice that listing commands from ebtablesRemoveSubChainsFW pop up to
the top of the list. This may seen again as perplexing but it is of
a different kind. It only moves nonmodifying listing commands to the top
which does not perplexing understanding command flow much. Also if the
calls will be optimized we have only one listing command on the
top.
---
src/nwfilter/nwfilter_ebiptables_driver.c | 154 +++++++++++-----------
1 file changed, 75 insertions(+), 79 deletions(-)
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 54065a0f75..6952ebc059 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -148,6 +148,42 @@ static char chainprefixes_host_temp[3] = {
0
};
+
+static int
+ebiptablesQueryCallback(virFirewall *fw G_GNUC_UNUSED,
+ virFirewallLayer layer G_GNUC_UNUSED,
+ const char *const *lines,
+ void *opaque)
+{
+ GStrv *capture = opaque;
+
+ *capture = g_strdupv((gchar **)lines);
+ return 0;
+}
+
+
+static GStrv
+ebiptablesQuery(virFirewallLayer layer,
+ ...)
+{
+ g_autoptr(virFirewall) fw = virFirewallNew();
+ g_auto(GStrv) capture = NULL;
+ va_list args;
+
+ virFirewallStartTransaction(fw, 0);
+
+ va_start(args, layer);
+ virFirewallAddRuleFullV(fw, layer, true,
+ ebiptablesQueryCallback, &capture, args);
+ va_end(args);
+
+ if (virFirewallApply(fw) < 0)
+ return NULL;
+
+ return g_steal_pointer(&capture);
+}
+
+
static int
printVar(virNWFilterVarCombIter *vars,
char *buf, int bufsize,
@@ -2514,47 +2550,6 @@ ebtablesLinkTmpRootChainFW(virFirewall *fw,
}
-static void
-_ebtablesRemoveRootChainFW(virFirewall *fw,
- bool incoming, const char *ifname,
- int isTempChain)
-{
- char chain[MAX_CHAINNAME_LENGTH];
- char chainPrefix;
- if (isTempChain)
- chainPrefix = incoming ? CHAINPREFIX_HOST_IN_TEMP
- : CHAINPREFIX_HOST_OUT_TEMP;
- else
- chainPrefix = incoming ? CHAINPREFIX_HOST_IN
- : CHAINPREFIX_HOST_OUT;
-
- PRINT_ROOT_CHAIN(chain, chainPrefix, ifname);
-
- virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_ETHERNET,
- true, NULL, NULL,
- "-t", "nat", "-F", chain,
NULL);
- virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_ETHERNET,
- true, NULL, NULL,
- "-t", "nat", "-X", chain,
NULL);
-}
-
-
-static void
-ebtablesRemoveRootChainFW(virFirewall *fw,
- bool incoming, const char *ifname)
-{
- _ebtablesRemoveRootChainFW(fw, incoming, ifname, false);
-}
-
-
-static void
-ebtablesRemoveTmpRootChainFW(virFirewall *fw,
- bool incoming, const char *ifname)
-{
- _ebtablesRemoveRootChainFW(fw, incoming, ifname, 1);
-}
-
-
static void
_ebtablesUnlinkRootChainFW(virFirewall *fw,
bool incoming, const char *ifname,
@@ -2649,10 +2644,9 @@ static int
ebtablesRemoveSubChainsQuery(virFirewall *fw,
virFirewallLayer layer,
const char *const *lines,
- void *opaque)
+ const char *chainprefixes)
{
size_t i, j;
- const char *chainprefixes = opaque;
for (i = 0; lines[i] != NULL; i++) {
char *tmp = strstr(lines[i], "-j ");
@@ -2665,17 +2659,21 @@ ebtablesRemoveSubChainsQuery(virFirewall *fw,
for (j = 0; chainprefixes[j]; j++) {
if (tmp[0] == chainprefixes[j] &&
tmp[1] == '-') {
+ g_auto(GStrv) capture = NULL;
+
VIR_DEBUG("Processing chain '%s'", tmp);
- virFirewallAddRuleFull(fw, layer,
- false, ebtablesRemoveSubChainsQuery,
- (void *)chainprefixes,
- "-t", "nat", "-L",
tmp, NULL);
+ capture = ebiptablesQuery(layer,
+ "-t", "nat",
"-L", tmp, NULL);
virFirewallAddRuleFull(fw, layer,
true, NULL, NULL,
"-t", "nat", "-F",
tmp, NULL);
virFirewallAddRuleFull(fw, layer,
true, NULL, NULL,
"-t", "nat", "-X",
tmp, NULL);
+ if (capture)
+ ebtablesRemoveSubChainsQuery(fw, layer,
+ (const char *const *)capture,
+ chainprefixes);
}
}
}
@@ -2693,11 +2691,21 @@ _ebtablesRemoveSubChainsFW(virFirewall *fw,
size_t i;
for (i = 0; chainprefixes[i] != 0; i++) {
+ g_auto(GStrv) capture = NULL;
PRINT_ROOT_CHAIN(rootchain, chainprefixes[i], ifname);
+
+ capture = ebiptablesQuery(VIR_FIREWALL_LAYER_ETHERNET,
+ "-t", "nat", "-L",
rootchain, NULL);
virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_ETHERNET,
- false, ebtablesRemoveSubChainsQuery,
- (void *)chainprefixes,
- "-t", "nat", "-L",
rootchain, NULL);
+ true, NULL, NULL,
+ "-t", "nat", "-F",
rootchain, NULL);
+ virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_ETHERNET,
+ true, NULL, NULL,
+ "-t", "nat", "-X",
rootchain, NULL);
+ if (capture)
+ ebtablesRemoveSubChainsQuery(fw, VIR_FIREWALL_LAYER_ETHERNET,
+ (const char *const *)capture,
+ chainprefixes);
}
}
@@ -3079,14 +3087,10 @@ ebtablesCleanAll(const char *ifname)
ebtablesUnlinkRootChainFW(fw, true, ifname);
ebtablesUnlinkRootChainFW(fw, false, ifname);
ebtablesRemoveSubChainsFW(fw, ifname);
- ebtablesRemoveRootChainFW(fw, true, ifname);
- ebtablesRemoveRootChainFW(fw, false, ifname);
ebtablesUnlinkTmpRootChainFW(fw, true, ifname);
ebtablesUnlinkTmpRootChainFW(fw, false, ifname);
ebtablesRemoveTmpSubChainsFW(fw, ifname);
- ebtablesRemoveTmpRootChainFW(fw, true, ifname);
- ebtablesRemoveTmpRootChainFW(fw, false, ifname);
return virFirewallApply(fw);
}
@@ -3346,8 +3350,6 @@ ebiptablesApplyNewRules(const char *ifname,
ebtablesUnlinkTmpRootChainFW(fw, true, ifname);
ebtablesUnlinkTmpRootChainFW(fw, false, ifname);
ebtablesRemoveTmpSubChainsFW(fw, ifname);
- ebtablesRemoveTmpRootChainFW(fw, true, ifname);
- ebtablesRemoveTmpRootChainFW(fw, false, ifname);
virFirewallStartTransaction(fw, 0);
@@ -3499,25 +3501,27 @@ ebiptablesApplyNewRules(const char *ifname,
if (virHashSize(chains_out_set) != 0)
ebtablesLinkTmpRootChainFW(fw, false, ifname);
- virFirewallStartRollback(fw, 0);
- ebtablesUnlinkTmpRootChainFW(fw, true, ifname);
- ebtablesUnlinkTmpRootChainFW(fw, false, ifname);
- if (haveIp6tables) {
- iptablesUnlinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
- iptablesRemoveTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
- }
+ if (virFirewallApply(fw) < 0) {
+ g_autoptr(virFirewall) rollback = virFirewallNew();
- if (haveIptables) {
- iptablesUnlinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
- iptablesRemoveTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
- }
+ virFirewallStartTransaction(rollback, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
- ebtablesRemoveTmpSubChainsFW(fw, ifname);
- ebtablesRemoveTmpRootChainFW(fw, true, ifname);
- ebtablesRemoveTmpRootChainFW(fw, false, ifname);
+ if (haveIp6tables) {
+ iptablesUnlinkTmpRootChainsFW(rollback, VIR_FIREWALL_LAYER_IPV6, ifname);
+ iptablesRemoveTmpRootChainsFW(rollback, VIR_FIREWALL_LAYER_IPV6, ifname);
+ }
+
+ if (haveIptables) {
+ iptablesUnlinkTmpRootChainsFW(rollback, VIR_FIREWALL_LAYER_IPV4, ifname);
+ iptablesRemoveTmpRootChainsFW(rollback, VIR_FIREWALL_LAYER_IPV4, ifname);
+ }
+
+ ebtablesUnlinkTmpRootChainFW(rollback, true, ifname);
+ ebtablesUnlinkTmpRootChainFW(rollback, false, ifname);
+ ebtablesRemoveTmpSubChainsFW(rollback, ifname);
- if (virFirewallApply(fw) < 0)
goto cleanup;
+ }
ret = 0;
@@ -3541,8 +3545,6 @@ ebiptablesTearNewRulesFW(virFirewall *fw, const char *ifname)
ebtablesUnlinkTmpRootChainFW(fw, true, ifname);
ebtablesUnlinkTmpRootChainFW(fw, false, ifname);
ebtablesRemoveTmpSubChainsFW(fw, ifname);
- ebtablesRemoveTmpRootChainFW(fw, true, ifname);
- ebtablesRemoveTmpRootChainFW(fw, false, ifname);
}
@@ -3576,8 +3578,6 @@ ebiptablesTearOldRules(const char *ifname)
ebtablesUnlinkRootChainFW(fw, true, ifname);
ebtablesUnlinkRootChainFW(fw, false, ifname);
ebtablesRemoveSubChainsFW(fw, ifname);
- ebtablesRemoveRootChainFW(fw, true, ifname);
- ebtablesRemoveRootChainFW(fw, false, ifname);
ebtablesRenameTmpSubAndRootChainsFW(fw, ifname);
return virFirewallApply(fw);
@@ -3612,12 +3612,8 @@ ebiptablesAllTeardown(const char *ifname)
ebtablesUnlinkRootChainFW(fw, true, ifname);
ebtablesUnlinkRootChainFW(fw, false, ifname);
-
ebtablesRemoveSubChainsFW(fw, ifname);
- ebtablesRemoveRootChainFW(fw, true, ifname);
- ebtablesRemoveRootChainFW(fw, false, ifname);
-
return virFirewallApply(fw);
}
--
2.31.1
10:40 a.m.
New subject: [RFC patch 2/2] nwfilter: don't query netfilter inside transactions
On Wed, Mar 02, 2022 at 11:34:56AM +0300, Nikolay Shirokovskiy wrote:
In order to delete or rename tree of chains in netfilter we use
generic
code that inspect current state of netfilter and traverse the tree
accordingly. I found the way traverse is done a bit hard to think about.
As an example I'll take ebtable rules generated by
ebiptablesTearOldRules for input chains only. Common arguments like '-t
nat' and '--concurrent' are also filtered. To illustrate the point
libvirt-I-vnet1 has one subchain I-vnet1-mac.
Let's look at ebtablesRemoveSubChainsFW[1] work. It make a recursive
deletion of chains. I marked below the ebtables calls that it generates
in the whole list of commands that generates ebiptablesTearOldRules. One
can see that the rules perplex with the ebtablesRenameTmpSubAndRootChainsFW[2]
rules and this looks a bit hard to manage. I'd prefer that the order of
ebtables commands will be aligned with code.
ebtables -D PREROUTING -i vnet1 -j libvirt-I-vnet1
ebtables -L libvirt-I-vnet1 # listing to detect subchains[1]
ebtables -F libvirt-I-vnet1
ebtables -X libvirt-I-vnet1
ebtables -L libvirt-J-vnet1 # [2]
ebtables -E libvirt-J-vnet1 libvirt-I-vnet1 # [2]
ebtables -L I-vnet1-mac # listing to detect subchains[1]
ebtables -F I-vnet1-mac # flushing to unlink subchains[1]
ebtables -X I-vnet1-mac # deleting chain itself[1]
ebtables -L J-vnet1-mac # [2]
ebtables -F I-vnet1-mac # [2]
ebtables -X I-vnet1-mac # [2]
ebtables -E J-vnet1-mac I-vnet1-mac # [2]
Notice also that we need to flush libvirt-I-vnet1 to unlink subchains in
order to be able to delete them. And again code look like reversed:
ebtablesRemoveSubChainsFW(fw, ifname);
ebtablesRemoveRootChainFW(fw, true, ifname);
ebtablesRemoveRootChainFW(fw, false, ifname);
This is because ebtablesRemoveSubChainsFW only adds listing call and
makes actual subchains cleanup in the the process of virFirewallApply.
So to make it more manageble/straightforward I propose here to list and
add resulted commands inplace. Note that in the process we need:
1. Move ebtablesRemoveTmpRootChainFW rules to the _ebtablesRemoveSubChainsFW
to keep correct order - first flush parent and then delete subchains.
2. Avoid using virFirewallStartRollback as otherwise there will be
unnesseary listing call done in ebtablesRemoveTmpSubChainsFW.
The fact we can't use virFirewallStartRollback in this approach can be
seen as a donwside. However it is not a real rollback - you need to code
all the steps to rollback on you own so it can be easily replaced by
same virFirewallApply. Not only in this place but in all the other places.
Now compare the commands order in ebiptablesTearOldRules after patch:
ebtables -L libvirt-I-vnet1 # listing to detect subchains [1]
ebtables -L I-vnet1-mac # listing to detect subchains [1]
ebtables -D PREROUTING -i vnet1 -j libvirt-I-vnet1
ebtables -F libvirt-I-vnet1 # flushing and deleting of chain [1]
ebtables -X libvirt-I-vnet1
ebtables -F I-vnet1-mac # flushing and deleting of subchain [1]
ebtables -X I-vnet1-mac
ebtables -L libvirt-J-vnet1
ebtables -E libvirt-J-vnet1 libvirt-I-vnet1
ebtables -L J-vnet1-mac
ebtables -F I-vnet1-mac
ebtables -X I-vnet1-mac
ebtables -E J-vnet1-mac I-vnet1-mac
Notice that listing commands from ebtablesRemoveSubChainsFW pop up to
the top of the list. This may seen again as perplexing but it is of
a different kind. It only moves nonmodifying listing commands to the top
which does not perplexing understanding command flow much. Also if the
calls will be optimized we have only one listing command on the
top.
---
src/nwfilter/nwfilter_ebiptables_driver.c | 154 +++++++++++-----------
1 file changed, 75 insertions(+), 79 deletions(-)
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 54065a0f75..6952ebc059 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -148,6 +148,42 @@ static char chainprefixes_host_temp[3] = {
0
};
+
+static int
+ebiptablesQueryCallback(virFirewall *fw G_GNUC_UNUSED,
+ virFirewallLayer layer G_GNUC_UNUSED,
+ const char *const *lines,
+ void *opaque)
+{
+ GStrv *capture = opaque;
+
+ *capture = g_strdupv((gchar **)lines);
+ return 0;
+}
+
+
+static GStrv
+ebiptablesQuery(virFirewallLayer layer,
+ ...)
+{
+ g_autoptr(virFirewall) fw = virFirewallNew();
+ g_auto(GStrv) capture = NULL;
+ va_list args;
+
+ virFirewallStartTransaction(fw, 0);
+
+ va_start(args, layer);
+ virFirewallAddRuleFullV(fw, layer, true,
+ ebiptablesQueryCallback, &capture, args);
+ va_end(args);
+
+ if (virFirewallApply(fw) < 0)
+ return NULL;
+
+ return g_steal_pointer(&capture);
+}
This method illustrates why we didn't take this approach in the
first place. The intention of the current design is to have
clean separation between deciding what iptables commands to
invoke, and when we actually invoke them.
This breaks that separation, because now in the middle of
populating the virFirewall rules we want to run, we have
to actually invoke the firewall tools multiple times to
query current state.
@@ -2649,10 +2644,9 @@ static int
ebtablesRemoveSubChainsQuery(virFirewall *fw,
virFirewallLayer layer,
const char *const *lines,
- void *opaque)
+ const char *chainprefixes)
{
size_t i, j;
- const char *chainprefixes = opaque;
for (i = 0; lines[i] != NULL; i++) {
char *tmp = strstr(lines[i], "-j ");
@@ -2665,17 +2659,21 @@ ebtablesRemoveSubChainsQuery(virFirewall *fw,
for (j = 0; chainprefixes[j]; j++) {
if (tmp[0] == chainprefixes[j] &&
tmp[1] == '-') {
+ g_auto(GStrv) capture = NULL;
+
VIR_DEBUG("Processing chain '%s'", tmp);
- virFirewallAddRuleFull(fw, layer,
- false, ebtablesRemoveSubChainsQuery,
- (void *)chainprefixes,
- "-t", "nat", "-L",
tmp, NULL);
+ capture = ebiptablesQuery(layer,
+ "-t", "nat",
"-L", tmp, NULL);
virFirewallAddRuleFull(fw, layer,
true, NULL, NULL,
"-t", "nat", "-F",
tmp, NULL);
virFirewallAddRuleFull(fw, layer,
true, NULL, NULL,
"-t", "nat", "-X",
tmp, NULL);
+ if (capture)
+ ebtablesRemoveSubChainsQuery(fw, layer,
+ (const char *const *)capture,
+ chainprefixes);
}
....here we're still populating the 'fw' object, but actually
also running firewall commands at the same time inside the
ebtablesRemoveSubChainsQuery method.
The current implementation on the backend of virFirewallApply
is not technically transactional, but the intent behind the
design is that we would be able to serialize execution of the
virFirewallApply method. This wouldn't be possible with the
way this patch changes things, as now we're dealing with
multiple virFirewall object instances.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
2:32 a.m.
New subject: [RFC patch 2/2] nwfilter: don't query netfilter inside transactions
ср, 9 мар. 2022 г. в 19:40, Daniel P. Berrangé <berrange(a)redhat.com>:
On Wed, Mar 02, 2022 at 11:34:56AM +0300, Nikolay Shirokovskiy
wrote:
> In order to delete or rename tree of chains in netfilter we use generic
> code that inspect current state of netfilter and traverse the tree
> accordingly. I found the way traverse is done a bit hard to think about.
>
> As an example I'll take ebtable rules generated by
> ebiptablesTearOldRules for input chains only. Common arguments like '-t
> nat' and '--concurrent' are also filtered. To illustrate the point
> libvirt-I-vnet1 has one subchain I-vnet1-mac.
>
> Let's look at ebtablesRemoveSubChainsFW[1] work. It make a recursive
> deletion of chains. I marked below the ebtables calls that it generates
> in the whole list of commands that generates ebiptablesTearOldRules. One
> can see that the rules perplex with the
ebtablesRenameTmpSubAndRootChainsFW[2]
> rules and this looks a bit hard to manage. I'd prefer that the order of
> ebtables commands will be aligned with code.
>
> ebtables -D PREROUTING -i vnet1 -j libvirt-I-vnet1
> ebtables -L libvirt-I-vnet1 # listing to detect subchains[1]
> ebtables -F libvirt-I-vnet1
> ebtables -X libvirt-I-vnet1
> ebtables -L libvirt-J-vnet1 # [2]
> ebtables -E libvirt-J-vnet1 libvirt-I-vnet1 # [2]
> ebtables -L I-vnet1-mac # listing to detect subchains[1]
> ebtables -F I-vnet1-mac # flushing to unlink subchains[1]
> ebtables -X I-vnet1-mac # deleting chain itself[1]
> ebtables -L J-vnet1-mac # [2]
> ebtables -F I-vnet1-mac # [2]
> ebtables -X I-vnet1-mac # [2]
> ebtables -E J-vnet1-mac I-vnet1-mac # [2]
>
> Notice also that we need to flush libvirt-I-vnet1 to unlink subchains in
> order to be able to delete them. And again code look like reversed:
>
> ebtablesRemoveSubChainsFW(fw, ifname);
> ebtablesRemoveRootChainFW(fw, true, ifname);
> ebtablesRemoveRootChainFW(fw, false, ifname);
>
> This is because ebtablesRemoveSubChainsFW only adds listing call and
> makes actual subchains cleanup in the the process of virFirewallApply.
>
> So to make it more manageble/straightforward I propose here to list and
> add resulted commands inplace. Note that in the process we need:
>
> 1. Move ebtablesRemoveTmpRootChainFW rules to the
_ebtablesRemoveSubChainsFW
> to keep correct order - first flush parent and then delete subchains.
>
> 2. Avoid using virFirewallStartRollback as otherwise there will be
> unnesseary listing call done in ebtablesRemoveTmpSubChainsFW.
>
> The fact we can't use virFirewallStartRollback in this approach can be
> seen as a donwside. However it is not a real rollback - you need to code
> all the steps to rollback on you own so it can be easily replaced by
> same virFirewallApply. Not only in this place but in all the other
places.
>
> Now compare the commands order in ebiptablesTearOldRules after patch:
>
> ebtables -L libvirt-I-vnet1 # listing to detect subchains [1]
> ebtables -L I-vnet1-mac # listing to detect subchains [1]
> ebtables -D PREROUTING -i vnet1 -j libvirt-I-vnet1
> ebtables -F libvirt-I-vnet1 # flushing and deleting of chain [1]
> ebtables -X libvirt-I-vnet1
> ebtables -F I-vnet1-mac # flushing and deleting of subchain [1]
> ebtables -X I-vnet1-mac
> ebtables -L libvirt-J-vnet1
> ebtables -E libvirt-J-vnet1 libvirt-I-vnet1
> ebtables -L J-vnet1-mac
> ebtables -F I-vnet1-mac
> ebtables -X I-vnet1-mac
> ebtables -E J-vnet1-mac I-vnet1-mac
>
> Notice that listing commands from ebtablesRemoveSubChainsFW pop up to
> the top of the list. This may seen again as perplexing but it is of
> a different kind. It only moves nonmodifying listing commands to the top
> which does not perplexing understanding command flow much. Also if the
> calls will be optimized we have only one listing command on the
> top.
> ---
> src/nwfilter/nwfilter_ebiptables_driver.c | 154 +++++++++++-----------
> 1 file changed, 75 insertions(+), 79 deletions(-)
>
> diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c
b/src/nwfilter/nwfilter_ebiptables_driver.c
> index 54065a0f75..6952ebc059 100644
> --- a/src/nwfilter/nwfilter_ebiptables_driver.c
> +++ b/src/nwfilter/nwfilter_ebiptables_driver.c
> @@ -148,6 +148,42 @@ static char chainprefixes_host_temp[3] = {
> 0
> };
>
> +
> +static int
> +ebiptablesQueryCallback(virFirewall *fw G_GNUC_UNUSED,
> + virFirewallLayer layer G_GNUC_UNUSED,
> + const char *const *lines,
> + void *opaque)
> +{
> + GStrv *capture = opaque;
> +
> + *capture = g_strdupv((gchar **)lines);
> + return 0;
> +}
> +
> +
> +static GStrv
> +ebiptablesQuery(virFirewallLayer layer,
> + ...)
> +{
> + g_autoptr(virFirewall) fw = virFirewallNew();
> + g_auto(GStrv) capture = NULL;
> + va_list args;
> +
> + virFirewallStartTransaction(fw, 0);
> +
> + va_start(args, layer);
> + virFirewallAddRuleFullV(fw, layer, true,
> + ebiptablesQueryCallback, &capture, args);
> + va_end(args);
> +
> + if (virFirewallApply(fw) < 0)
> + return NULL;
> +
> + return g_steal_pointer(&capture);
> +}
This method illustrates why we didn't take this approach in the
first place. The intention of the current design is to have
clean separation between deciding what iptables commands to
invoke, and when we actually invoke them.
This breaks that separation, because now in the middle of
populating the virFirewall rules we want to run, we have
to actually invoke the firewall tools multiple times to
query current state.
> @@ -2649,10 +2644,9 @@ static int
> ebtablesRemoveSubChainsQuery(virFirewall *fw,
> virFirewallLayer layer,
> const char *const *lines,
> - void *opaque)
> + const char *chainprefixes)
> {
> size_t i, j;
> - const char *chainprefixes = opaque;
>
> for (i = 0; lines[i] != NULL; i++) {
> char *tmp = strstr(lines[i], "-j ");
> @@ -2665,17 +2659,21 @@ ebtablesRemoveSubChainsQuery(virFirewall *fw,
> for (j = 0; chainprefixes[j]; j++) {
> if (tmp[0] == chainprefixes[j] &&
> tmp[1] == '-') {
> + g_auto(GStrv) capture = NULL;
> +
> VIR_DEBUG("Processing chain '%s'", tmp);
> - virFirewallAddRuleFull(fw, layer,
> - false,
ebtablesRemoveSubChainsQuery,
> - (void *)chainprefixes,
> - "-t", "nat",
"-L", tmp, NULL);
> + capture = ebiptablesQuery(layer,
> + "-t", "nat",
"-L", tmp, NULL);
> virFirewallAddRuleFull(fw, layer,
> true, NULL, NULL,
> "-t", "nat",
"-F", tmp, NULL);
> virFirewallAddRuleFull(fw, layer,
> true, NULL, NULL,
> "-t", "nat",
"-X", tmp, NULL);
> + if (capture)
> + ebtablesRemoveSubChainsQuery(fw, layer,
> + (const char *const
*)capture,
> + chainprefixes);
> }
....here we're still populating the 'fw' object, but actually
also running firewall commands at the same time inside the
ebtablesRemoveSubChainsQuery method.
The current implementation on the backend of virFirewallApply
is not technically transactional, but the intent behind the
design is that we would be able to serialize execution of the
virFirewallApply method. This wouldn't be possible with the
way this patch changes things, as now we're dealing with
multiple virFirewall object instances.
Well I'd say if we'd have a transaction like with a nfttables we anyway
first
have to inspect tables then make a patch and then apply it. You have to
inspect tables
anyway outside the transaction. Which is basically what this patch does.
Anyway I think it is for good to address the issue with perplexing rules
from different
larger logical operations which is illustrated above (removing old
chains and renaming tmp chains
to non-tmp ones.
As an alternative I considered that rules that are created inside the
transaction could be added
to some marked place in the rules list instead of always at the end of the
list. But IMHO this
is more complicated. I'd prefer a simple approach which is to inspect
tables/create patch/apply patch.
Nikolay
982
days inactive
991
days old
4 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Nikolay Shirokovskiy