1:08 p.m.
Thanks for the feedback. I've applied the iptables -L with a callback
handler. The callback handler then decides whether or not to create the
base chains.
I changed the commit message from
"nwfilter: Avoid firewall hole during VM startup by checking rule presence" to
"nwfilter: Check before removing and reinserting iptable base chains".
I also edited nwfilterxml2firewalltest, so it pretends that currently there are
no chains and I changed the expected order of the commonRules.
Dion Bosschieter (1):
nwfilter: Check before removing and reinserting iptable base chains
src/nwfilter/nwfilter_ebiptables_driver.c | 203 +++++++++++++---------
tests/nwfilterxml2firewalltest.c | 58 +++++--
2 files changed, 163 insertions(+), 98 deletions(-)
--
2.43.0
1:08 p.m.
New subject: [PATCH v2 1/1] nwfilter: Check before removing and reinserting iptable base chains
Upon VM bootstrapping (start,restore,incoming migration)
iptablesCreateBaseChainsFW is called and unconditionally deletes and
reinserts top-level firewall chain jumps (e.g. INPUT, FORWARD rules).
This briefly allows packets to continue, allowing packets through
until the base chain iptables -I commands run.
This commit ensures that the base chains are only created once per layer
(IPV4/IPV6) and checks whether the expected rules already exist using
`iptables -L`. If they do, no delete/insert operations are performed.
By checking for the existence of rules we can prevent more rules from
being created if they already exist. Possibly speeding up nwfilter by
reducing the amount of iptable commands it executes. This however is not
part of this patch.
Solves: https://gitlab.com/libvirt/libvirt/-/issues/784
Signed-off-by: Dion Bosschieter <dionbosschieter(a)gmail.com>
---
src/nwfilter/nwfilter_ebiptables_driver.c | 203 +++++++++++++---------
tests/nwfilterxml2firewalltest.c | 58 +++++--
2 files changed, 163 insertions(+), 98 deletions(-)
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 067df6e612..18cb870fba 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -131,6 +131,25 @@ static char chainprefixes_host_temp[3] = {
0
};
+typedef struct {
+ const char *chain;
+ const char *position;
+ const char *targetChain;
+} iptablesBaseChainFW;
+
+typedef struct {
+ const char *ifname;
+ int nrules;
+ virNWFilterRuleInst **rules;
+} chainCreateCallbackData;
+
+static iptablesBaseChainFW fw_base_chains[] = {
+ {"FORWARD", "1", VIRT_IN_CHAIN},
+ {"FORWARD", "2", VIRT_OUT_CHAIN},
+ {"FORWARD", "3", VIRT_IN_POST_CHAIN},
+ {"INPUT", "1", HOST_IN_CHAIN},
+};
+
static int
printVar(virNWFilterVarCombIter *vars,
char *buf, int bufsize,
@@ -398,46 +417,6 @@ ebtablesHandleEthHdr(virFirewall *fw,
/************************ iptables support ************************/
-
-static void
-iptablesCreateBaseChainsFW(virFirewall *fw,
- virFirewallLayer layer)
-{
- virFirewallAddCmdFull(fw, layer,
- true, NULL, NULL,
- "-N", VIRT_IN_CHAIN, NULL);
- virFirewallAddCmdFull(fw, layer,
- true, NULL, NULL,
- "-N", VIRT_OUT_CHAIN, NULL);
- virFirewallAddCmdFull(fw, layer,
- true, NULL, NULL,
- "-N", VIRT_IN_POST_CHAIN, NULL);
- virFirewallAddCmdFull(fw, layer,
- true, NULL, NULL,
- "-N", HOST_IN_CHAIN, NULL);
- virFirewallAddCmdFull(fw, layer,
- true, NULL, NULL,
- "-D", "FORWARD", "-j",
VIRT_IN_CHAIN, NULL);
- virFirewallAddCmdFull(fw, layer,
- true, NULL, NULL,
- "-D", "FORWARD", "-j",
VIRT_OUT_CHAIN, NULL);
- virFirewallAddCmdFull(fw, layer,
- true, NULL, NULL,
- "-D", "FORWARD", "-j",
VIRT_IN_POST_CHAIN, NULL);
- virFirewallAddCmdFull(fw, layer,
- true, NULL, NULL,
- "-D", "INPUT", "-j",
HOST_IN_CHAIN, NULL);
- virFirewallAddCmd(fw, layer,
- "-I", "FORWARD", "1", "-j",
VIRT_IN_CHAIN, NULL);
- virFirewallAddCmd(fw, layer,
- "-I", "FORWARD", "2", "-j",
VIRT_OUT_CHAIN, NULL);
- virFirewallAddCmd(fw, layer,
- "-I", "FORWARD", "3", "-j",
VIRT_IN_POST_CHAIN, NULL);
- virFirewallAddCmd(fw, layer,
- "-I", "INPUT", "1", "-j",
HOST_IN_CHAIN, NULL);
-}
-
-
static void
iptablesCreateTmpRootChainFW(virFirewall *fw,
virFirewallLayer layer,
@@ -3148,6 +3127,7 @@ iptablesCheckBridgeNFCallEnabled(bool isIPv6)
}
}
+
/*
* Given a filtername determine the protocol it is used for evaluating
* We do prefix-matching to determine the protocol.
@@ -3202,6 +3182,104 @@ iptablesRuleInstCommand(virFirewall *fw,
}
+static int
+iptablesHandleCreateChainAndRules(virFirewall *fw,
+ virFirewallLayer layer,
+ const char *const *lines,
+ void *opaque)
+{
+ size_t i, j;
+ static bool baseChainDefined[G_N_ELEMENTS(fw_base_chains)] = { false };
+ chainCreateCallbackData *cbdata = opaque;
+ bool isIPv6 = layer == VIR_FIREWALL_LAYER_IPV6;
+
+ iptablesUnlinkTmpRootChainsFW(fw, layer, cbdata->ifname);
+ iptablesRemoveTmpRootChainsFW(fw, layer, cbdata->ifname);
+
+ // parse iptables -L output to see if base chains exist
+ for (i = 0; lines[i] != NULL; i++) {
+ if (!STRPREFIX(lines[i], "Chain ")) {
+ // line doesn't start with Chain
+ continue;
+ };
+
+ VIR_DEBUG("Considering chain for comparison '%s'", lines[i]);
+
+ for (j = 0; j < G_N_ELEMENTS(fw_base_chains); j++) {
+ // if chain matches basechain
+ if (STRPREFIX(lines[i]+6, fw_base_chains[j].targetChain)) {
+ VIR_DEBUG("Found chain '%s'",
fw_base_chains[j].targetChain);
+ baseChainDefined[j] = true;
+ }
+ }
+ }
+
+ // go through parsing results and add basechain commands if necessary
+ for (i = 0; i < G_N_ELEMENTS(fw_base_chains); i++) {
+ if (!baseChainDefined[i]) {
+ VIR_DEBUG("Defining base chain '%s'",
fw_base_chains[i].targetChain);
+
+ virFirewallAddCmdFull(fw, layer,
+ true, NULL, NULL,
+ "-N", fw_base_chains[i].targetChain, NULL);
+ virFirewallAddCmdFull(fw, layer,
+ true, NULL, NULL,
+ "-D", fw_base_chains[i].chain,
"-j",
+ fw_base_chains[i].targetChain, NULL);
+ virFirewallAddCmd(fw, layer,
+ "-I", fw_base_chains[i].chain,
fw_base_chains[i].position,
+ "-j", fw_base_chains[i].targetChain, NULL);
+ }
+ }
+
+ iptablesCreateTmpRootChainsFW(fw, layer, cbdata->ifname);
+ iptablesLinkTmpRootChainsFW(fw, layer, cbdata->ifname);
+ iptablesSetupVirtInPostFW(fw, layer, cbdata->ifname);
+
+ for (i = 0; i < cbdata->nrules; i++) {
+ if (isIPv6 == false &&
+ virNWFilterRuleIsProtocolIPv4(cbdata->rules[i]->def)) {
+ if (iptablesRuleInstCommand(fw, cbdata->ifname, cbdata->rules[i]) <
0)
+ goto cleanup;
+ } else if (isIPv6 &&
virNWFilterRuleIsProtocolIPv6(cbdata->rules[i]->def)) {
+ if (iptablesRuleInstCommand(fw, cbdata->ifname, cbdata->rules[i]) <
0)
+ goto cleanup;
+ }
+ }
+
+ iptablesCheckBridgeNFCallEnabled(isIPv6);
+
+ cleanup:
+
+ return 0;
+}
+
+
+/**
+ * iptablesCreateChainsAndRules
+ *
+ * @fw: the firewall ruleset instance
+ * @layer: the firewall layer
+ * @cbdata: callback data struct which holds variables that
+ * the call back handler needs in order to create
+ * the base chain jumps and the dependant rules
+ * the ICMP rule from being created
+ *
+ * Run iptables -L and parse if chains already exist
+ * skips creation of base chain jumps if possible
+ * see handler in iptablesHandleCreateChainAndRules
+ */
+static void iptablesCreateChainsAndRules(virFirewall *fw,
+ virFirewallLayer layer,
+ chainCreateCallbackData *cbdata)
+{
+ virFirewallAddCmdFull(fw, layer,
+ false, iptablesHandleCreateChainAndRules,
+ (void *)cbdata,
+ "-L", NULL, NULL);
+}
+
+
static int
ebtablesRuleInstCommand(virFirewall *fw,
const char *ifname,
@@ -3311,6 +3389,7 @@ ebiptablesApplyNewRules(const char *ifname,
g_autofree ebtablesSubChainInst **subchains = NULL;
size_t nsubchains = 0;
int ret = -1;
+ chainCreateCallbackData chainCallbackData = {ifname, nrules, rules};
if (nrules) {
g_qsort_with_data(rules, nrules, sizeof(rules[0]),
@@ -3429,47 +3508,9 @@ ebiptablesApplyNewRules(const char *ifname,
}
if (haveIptables) {
- iptablesUnlinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
- iptablesRemoveTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
-
- iptablesCreateBaseChainsFW(fw, VIR_FIREWALL_LAYER_IPV4);
- iptablesCreateTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
-
- iptablesLinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
- iptablesSetupVirtInPostFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
-
- for (i = 0; i < nrules; i++) {
- if (virNWFilterRuleIsProtocolIPv4(rules[i]->def)) {
- if (iptablesRuleInstCommand(fw,
- ifname,
- rules[i]) < 0)
- goto cleanup;
- }
- }
-
- iptablesCheckBridgeNFCallEnabled(false);
- }
-
- if (haveIp6tables) {
- iptablesUnlinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
- iptablesRemoveTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
-
- iptablesCreateBaseChainsFW(fw, VIR_FIREWALL_LAYER_IPV6);
- iptablesCreateTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
-
- iptablesLinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
- iptablesSetupVirtInPostFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
-
- for (i = 0; i < nrules; i++) {
- if (virNWFilterRuleIsProtocolIPv6(rules[i]->def)) {
- if (iptablesRuleInstCommand(fw,
- ifname,
- rules[i]) < 0)
- goto cleanup;
- }
- }
-
- iptablesCheckBridgeNFCallEnabled(true);
+ iptablesCreateChainsAndRules(fw, VIR_FIREWALL_LAYER_IPV4,
&chainCallbackData);
+ } if (haveIp6tables) {
+ iptablesCreateChainsAndRules(fw, VIR_FIREWALL_LAYER_IPV6,
&chainCallbackData);
}
if (virHashSize(chains_in_set) != 0)
diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c
index b78b1b7947..3da28521b0 100644
--- a/tests/nwfilterxml2firewalltest.c
+++ b/tests/nwfilterxml2firewalltest.c
@@ -67,6 +67,14 @@ static const char *commonRules[] = {
"ebtables \\\n--concurrent \\\n-t nat \\\n-N libvirt-J-vnet0\n"
"ebtables \\\n--concurrent \\\n-t nat \\\n-N libvirt-P-vnet0\n",
+ /* Querying existence of rules */
+ "iptables \\\n-w \\\n-L\n",
+ "ip6tables \\\n-w \\\n-L\n",
+
+ /* Inserting ebtables rules */
+ "ebtables \\\n--concurrent \\\n-t nat \\\n-A PREROUTING \\\n-i vnet0 \\\n-j
libvirt-J-vnet0\n"
+ "ebtables \\\n--concurrent \\\n-t nat \\\n-A POSTROUTING \\\n-o vnet0 \\\n-j
libvirt-P-vnet0\n",
+
/* Dropping iptables rules */
"iptables \\\n-w \\\n-D libvirt-out \\\n-m physdev \\\n--physdev-is-bridged
\\\n--physdev-out vnet0 \\\n-g FP-vnet0\n"
"iptables \\\n-w \\\n-D libvirt-out \\\n-m physdev \\\n--physdev-out vnet0
\\\n-g FP-vnet0\n"
@@ -81,16 +89,16 @@ static const char *commonRules[] = {
/* Creating iptables chains */
"iptables \\\n-w \\\n-N libvirt-in\n"
- "iptables \\\n-w \\\n-N libvirt-out\n"
- "iptables \\\n-w \\\n-N libvirt-in-post\n"
- "iptables \\\n-w \\\n-N libvirt-host-in\n"
"iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in\n"
- "iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-out\n"
- "iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in-post\n"
- "iptables \\\n-w \\\n-D INPUT \\\n-j libvirt-host-in\n"
"iptables \\\n-w \\\n-I FORWARD 1 \\\n-j libvirt-in\n"
+ "iptables \\\n-w \\\n-N libvirt-out\n"
+ "iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-out\n"
"iptables \\\n-w \\\n-I FORWARD 2 \\\n-j libvirt-out\n"
+ "iptables \\\n-w \\\n-N libvirt-in-post\n"
+ "iptables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in-post\n"
"iptables \\\n-w \\\n-I FORWARD 3 \\\n-j libvirt-in-post\n"
+ "iptables \\\n-w \\\n-N libvirt-host-in\n"
+ "iptables \\\n-w \\\n-D INPUT \\\n-j libvirt-host-in\n"
"iptables \\\n-w \\\n-I INPUT 1 \\\n-j libvirt-host-in\n"
"iptables \\\n-w \\\n-N FP-vnet0\n"
"iptables \\\n-w \\\n-N FJ-vnet0\n"
@@ -115,16 +123,16 @@ static const char *commonRules[] = {
/* Creating ip6tables chains */
"ip6tables \\\n-w \\\n-N libvirt-in\n"
- "ip6tables \\\n-w \\\n-N libvirt-out\n"
- "ip6tables \\\n-w \\\n-N libvirt-in-post\n"
- "ip6tables \\\n-w \\\n-N libvirt-host-in\n"
"ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in\n"
- "ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-out\n"
- "ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in-post\n"
- "ip6tables \\\n-w \\\n-D INPUT \\\n-j libvirt-host-in\n"
"ip6tables \\\n-w \\\n-I FORWARD 1 \\\n-j libvirt-in\n"
+ "ip6tables \\\n-w \\\n-N libvirt-out\n"
+ "ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-out\n"
"ip6tables \\\n-w \\\n-I FORWARD 2 \\\n-j libvirt-out\n"
+ "ip6tables \\\n-w \\\n-N libvirt-in-post\n"
+ "ip6tables \\\n-w \\\n-D FORWARD \\\n-j libvirt-in-post\n"
"ip6tables \\\n-w \\\n-I FORWARD 3 \\\n-j libvirt-in-post\n"
+ "ip6tables \\\n-w \\\n-N libvirt-host-in\n"
+ "ip6tables \\\n-w \\\n-D INPUT \\\n-j libvirt-host-in\n"
"ip6tables \\\n-w \\\n-I INPUT 1 \\\n-j libvirt-host-in\n"
"ip6tables \\\n-w \\\n-N FP-vnet0\n"
"ip6tables \\\n-w \\\n-N FJ-vnet0\n"
@@ -134,10 +142,6 @@ static const char *commonRules[] = {
"ip6tables \\\n-w \\\n-A libvirt-host-in \\\n-m physdev \\\n--physdev-in vnet0
\\\n-g HJ-vnet0\n"
"ip6tables \\\n-w \\\n-D libvirt-in-post \\\n-m physdev \\\n--physdev-in vnet0
\\\n-j ACCEPT\n"
"ip6tables \\\n-w \\\n-A libvirt-in-post \\\n-m physdev \\\n--physdev-in vnet0
\\\n-j ACCEPT\n",
-
- /* Inserting ebtables rules */
- "ebtables \\\n--concurrent \\\n-t nat \\\n-A PREROUTING \\\n-i vnet0 \\\n-j
libvirt-J-vnet0\n"
- "ebtables \\\n--concurrent \\\n-t nat \\\n-A POSTROUTING \\\n-o vnet0 \\\n-j
libvirt-P-vnet0\n",
};
@@ -342,6 +346,26 @@ static int testSetDefaultParameters(GHashTable *vars)
return 0;
}
+static void
+testCommandDryRunCallback(const char *const*args,
+ const char *const*env G_GNUC_UNUSED,
+ const char *input G_GNUC_UNUSED,
+ char **output,
+ char **error G_GNUC_UNUSED,
+ int *status,
+ void *opaque G_GNUC_UNUSED)
+{
+ if (!STREQ(args[0], "iptables") && !STREQ(args[0],
"ip6tables")) {
+ return;
+ }
+
+ // simulate an empty existing set rules
+ if (STREQ(args[1], "-w") && STREQ(args[2], "-L")) {
+ *output = g_strdup("Chain nothing\n");
+ *status = EXIT_SUCCESS;
+ }
+}
+
static int testCompareXMLToArgvFiles(const char *xml,
const char *cmdline)
{
@@ -352,7 +376,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
int ret = -1;
g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew();
- virCommandSetDryRun(dryRunToken, &buf, true, true, NULL, NULL);
+ virCommandSetDryRun(dryRunToken, &buf, true, true, testCommandDryRunCallback,
NULL);
if (testSetDefaultParameters(vars) < 0)
goto cleanup;
--
2.43.0
1
days inactive
1
days old
1 comments
1 participants
participants (1)
-
Dion Bosschieter